Conversation
Updated test environment against which the tests are running.
Extended the Polling Challenge tests to cover approval flows with QR Code...
…oleanCollector (agreement checkbox).
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds ZXing test dependency, a large polling flow JSON and end-to-end polling tests (including QR approval), expands password-policy validation tests, extends form-field instrumentation tests (multi-link labels, phone object format, agreement flow), and pins JaCoCo version for test coverage. ChangesPolling and Form Field Test Coverage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
davinci/src/androidTest/kotlin/com/pingidentity/davinci/FormFieldValidationTest.kt (1)
2-2:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate modified-file copyright year range.
Line 2 should use the modified-file range format (e.g.,
2025-2026) per repo rule.Based on learnings, existing Kotlin/Java files modified in this repo should use a year range from original year through current year.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@davinci/src/androidTest/kotlin/com/pingidentity/davinci/FormFieldValidationTest.kt` at line 2, Update the copyright header in FormFieldValidationTest.kt to use the modified-file year range format (e.g., replace the single year "2025" on line 2 with a range like "2025-2026"); locate the top-of-file copyright comment in FormFieldValidationTest.kt and change the year to span from the original copyright year through the current year per repo rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@davinci/src/androidTest/kotlin/com/pingidentity/davinci/FormFieldsTest.kt`:
- Around line 714-735: The test
phoneNumberSubmissionWithEmptyPhoneOmitsFieldTest currently never advances past
setting the submit value, so it only checks local collector behavior; to
actually exercise the POST/server path call node = node.next() after setting
(node.collectors[SUBMIT_BUTTON_INDEX] as SubmitCollector).value = "Submit" to
trigger submission and then assert the server-side validation response (or,
alternatively, rename the test and its comments to indicate it only verifies
local validate()/payload() behavior); update assertions accordingly to either
expect a server error after advancing the node or keep the existing local
assertions if you rename/scope the test.
In
`@davinci/src/androidTest/kotlin/com/pingidentity/davinci/FormFieldValidationTest.kt`:
- Around line 32-43: Remove the duplicate import of assertFalse in
FormFieldValidationTest.kt so only one assertion API is used; specifically
delete the import line referencing junit.framework.TestCase.assertFalse (leave
kotlin.test.assertFalse) and ensure all uses of assertFalse in the file remain
unqualified to consistently resolve to kotlin.test.assertFalse throughout the
test class.
In
`@davinci/src/androidTest/kotlin/com/pingidentity/davinci/PollingCollectorE2ETests.kt`:
- Around line 43-44: The test class PollingCollectorE2ETests is incorrectly
marked `@SmallTest`; change the annotation to `@LargeTest` to reflect network-backed
E2E behavior, and in the approval/polling flow replace any direct
URL.openStream() calls with an HttpURLConnection (or URLConnection) where you
set connect and read timeouts (setConnectTimeout/setReadTimeout) and avoid
unbounded Thread.sleep/fixed delays by implementing a polling strategy with
bounded overall timeout (e.g., total deadline + retry/backoff loop or
exponential backoff) so the tests cannot hang indefinitely.
- Around line 236-244: Replace the fragile fixed-delay approval in
challengePollingApproval and qrCodeChallengePollingApproval by triggering the
approval when a poll event is observed: subscribe to
pollingCollector.pollStatus() and, upon receiving the first (or appropriate)
emitted status, call the existing openApprovalUrl(...) helper to perform the
approval; remove the hard-coded delay(3000L) and ensure the approval coroutine
runs only after observing the poll emission. Also modify openApprovalUrl(...)
(or the URL-opening path used by the tests) to use a URLConnection with explicit
connect and read timeouts (non-zero) so the approval HTTP request cannot hang
indefinitely. Ensure references: pollingCollector.pollStatus(),
challengePollingApproval, qrCodeChallengePollingApproval, and
openApprovalUrl(...) are updated accordingly.
---
Outside diff comments:
In
`@davinci/src/androidTest/kotlin/com/pingidentity/davinci/FormFieldValidationTest.kt`:
- Line 2: Update the copyright header in FormFieldValidationTest.kt to use the
modified-file year range format (e.g., replace the single year "2025" on line 2
with a range like "2025-2026"); locate the top-of-file copyright comment in
FormFieldValidationTest.kt and change the year to span from the original
copyright year through the current year per repo rules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b7883a31-9422-4c66-8ff0-ac5b2497ad75
📒 Files selected for processing (5)
davinci/build.gradle.ktsdavinci/src/androidTest/kotlin/com/pingidentity/davinci/FormFieldValidationTest.ktdavinci/src/androidTest/kotlin/com/pingidentity/davinci/FormFieldsTest.ktdavinci/src/androidTest/kotlin/com/pingidentity/davinci/Polling-Collector-Tests-Flows.jsondavinci/src/androidTest/kotlin/com/pingidentity/davinci/PollingCollectorE2ETests.kt
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #204 +/- ##
=============================================
+ Coverage 42.64% 44.94% +2.29%
- Complexity 1287 1389 +102
=============================================
Files 312 312
Lines 9447 9465 +18
Branches 1403 1429 +26
=============================================
+ Hits 4029 4254 +225
+ Misses 4862 4650 -212
- Partials 556 561 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
build-logic/convention/src/main/kotlin/com/pingidentity/convention/JacocoPlugin.kt (1)
2-2:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate copyright year range for a modified Kotlin file.
Line 2 should include the current year (2026) for a modified existing file; otherwise this violates repository copyright-header compliance.
Proposed fix
- * Copyright (c) 2024 - 2025 Ping Identity Corporation. All rights reserved. + * Copyright (c) 2024 - 2026 Ping Identity Corporation. All rights reserved.Based on learnings: In the ForgeRock/ping-android-sdk repository, existing modified Kotlin/Java files must use a creation-year-through-current-year range.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/convention/src/main/kotlin/com/pingidentity/convention/JacocoPlugin.kt` at line 2, Update the copyright header in JacocoPlugin.kt to include the current year (2026) by changing the creation-year range to end with 2026 (e.g., "2024 - 2026") so the modified Kotlin file complies with repository copyright-header rules; locate the top-of-file comment in build-logic/convention/src/main/kotlin/com/pingidentity/convention/JacocoPlugin.kt and replace the existing year range on the second line accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@build-logic/convention/src/main/kotlin/com/pingidentity/convention/JacocoPlugin.kt`:
- Line 2: Update the copyright header in JacocoPlugin.kt to include the current
year (2026) by changing the creation-year range to end with 2026 (e.g., "2024 -
2026") so the modified Kotlin file complies with repository copyright-header
rules; locate the top-of-file comment in
build-logic/convention/src/main/kotlin/com/pingidentity/convention/JacocoPlugin.kt
and replace the existing year range on the second line accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aed51049-029e-4b58-9c17-07526b0e6df7
📒 Files selected for processing (1)
build-logic/convention/src/main/kotlin/com/pingidentity/convention/JacocoPlugin.kt
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@davinci/src/androidTest/kotlin/com/pingidentity/davinci/FormFieldValidationTest.kt`:
- Around line 310-313: The two assertions use the wrong argument order and call
.toString()/!! which can throw NPEs before the assertion; change them to call
assertNotNull with the actual nullable values first and the message second, e.g.
assertNotNull(passwordField, "PASSWORD_VERIFY field not found in form
components") and assertNotNull(passwordField?.get("passwordPolicy"),
"passwordPolicy must be embedded inside the PASSWORD_VERIFY field") (remove
.toString() and !! so you assert the raw nullable values instead of masking
nulls).
In
`@davinci/src/androidTest/kotlin/com/pingidentity/davinci/PollingCollectorE2ETests.kt`:
- Around line 247-253: The test can hang because firstContinueSeen.await() (and
similarly approvalJob.await()) may never complete; wrap these awaits in a
coroutine timeout (e.g., use kotlinx.coroutines.withTimeout or
withTimeoutOrNull) so the test fails fast instead of hanging. Locate the await
calls (firstContinueSeen.await() and the approvalJob.await() inside the
magic-link and QR-code approval flows) and replace them with a timed variant
(for example: withTimeout(30_000) { firstContinueSeen.await() } or use
withTimeoutOrNull and assert null as a failure), ensuring you import
kotlinx.coroutines.withTimeout/withTimeoutOrNull.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f7bd7075-c514-47dc-944b-acd2e64481b7
📒 Files selected for processing (4)
davinci/src/androidTest/kotlin/com/pingidentity/davinci/FormFieldValidationTest.ktdavinci/src/androidTest/kotlin/com/pingidentity/davinci/FormFieldsTest.ktdavinci/src/androidTest/kotlin/com/pingidentity/davinci/PollingCollectorE2ETests.ktgradle.properties
✅ Files skipped from review due to trivial changes (1)
- gradle.properties
| assertNotNull("PASSWORD_VERIFY field not found in form components", passwordField.toString()) | ||
| assertNotNull( | ||
| "passwordPolicy must be embedded inside the PASSWORD_VERIFY field", | ||
| passwordField!!["passwordPolicy"].toString()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="davinci/src/androidTest/kotlin/com/pingidentity/davinci/FormFieldValidationTest.kt"
echo "Checking for message-first assertNotNull usage:"
rg -n 'assertNotNull\(\s*"[^"]+"\s*,\s*' "$FILE"
echo
echo "Checking for toString()-based null assertions:"
rg -n 'assertNotNull\([^)]*toString\(\)' "$FILE"Repository: ForgeRock/ping-android-sdk
Length of output: 378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="davinci/src/androidTest/kotlin/com/pingidentity/davinci/FormFieldValidationTest.kt"
nl -ba "$FILE" | sed -n '290,340p'Repository: ForgeRock/ping-android-sdk
Length of output: 113
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="davinci/src/androidTest/kotlin/com/pingidentity/davinci/FormFieldValidationTest.kt"
awk 'NR>=290 && NR<=340 {printf "%d:%s\n", NR, $0}' "$FILE"Repository: ForgeRock/ping-android-sdk
Length of output: 2428
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="davinci/src/androidTest/kotlin/com/pingidentity/davinci/FormFieldValidationTest.kt"
awk 'NR<=80{printf "%d:%s\n", NR, $0}' "$FILE"Repository: ForgeRock/ping-android-sdk
Length of output: 3262
Fix assertNotNull argument order and remove null-masking .toString()/!!
In FormFieldValidationTest.kt lines 310-313, kotlin.test.assertNotNull is called as assertNotNull("message", <value>.toString()), so the non-null message string is used as the actual value (the assertion can pass even when passwordField/passwordPolicy are null). Also, passwordField.toString() / passwordField!!["passwordPolicy"].toString() can throw NPE before the assertion message is surfaced.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@davinci/src/androidTest/kotlin/com/pingidentity/davinci/FormFieldValidationTest.kt`
around lines 310 - 313, The two assertions use the wrong argument order and call
.toString()/!! which can throw NPEs before the assertion; change them to call
assertNotNull with the actual nullable values first and the message second, e.g.
assertNotNull(passwordField, "PASSWORD_VERIFY field not found in form
components") and assertNotNull(passwordField?.get("passwordPolicy"),
"passwordPolicy must be embedded inside the PASSWORD_VERIFY field") (remove
.toString() and !! so you assert the raw nullable values instead of masking
nulls).
| firstContinueSeen.await() | ||
| (java.net.URL(magicLink).openConnection() as java.net.HttpURLConnection).apply { | ||
| connectTimeout = 10_000 | ||
| readTimeout = 10_000 | ||
| connect() | ||
| disconnect() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="davinci/src/androidTest/kotlin/com/pingidentity/davinci/PollingCollectorE2ETests.kt"
echo "Inspect approval await blocks:"
nl -ba "$FILE" | sed -n '236,255p'
nl -ba "$FILE" | sed -n '360,373p'
echo
echo "Check for timeout-guarded awaits:"
rg -n 'firstContinueSeen\.await\(\)|withTimeout\(' "$FILE"Repository: ForgeRock/ping-android-sdk
Length of output: 162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="davinci/src/androidTest/kotlin/com/pingidentity/davinci/PollingCollectorE2ETests.kt"
echo "=== Context around lines ~240-260 ==="
sed -n '230,270p' "$FILE" | awk '{
# best-effort: annotate with approximate line numbers using NR + (start-1)
}'
echo "=== Context around lines ~350-390 ==="
sed -n '350,390p' "$FILE"
echo
echo "=== All occurrences of firstContinueSeen.await() in the file ==="
rg -n --no-heading 'firstContinueSeen\.await\(\)' "$FILE" || true
echo
echo "=== Any existing timeout guard usage in the file ==="
rg -n --no-heading 'withTimeout\(|Timeout' "$FILE" || trueRepository: ForgeRock/ping-android-sdk
Length of output: 2577
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="davinci/src/androidTest/kotlin/com/pingidentity/davinci/PollingCollectorE2ETests.kt"
echo "=== File header/context (top ~1-120) ==="
sed -n '1,120p' "$FILE"
echo
echo "=== Tests/helpers with polling timeouts (~120-220) ==="
sed -n '120,220p' "$FILE"
echo
echo "=== Context around firstContinueSeen.await() `#1` (~220-270) ==="
sed -n '220,270p' "$FILE"
echo
echo "=== Context around firstContinueSeen.await() `#2` (~330-390) ==="
sed -n '330,390p' "$FILE"Repository: ForgeRock/ping-android-sdk
Length of output: 15782
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate PollingCollector implementation + PollingStatus definitions
rg -n --no-heading "class PollingCollector|fun pollStatus\(" -S davinci/src main/src test/src androidTest/src . || true
# Narrow to davinci collector package
rg -n --no-heading "class PollingCollector|fun pollStatus\(|sealed class PollingStatus|class .*PollingStatus|enum class .*PollingStatus" -S davinci/src . || true
# List files that look relevant
fd -t f -e kt -e kts -e java "PollingCollector" davinci/src . 2>/dev/null || true
fd -t f -e kt -e kts -e java "PollingStatus" davinci/src . 2>/dev/null || trueRepository: ForgeRock/ping-android-sdk
Length of output: 4028
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="davinci/src/main/kotlin/com/pingidentity/davinci/collector/PollingCollector.kt"
echo "=== File length ==="
wc -l "$FILE"
echo
echo "=== PollingStatus definitions (~40-110) ==="
sed -n '40,120p' "$FILE"
echo
echo "=== PollingCollector.pollStatus() (~260-380) ==="
sed -n '260,380p' "$FILE"
echo
echo "=== Any mention of TimedOut / retry logic near pollStatus() (~200-320) ==="
rg -n --no-heading "TimedOut|retry|maxRetries|pollRetries|pollInterval|delay|timeout" "$FILE" | head -n 80Repository: ForgeRock/ping-android-sdk
Length of output: 13057
Add a timeout around firstContinueSeen.await() to prevent CI hangs
PollingCollector.pollStatus() can emit Expired, Error, or Complete without ever emitting the first PollingStatus.Continue; in that case approvalJob.await() will never complete (both the magic-link approval at lines 247-253 and the QR-code approval at 366-372).
🔧 Minimal hardening diff
import kotlinx.coroutines.runBlocking
+import kotlinx.coroutines.withTimeout
import kotlinx.coroutines.withContext
@@
val approvalJob = async(Dispatchers.IO) {
- firstContinueSeen.await()
+ withTimeout(20_000L) { firstContinueSeen.await() }
(java.net.URL(magicLink).openConnection() as java.net.HttpURLConnection).apply {
connectTimeout = 10_000
readTimeout = 10_000
connect()
disconnect()
}
}
@@
val approvalJob = async(Dispatchers.IO) {
- firstContinueSeen.await()
+ withTimeout(20_000L) { firstContinueSeen.await() }
(java.net.URL(approvalUrl).openConnection() as java.net.HttpURLConnection).apply {
connectTimeout = 10_000
readTimeout = 10_000
connect()
disconnect()
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@davinci/src/androidTest/kotlin/com/pingidentity/davinci/PollingCollectorE2ETests.kt`
around lines 247 - 253, The test can hang because firstContinueSeen.await() (and
similarly approvalJob.await()) may never complete; wrap these awaits in a
coroutine timeout (e.g., use kotlinx.coroutines.withTimeout or
withTimeoutOrNull) so the test fails fast instead of hanging. Locate the await
calls (firstContinueSeen.await() and the approvalJob.await() inside the
magic-link and QR-code approval flows) and replace them with a timed variant
(for example: withTimeout(30_000) { firstContinueSeen.await() } or use
withTimeoutOrNull and assert null as a failure), ensuring you import
kotlinx.coroutines.withTimeout/withTimeoutOrNull.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
build-logic/convention/src/main/kotlin/com/pingidentity/convention/AndroidLibraryConventionPlugin.kt (1)
2-2:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate copyright year to include 2026.
This file is being modified in 2026, so the copyright year range should extend through the current year. Based on learnings, modified files should use a year range from the original creation year through the current year.
📅 Proposed fix
- * Copyright (c) 2024 - 2025 Ping Identity Corporation. All rights reserved. + * Copyright (c) 2024 - 2026 Ping Identity Corporation. All rights reserved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/convention/src/main/kotlin/com/pingidentity/convention/AndroidLibraryConventionPlugin.kt` at line 2, Update the copyright header in AndroidLibraryConventionPlugin.kt by extending the year range to include 2026: locate the file-level comment line that currently reads "Copyright (c) 2024 - 2025 Ping Identity Corporation. All rights reserved." and change the end year to 2026 so it reads through 2026; ensure you only modify that header text and do not alter the AndroidLibraryConventionPlugin class or other code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@build-logic/convention/src/main/kotlin/com/pingidentity/convention/AndroidLibraryConventionPlugin.kt`:
- Line 2: Update the copyright header in AndroidLibraryConventionPlugin.kt by
extending the year range to include 2026: locate the file-level comment line
that currently reads "Copyright (c) 2024 - 2025 Ping Identity Corporation. All
rights reserved." and change the end year to 2026 so it reads through 2026;
ensure you only modify that header text and do not alter the
AndroidLibraryConventionPlugin class or other code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 31e8f57d-c753-4aeb-9a20-d4b195147b71
📒 Files selected for processing (2)
build-logic/convention/src/main/kotlin/com/pingidentity/convention/AndroidLibraryConventionPlugin.ktgradle.properties
💤 Files with no reviewable changes (1)
- gradle.properties
| namespace = "com.pingidentity.${project.name.replace("-", ".")}" | ||
| // bcprov-jdk18on 1.83+ contains Java 25 multi-release class files (major version 69). | ||
| // AGP's default JaCoCo 0.8.12 (ASM 9.7) cannot instrument them; 0.8.13 (ASM 9.8) can. | ||
| testCoverage { jacocoVersion = "0.8.13" } |
There was a problem hiding this comment.
PR #201 broke the pipeline. This fixes it...
| clientId = "021b83ce-a9b1-4ad4-8c1d-79e576eeab76" | ||
| discoveryEndpoint = "https://auth.pingone.ca/02fb4743-189a-4bc7-9d6c-a919edfe6447/as/.well-known/openid-configuration" | ||
| clientId = "a6859a12-5e6e-4f64-96bb-cc8577706bee" | ||
| discoveryEndpoint = "https://auth.pingone.ca/300c4f2a-39d4-4ba9-a18a-f6de246006f4/as/.well-known/openid-configuration" |
There was a problem hiding this comment.
This is the new shared environment dedicated to QA e2e tests... We are gonna gradually switch to this one...
| } | ||
|
|
||
| @Test | ||
| fun passwordClearPasswordAfterCloseTest() = runTest { |
There was a problem hiding this comment.
Incorrect assertNotNull signature — assertion message and value are swapped
assertNotNull("PASSWORD_VERIFY field not found in form components", passwordField.toString())
assertNotNull("passwordPolicy must be embedded inside the PASSWORD_VERIFY field", passwordField!!["passwordPolicy"].toString())kotlin.test.assertNotNull(message, actual) was deprecated — the two-arg overload is assertNotNull(actual, message). As written, the string literal is always non-null, so this assertion never fails regardless of whether passwordField is actually null.
Also, calling .toString() on a nullable before the null-check defeats the purpose entirely — null.toString() returns "null" (a non-null string), so the assertion passes even when the value is missing.
Fix:
assertNotNull(passwordField, "PASSWORD_VERIFY field not found in form components")
assertNotNull(passwordField!!["passwordPolicy"], "passwordPolicy must be embedded inside the PASSWORD_VERIFY field")
| androidTestImplementation(libs.androidx.test.runner) | ||
| androidTestImplementation(project(":foundation:testrail")) | ||
| androidTestImplementation(project(":protect")) | ||
| androidTestImplementation("com.google.zxing:core:3.5.3") |
There was a problem hiding this comment.
ZXing dependency not version-catalogued
androidTestImplementation("com.google.zxing:core:3.5.3")Every other dependency in this file uses the libs.* version catalog alias (e.g. libs.androidx.test.runner). Adding a hard-coded GAV string bypasses the catalog, making it invisible to dependencyUpdates checks and inconsistent with the project style.
Add zxing-core = { module = "com.google.zxing:core", version = "3.5.3" } to gradle/libs.versions.toml and reference it as libs.zxing.core here.
| import com.pingidentity.davinci.plugin.collectors | ||
| import com.pingidentity.logger.Logger | ||
| import com.pingidentity.logger.STANDARD | ||
| import com.pingidentity.orchestrate.ContinueNode |
There was a problem hiding this comment.
runBlocking + withContext(Dispatchers.IO) in tests instead of runTest
challengePollingTimeout, challengePollingApproval, qrCodeChallengePollingTimeout, and qrCodeChallengePollingApproval all use runBlocking with explicit withContext(Dispatchers.IO) wrappers. The simpler polling tests use runTest directly.
runTest is the preferred coroutine test runner; it integrates with TestCoroutineScheduler, supports virtual time, and avoids blocking the test thread. Using runBlocking forces real-time waits and makes the tests slower than necessary.
If the reason is that pollStatus() involves real network I/O that cannot be faked out, that is fine — but the pattern should at least be documented here so future maintainers do not wonder why the inconsistency exists.
|
|
||
| // Cycle 1 — retriesAllowed 3→2, emits Complete("continue"); server rewinds to same form | ||
| var statuses = pollingCollector.pollStatus().toList() | ||
| assertEquals(1, statuses.size) |
There was a problem hiding this comment.
continuePollingTimeout reuses the same pollingCollector instance across cycles
var node = node.next() as ContinueNode // server rewinds to same form
// ...
statuses = pollingCollector.pollStatus().toList() // called on the old collectorAfter node.next(), the server returns a fresh ContinueNode — but the test continues calling pollStatus() on the pollingCollector extracted from the previous node. If the SDK returns a new collector instance for the rewound form, the retriesAllowed counter may be double-counted or the state may be stale.
The test should re-extract the collector after each node.next():
node = node.next() as ContinueNode
val pollingCollector = node.collectors.filterIsInstance<PollingCollector>().first()Or document explicitly that the same collector instance is intentionally shared across cycles (if the SDK guarantees this).
| node = node.next() as ContinueNode | ||
|
|
||
| assertEquals("Automation - Polling", node.name) | ||
| val pollingCollector = node.collectors.filterIsInstance<PollingCollector>().first() |
There was a problem hiding this comment.
pollInterval and pollRetries are stringly-typed comparisons
assertEquals("2000", pollingCollector.pollInterval)
assertEquals("3", pollingCollector.pollRetries)If pollInterval and pollRetries are exposed as String properties it is better to either:
- expose them as
Int/Longin the SDK and compare numerically, or - at least assert
pollingCollector.pollInterval.toInt() == 2000so the assertion message shows a meaningful value mismatch rather than a string difference.
Also worth noting: continuePollingTimeout asserts these as strings, but challengePollingApproval uses pollingCollector.pollInterval.toInt() > 0 — the inconsistency suggests the string API is an accident rather than a design choice.
2 participants
JIRA Ticket
SDKS-4249 [QA] Support for new DaVinci collectors
Description
New tests added in FormFieldsTest.kt, FormFieldValidationTest.kt, and PollingCollectorE2ETests.kt covering the following collectors/features:
Summary by CodeRabbit
New Features
Tests
Chores