test(SDKS-5126): improve integration tests across JS, Android, and iOS#56
Open
pingidentity-gaurav wants to merge 2 commits into
Open
test(SDKS-5126): improve integration tests across JS, Android, and iOS#56pingidentity-gaurav wants to merge 2 commits into
pingidentity-gaurav wants to merge 2 commits into
Conversation
…S layers - Fix 12 failing Jest integration tests: assertions now use .code instead of .error to match PingError subclass shape - Add missing coverage config (collectCoverageFrom, coverageDirectory, coverageReporters) to 5 jest.config.js files so CI lcov artifacts are generated - Flip continue-on-error to false on js-unit-tests job so failures are no longer silently swallowed - Extend native-spec-contracts.test.ts with 3 missing packages and 2 fido methods for compile-time drift detection - Add global mocks for rn-device-client and rn-push in jest.setup.js - Add encode/decode round-trip tests in push.test.ts and external-idp.test.ts using jest.requireActual - Add Android Robolectric field-name assertions for binding getAllKeys (5 tests) and oath encodeCredential/generateCodeWithValidity (9 tests) - Extract serializeUserKey helper in binding iOS and encodeCredential/encodeCodeInfo helpers in oath iOS; add XCTest field-name assertions for both (20 tests total) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughUpdates bridge serialization helpers and contract tests for binding and oath modules, expands native-spec compile-time checks and Jest mocks, normalizes integration rejection assertions to mapped error codes, and adds Jest coverage configuration plus workflow gating changes. ChangesBridge contracts and integration test alignment
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
PingTestRunner/__tests__/integration/push.test.ts (1)
192-230: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract the shared
react-nativemock to avoid drift.
loadPushWithRealHelpersduplicates the entirereact-nativemock block (Lines 201-219) already defined inloadPush(Lines 142-160). The two loaders now differ only in theNativeRNPingPushfactory. Extracting the commonjest.doMock('react-native', ...)setup into a small helper keeps the two paths in sync if the mocked RN surface changes.🤖 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 `@PingTestRunner/__tests__/integration/push.test.ts` around lines 192 - 230, The `loadPushWithRealHelpers` function duplicates the identical `jest.doMock('react-native', ...)` mock setup that already exists in the `loadPush` function. Extract this common mock configuration into a separate helper function and call it from both `loadPush` and `loadPushWithRealHelpers` to eliminate duplication and prevent the mocks from drifting if the React Native surface being mocked changes in the future.
🤖 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
`@packages/oath/android/src/test/java/com/pingidentity/rnoath/RNPingOathCommonTest.kt`:
- Around line 751-766: The test method
encodeCredential_doesNotSerializeSecretField currently only asserts that the
"secret" field is not serialized to the bridge. Add an additional assertion
immediately after the existing assertFalse call for "secret" to also verify that
the "secretKey" field is not present in the map, using the same assertFalse
pattern with an appropriate error message indicating that secretKey must not be
serialized to the bridge.
In `@PingTestRunner/__tests__/integration/native-spec-contracts.test.ts`:
- Around line 157-183: The comment on the _PushMockedMethods type declaration
states "all 22 bridge methods," but the actual Pick<PushSpec> type currently
includes only 21 methods. Count the methods listed in the _PushMockedMethods
Pick type and update the comment to reflect the actual number of methods being
mocked instead of 22.
---
Nitpick comments:
In `@PingTestRunner/__tests__/integration/push.test.ts`:
- Around line 192-230: The `loadPushWithRealHelpers` function duplicates the
identical `jest.doMock('react-native', ...)` mock setup that already exists in
the `loadPush` function. Extract this common mock configuration into a separate
helper function and call it from both `loadPush` and `loadPushWithRealHelpers`
to eliminate duplication and prevent the mocks from drifting if the React Native
surface being mocked changes in the future.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7dbd235e-2aaa-4e61-af2e-6d5b46a3b041
📒 Files selected for processing (19)
.github/workflows/js-unit-tests.ymlPingTestRunner/__tests__/integration/device-client.test.tsPingTestRunner/__tests__/integration/external-idp.test.tsPingTestRunner/__tests__/integration/native-spec-contracts.test.tsPingTestRunner/__tests__/integration/oath.test.tsPingTestRunner/__tests__/integration/push.test.tsPingTestRunner/jest.setup.jspackages/binding/android/build.gradlepackages/binding/android/src/test/java/com/pingidentity/rnbinding/RNPingBindingTest.ktpackages/binding/ios/RNPingBindingCommon.swiftpackages/binding/ios/Tests/RNPingBindingCommonTests.swiftpackages/binding/jest.config.jspackages/device-client/jest.config.jspackages/fido/jest.config.jspackages/oath/android/src/test/java/com/pingidentity/rnoath/RNPingOathCommonTest.ktpackages/oath/ios/RNPingOathCommon.swiftpackages/oath/ios/Tests/RNPingOathCommonTests.swiftpackages/push/jest.config.jspackages/types/jest.config.js
Contributor
|
- Fix comment drift: "22 bridge methods" -> "21" in native-spec-contracts.test.ts - Also assert secretKey is absent from oath credential bridge payload (alongside secret) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
1 participant
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.
Summary
{ type, error }) to matchPingErrorsubclass shape ({ type, code })coverageDirectory/collectCoverageFrom/coverageReportersto 5 jest configs (binding,device-client,fido,push,types) so CI lcov artifacts are generatedcontinue-on-error: falseonjs-unit-testsjob - failures no longer silently pass CInative-spec-contracts.test.tswith 3 missing packages (rn-device-client,rn-external-idp,rn-push) and 2 missing fido methods for compile-time method-name drift detectionrn-device-clientandrn-pushinjest.setup.jspush.test.tsandexternal-idp.test.tsusingjest.requireActualso realfromNative*helpers run against controlled native mocksbindinggetAllKeys (5 tests),oathencodeCredential + generateCodeWithValidity (9 tests)serializeUserKeyin binding iOS andencodeCredential/encodeCodeInfohelpers in oath iOS; add XCTest field-name assertions for both (20 tests) - verified withxcodebuild build-for-testingSummary by CodeRabbit
Bug Fixes
Tests
type/codepayloads.Chores