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).
Contributor
|
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}
Contributor
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.96% +2.31%
- Complexity 1287 1389 +102
=============================================
Files 312 312
Lines 9447 9465 +18
Branches 1403 1429 +26
=============================================
+ Hits 4029 4256 +227
+ Misses 4862 4650 -212
- Partials 556 559 +3
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:
|
Contributor
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
Contributor
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
Contributor
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
spetrov
commented
May 27, 2026
| 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" } |
Contributor
Author
There was a problem hiding this comment.
PR #201 broke the pipeline. This fixes it...
spetrov
commented
May 27, 2026
| 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" |
Contributor
Author
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...
witrisna
reviewed
May 27, 2026
witrisna
reviewed
May 27, 2026
witrisna
reviewed
May 27, 2026
witrisna
reviewed
May 27, 2026
witrisna
reviewed
May 27, 2026
witrisna
requested changes
May 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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