Skip to content

SDKS-5068: QA for the Unified SDK Configuration#213

Merged
spetrov merged 1 commit into
developfrom
SDKS-5068
Jun 23, 2026
Merged

SDKS-5068: QA for the Unified SDK Configuration#213
spetrov merged 1 commit into
developfrom
SDKS-5068

Conversation

@spetrov

@spetrov spetrov commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

JIRA Ticket

SDKS-5068 [QA] Standardize SDK Configuration

Description

Added instrumented test coverage for the JSON configuration factory functions DaVinci(json), Journey(json), OidcWebClient(json), OidcDeviceClient(json).
All tests live in the existing davinci and journey androidTest modules.

New test files

  • davinci/…/JsonConfigTest.kt — DaVinci(json) happy path (minimal + full fixture), default values for all optional fields, missing required field errors, unknown field tolerance, and wrong-type errors for timeout, clientId, scopes, and refreshThreshold
  • davinci/…/OidcDeviceClientJsonConfigTest.kt — OidcDeviceClient(json) success smoke tests (minimal + full fixture), missing required field, errors, unknown field tolerance, and wrong-type errors;
  • journey/…/JsonConfigTest.kt — Journey(json) happy path (minimal + full fixture), default values including par, Journey-specific defaults (realm, cookieName), custom cookieName round-trip, missing required field errors (including journey block), unknown field tolerance, and wrong-type errors for timeout, clientId, scopes, refreshThreshold, and cookieName
  • journey/…/OidcWebClientJsonConfigTest.kt — OidcWebClient(json) happy path (minimal + full fixture), default values including par = false, full optional field round-trip, openId endpoint override verification, missing required field errors, unknown field tolerance, and wrong-type errors

Summary by CodeRabbit

Release Notes

  • Tests
    • Added comprehensive test suites for JSON configuration parsing and validation across multiple modules.
    • Validates successful loading of minimal and full configurations with appropriate default value handling when optional fields are omitted.
    • Confirms error detection for missing required fields and invalid field types.
    • Validates handling of unknown configuration fields and endpoint override scenarios.
    • Added test configuration assets supporting multiple deployment configurations.

@coderabbitai

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds JSON test asset fixtures (minimal and full OIDC configurations) and Android instrumentation test suites for the davinci and journey modules. Tests cover JSON config parsing success, required-field validation errors, invalid-type errors, unknown-field tolerance, default value assertions, and OpenID endpoint override behavior for DaVinci, OidcDeviceClient, Journey, and OidcWebClient.

Changes

DaVinci and OidcDeviceClient JSON Config Tests

Layer / File(s) Summary
DaVinci test asset JSON fixtures
davinci/src/androidTest/assets/minimal-davinci.json, davinci/src/androidTest/assets/minimal-oidc-deviceclient.json, davinci/src/androidTest/assets/full-davinci-config.json, davinci/src/androidTest/assets/full-oidc-deviceclient.json
Adds minimal configs (required fields only: clientId, discoveryEndpoint, scopes, redirectUri) and full configs (all optional OIDC fields, PAR, additionalParameters, endpoint overrides) consumed by the instrumentation tests.
DaVinci JsonConfigTest suite
davinci/src/androidTest/kotlin/com/pingidentity/davinci/JsonConfigTest.kt
Tests DaVinci JSON parsing: minimal/full asset loading, default values, JsonConfigError.MissingRequiredField for missing oidc/clientId/discoveryEndpoint/scopes, JsonConfigError.InvalidType for wrong-typed fields, unknown-field tolerance, and openIdOverride endpoint assertion via OpenIdConfiguration.override().
DaVinci OidcDeviceClientJsonConfigTest suite
davinci/src/androidTest/kotlin/com/pingidentity/davinci/OidcDeviceClientJsonConfigTest.kt
Tests OidcDeviceClient JSON parsing: minimal/full asset success, missing-required-field errors with correct field names, unknown-field tolerance, and invalid-type errors for clientId, scopes, and refreshThreshold.

Journey and OidcWebClient JSON Config Tests

Layer / File(s) Summary
Journey build dependency and test asset JSON fixtures
journey/build.gradle.kts, journey/src/androidTest/assets/minimal-journey.json, journey/src/androidTest/assets/minimal-oidc-webclient.json, journey/src/androidTest/assets/full-journey-config.json, journey/src/androidTest/assets/full-oidc-webclient.json
Adds :foundation:browser as androidTestImplementation, and adds minimal configs (with journey.serverUrl and OIDC required fields) and full configs (with realm, cookie, all OIDC optional fields, PAR, endpoint overrides).
Journey JsonConfigTest suite
journey/src/androidTest/kotlin/com/pingidentity/journey/JsonConfigTest.kt
Tests Journey JSON parsing: minimal/full asset loading, defaults for realm/cookie/timeout/logger/OIDC optional fields, missing-required-field errors for journey and oidc blocks, unknown-field tolerance, invalid-type errors covering timeout/journey.cookieName/OIDC fields, and custom cookieName propagation.
Journey OidcWebClientJsonConfigTest suite
journey/src/androidTest/kotlin/com/pingidentity/journey/OidcWebClientJsonConfigTest.kt
Tests OidcWebClient JSON parsing: minimal/full asset loading, default value assertions, openId override endpoint assertions, missing-required-field errors, unknown-field tolerance, and invalid-type errors for timeout, oidc.clientId, oidc.scopes, and oidc.refreshThreshold.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • ForgeRock/ping-android-sdk#208: Introduces the JSON-driven construction entry points (DaVinci(jsonConfig), JsonConfigParser, OidcClientConfig endpoint override support) that the new test suites directly exercise and validate.
  • ForgeRock/ping-android-sdk#199: Adds device authorization/OpenID discovery and override behavior; the new tests assert device_authorization_endpoint values and openIdOverride endpoint mapping that depend on this PR's changes.
  • ForgeRock/ping-android-sdk#176: Introduces PAR support via OidcClientConfig.par; the new JSON assets and tests include par: true assertions that directly validate this field's parsing.

Suggested reviewers

  • witrisna
  • vahancouver

Poem

🐇 A rabbit hops through JSON trees,
checking fields with expertise—
clientId here, discoveryEndpoint there,
missing fields get caught with care!
Wrong types? Errors bubble right up.
Unknown fields? Ignored, no fuss.
Config parsing, tested with flare! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 2.86% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the main change: adding QA test coverage for unified SDK configuration validation.
Description check ✅ Passed The description includes the required JIRA ticket link and provides comprehensive details about the tests added, covering happy paths, default values, error handling, and edge cases.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch SDKS-5068

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
davinci/src/androidTest/kotlin/com/pingidentity/davinci/OidcDeviceClientJsonConfigTest.kt (1)

53-97: 🧹 Nitpick | 🔵 Trivial | 💤 Low value

Consider using array format for scopes to match JSON fixtures.

Line 58 defines scopes as a string "openid", while all JSON asset files consistently use array format. Although this is a placeholder value for the missing-field test, using array format would improve consistency with the fixtures and make the expected input format more explicit.

Lines 67-68 provide helpful context about the device flow not requiring redirectUri.

Suggested change for consistency
         val validOidc = buildJsonObject {
             put("clientId", "a6859a12-5e6e-4f64-96bb-cc8577706bee")
             put("discoveryEndpoint", "https://auth.pingone.ca/300c4f2a-39d4-4ba9-a18a-f6de246006f4/as/.well-known/openid-configuration")
-            put("scopes", "openid")
+            put("scopes", buildJsonArray { add("openid") })
         }
🤖 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/OidcDeviceClientJsonConfigTest.kt`
around lines 53 - 97, In the oidcDeviceClientMissingRequiredFieldReturnsError
function, the scopes field in the validOidc JsonObject is defined as a string
"openid" but should be an array format to maintain consistency with JSON asset
fixtures throughout the codebase. Change the scopes value to use array format
instead of string format when building the validOidc JsonObject.
davinci/src/androidTest/kotlin/com/pingidentity/davinci/JsonConfigTest.kt (1)

128-179: 🧹 Nitpick | 🔵 Trivial | 💤 Low value

Consider using array format for scopes to match JSON fixtures.

Line 134 defines scopes as a string "openid", while all JSON asset files consistently use array format (e.g., ["openid", "profile", "email"]). Although this test validates missing-field errors (so the scopes value is just a placeholder), using array format would improve consistency with the fixtures and make the expected input format more explicit.

Suggested change for consistency
         val base = buildJsonObject {
             put("oidc", buildJsonObject {
                 put("clientId", "a6859a12-5e6e-4f64-96bb-cc8577706bee")
                 put("discoveryEndpoint", "https://auth.pingone.ca/300c4f2a-39d4-4ba9-a18a-f6de246006f4/as/.well-known/openid-configuration")
-                put("scopes", "openid")
+                put("scopes", buildJsonArray { add("openid") })
                 put("redirectUri", "org.forgerock.demo://oauth2redirect")
             })
         }
🤖 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/JsonConfigTest.kt`
around lines 128 - 179, In the daVinciMissingRequiredFieldReturnsError test
function, the scopes field in the base JsonObject is currently defined as a
string "openid", but all JSON fixture files consistently use array format.
Modify the put statement for scopes in the base buildJsonObject to create a
JsonArray containing "openid" instead of a plain string value, to maintain
consistency with the fixture format used elsewhere in the codebase.
🤖 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/JsonConfigTest.kt`:
- Around line 128-179: In the daVinciMissingRequiredFieldReturnsError test
function, the scopes field in the base JsonObject is currently defined as a
string "openid", but all JSON fixture files consistently use array format.
Modify the put statement for scopes in the base buildJsonObject to create a
JsonArray containing "openid" instead of a plain string value, to maintain
consistency with the fixture format used elsewhere in the codebase.

In
`@davinci/src/androidTest/kotlin/com/pingidentity/davinci/OidcDeviceClientJsonConfigTest.kt`:
- Around line 53-97: In the oidcDeviceClientMissingRequiredFieldReturnsError
function, the scopes field in the validOidc JsonObject is defined as a string
"openid" but should be an array format to maintain consistency with JSON asset
fixtures throughout the codebase. Change the scopes value to use array format
instead of string format when building the validOidc JsonObject.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f1f28926-66a6-42a2-a399-99fb916c75d9

📥 Commits

Reviewing files that changed from the base of the PR and between ffc82b5 and 8f9a484.

📒 Files selected for processing (13)
  • davinci/src/androidTest/assets/full-davinci-config.json
  • davinci/src/androidTest/assets/full-oidc-deviceclient.json
  • davinci/src/androidTest/assets/minimal-davinci.json
  • davinci/src/androidTest/assets/minimal-oidc-deviceclient.json
  • davinci/src/androidTest/kotlin/com/pingidentity/davinci/JsonConfigTest.kt
  • davinci/src/androidTest/kotlin/com/pingidentity/davinci/OidcDeviceClientJsonConfigTest.kt
  • journey/build.gradle.kts
  • journey/src/androidTest/assets/full-journey-config.json
  • journey/src/androidTest/assets/full-oidc-webclient.json
  • journey/src/androidTest/assets/minimal-journey.json
  • journey/src/androidTest/assets/minimal-oidc-webclient.json
  • journey/src/androidTest/kotlin/com/pingidentity/journey/JsonConfigTest.kt
  • journey/src/androidTest/kotlin/com/pingidentity/journey/OidcWebClientJsonConfigTest.kt

@codecov

codecov Bot commented Jun 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 44.39%. Comparing base (ffc82b5) to head (8f9a484).

Additional details and impacted files
@@              Coverage Diff              @@
##             develop     #213      +/-   ##
=============================================
+ Coverage      44.12%   44.39%   +0.26%     
  Complexity      1393     1393              
=============================================
  Files            316      316              
  Lines           9760     9760              
  Branches        1495     1495              
=============================================
+ Hits            4307     4333      +26     
+ Misses          4891     4863      -28     
- Partials         562      564       +2     
Flag Coverage Δ
integration-tests 28.56% <ø> (+0.31%) ⬆️
unit-tests 26.03% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@spetrov spetrov merged commit 75aad51 into develop Jun 23, 2026
35 of 38 checks passed
@spetrov spetrov deleted the SDKS-5068 branch June 23, 2026 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants