Conversation
📝 WalkthroughWalkthroughAdds metadata and image ACR configuration, a miscellaneous collector flow fixture, and Android end-to-end tests for metadata and image collector behavior. ChangesDaVinci collector E2E coverage
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant MetadataCollectorE2ETest
participant ImageCollectorE2ETest
participant DaVinciFlow
participant MetadataCollector
participant ImageCollector
participant ResultForm
MetadataCollectorE2ETest->>DaVinciFlow: Navigate to metadata node
DaVinciFlow->>MetadataCollector: Create metadata collector
MetadataCollectorE2ETest->>MetadataCollector: Set result or error
MetadataCollector->>ResultForm: Advance to success or error branch
ImageCollectorE2ETest->>DaVinciFlow: Navigate to image node
DaVinciFlow->>ImageCollector: Create image collector
ImageCollectorE2ETest->>ImageCollector: Inspect image properties
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (3)
davinci/src/androidTest/kotlin/com/pingidentity/davinci/MetadataCollectorE2ETest.kt (3)
129-132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the validation error by type, not by
toString().
validateRequiresResponseBeforeSubmitassertserrors[0].toString()equals"Required".Requiredis a publicdata object; keep the test focused on the validation contract instead of the object’s string representation.♻️ Proposed fix
val errors = collector.validate() assertTrue(errors.isNotEmpty()) - assertEquals("Required", errors[0].toString()) + assertIs<Required>(errors[0])Add the imports:
import com.pingidentity.davinci.collector.Required import kotlin.test.assertIs🤖 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/MetadataCollectorE2ETest.kt` around lines 129 - 132, Update validateRequiresResponseBeforeSubmit to assert the first validation error is of type Required using assertIs, replacing the errors[0].toString() equality check while retaining the non-empty assertion.
21-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInstall the TestRail watcher rule and label the tests.
MetadataCollectorE2ETestimportsTestRailWatcher/Rule/TestWatcherandTestRailCase, but it declares no watcher rule and its@Testfunctions lack@TestRailCase. As a result, these tests do not report TestRail results. Add the watcher rule and annotate the test methods, or remove the unused imports if reporting is not required.🤖 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/MetadataCollectorE2ETest.kt` around lines 21 - 22, Update MetadataCollectorE2ETest to install a TestRail watcher rule using the imported TestRailWatcher/Rule/TestWatcher types and annotate each `@Test` method with TestRailCase so results are reported; if reporting is intentionally not required, remove the unused TestRail imports instead.Source: Learnings
58-61: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse type-based collector lookup for e2e form assertions.
LABEL_INDEX = 0andSUBMIT_INDEX = 1match this form only becauseERROR_DISPLAYdoes not create a collector in the current registry. If that behavior changes, these casts fail. Look up collectors by type, such asnode.collectors.filterIsInstance<LabelCollector>().first()orfilterIsInstance<SubmitCollector>().first(), before asserting values.🤖 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/MetadataCollectorE2ETest.kt` around lines 58 - 61, Update the e2e form assertions in MetadataCollectorE2ETest to stop relying on LABEL_INDEX and SUBMIT_INDEX. Retrieve the relevant collectors from node.collectors using filterIsInstance<LabelCollector>() and filterIsInstance<SubmitCollector>(), then assert their values while preserving the existing expectations.
🤖 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/MetadataCollectorE2ETest.kt`:
- Line 48: Replace the `@SmallTest` annotation on MetadataCollectorE2ETest with
`@LargeTest` so this live-network E2E suite is classified for the appropriate test
execution path.
---
Nitpick comments:
In
`@davinci/src/androidTest/kotlin/com/pingidentity/davinci/MetadataCollectorE2ETest.kt`:
- Around line 129-132: Update validateRequiresResponseBeforeSubmit to assert the
first validation error is of type Required using assertIs, replacing the
errors[0].toString() equality check while retaining the non-empty assertion.
- Around line 21-22: Update MetadataCollectorE2ETest to install a TestRail
watcher rule using the imported TestRailWatcher/Rule/TestWatcher types and
annotate each `@Test` method with TestRailCase so results are reported; if
reporting is intentionally not required, remove the unused TestRail imports
instead.
- Around line 58-61: Update the e2e form assertions in MetadataCollectorE2ETest
to stop relying on LABEL_INDEX and SUBMIT_INDEX. Retrieve the relevant
collectors from node.collectors using filterIsInstance<LabelCollector>() and
filterIsInstance<SubmitCollector>(), then assert their values while preserving
the existing expectations.
🪄 Autofix
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 Plus
Run ID: 15779947-5e0b-4032-b969-310145254dff
📒 Files selected for processing (4)
davinci/src/androidTest/kotlin/com/pingidentity/davinci/DaVinciTestConfig.ktdavinci/src/androidTest/kotlin/com/pingidentity/davinci/MetadataCollectorE2ETest.ktdavinci/src/androidTest/kotlin/com/pingidentity/davinci/Misc-Collectors-Flow.jsondavinci/src/main/assets/davinci_test_config.properties
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ImageCollectorE2ETest.kt`:
- Line 55: Align EXPECTED_HYPERLINK_URL with the current
Misc-Collectors-Flow.json fixture by setting it to the empty string, or update
the fixture to enable the hyperlink with the existing URL; keep the test
documentation and assertion behavior consistent with that choice.
🪄 Autofix
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 Plus
Run ID: 3d8f512e-ca51-4bc2-8d11-06e235da9355
📒 Files selected for processing (3)
davinci/src/androidTest/kotlin/com/pingidentity/davinci/DaVinciTestConfig.ktdavinci/src/androidTest/kotlin/com/pingidentity/davinci/ImageCollectorE2ETest.ktdavinci/src/main/assets/davinci_test_config.properties
🚧 Files skipped from review as they are similar to previous changes (2)
- davinci/src/androidTest/kotlin/com/pingidentity/davinci/DaVinciTestConfig.kt
- davinci/src/main/assets/davinci_test_config.properties
spetrov
changed the title
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
vibhorgoswami
left a comment
vibhorgoswami
left a comment
There was a problem hiding this comment.
I think the comment from Coderabbit is good. Otherwise all good.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
davinci/src/androidTest/kotlin/com/pingidentity/davinci/MetadataCollectorE2ETest.kt (1)
80-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHandle every
Nodepath before casting toContinueNode.
navigateToMetadataNode()castsdaVinci.start()and bothnode.next()calls unconditionally. Add a local helper that handlesContinueNode,SuccessNode,ErrorNode, andFailureNode, reports the non-ContinueNodeoutcome with its details, then uses the safe result for the subsequent collector/field checks. Apply the same pattern to the remainingnode.next() as ContinueNodesequences in the test.🤖 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/MetadataCollectorE2ETest.kt` around lines 80 - 87, Update navigateToMetadataNode and all remaining node.next() sequences to avoid unconditional ContinueNode casts. Add a local helper that accepts each Node outcome type (ContinueNode, SuccessNode, ErrorNode, and FailureNode), reports non-Continue outcomes with their details, and returns only a safe ContinueNode for collector and field assertions; use it for daVinci.start() and every subsequent transition.Source: Coding guidelines
🤖 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.
Nitpick comments:
In
`@davinci/src/androidTest/kotlin/com/pingidentity/davinci/MetadataCollectorE2ETest.kt`:
- Around line 80-87: Update navigateToMetadataNode and all remaining node.next()
sequences to avoid unconditional ContinueNode casts. Add a local helper that
accepts each Node outcome type (ContinueNode, SuccessNode, ErrorNode, and
FailureNode), reports non-Continue outcomes with their details, and returns only
a safe ContinueNode for collector and field assertions; use it for
daVinci.start() and every subsequent transition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c8fb3ef-67f1-4545-a582-28eea5d2006c
📒 Files selected for processing (2)
davinci/src/androidTest/kotlin/com/pingidentity/davinci/ImageCollectorE2ETest.ktdavinci/src/androidTest/kotlin/com/pingidentity/davinci/MetadataCollectorE2ETest.kt
💤 Files with no reviewable changes (1)
- davinci/src/androidTest/kotlin/com/pingidentity/davinci/ImageCollectorE2ETest.kt
2 participants
JIRA Ticket
SDKS-5281 Test new DaVinci components
Description
exchangeCustomMetadatacapability): collector shape, payload integrity,setResultsuccess path,setErrorerror path, validation gating, loop state isolation, and session recovery.ImageCollector.DAVINCI_METADATA_ACR_VALUESandDAVINCI_IMAGE_ACR_VALUESintodavinci_test_config.propertiesandDaVinciTestConfigto support the dedicated test flow.Summary by CodeRabbit