Tighten native consolidation contract gates after MRZ unification#1826
Tighten native consolidation contract gates after MRZ unification#1826transphorm merged 1 commit intodevfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe pull request restructures the PassportReader mock implementation by converting the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
transphorm
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
specs/projects/sdk/workstreams/native-consolidation/CONTRACTS.md (1)
27-27:⚠️ Potential issue | 🟠 MajorThe Android ordering guarantee is stronger than the test coverage.
In
app/tests/src/integrations/nfc/nfcScanner.test.ts, Lines 489-490 only verify thatreset()and Androidscan(...)were both called; they do not verify thatreset()happened first. Since this file says it only records facts pinned by tests, the "reset()thenscan(...)" wording is too strong unless you also add an ordering assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/projects/sdk/workstreams/native-consolidation/CONTRACTS.md` at line 27, The CONTRACTS.md statement "scan() must call reset() then call Android scan(...)" overstates the tests; either update the wording in CONTRACTS.md to say the tests only assert that reset() and Android scan(...) were both called (without ordering), or add an ordering assertion to the integration test app/tests/src/integrations/nfc/nfcScanner.test.ts (around the calls to reset() and Android scan(...)) to assert reset() occurred before scan(); locate the test that references reset() and scan() and either relax the CONTRACTS.md language or add a strict ordering check in the test so the contract matches the verified behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/tests/src/integrations/nfc/nfcScanner.test.ts`:
- Around line 426-429: The test assertion for isolatedScan currently uses
rejects.toMatchObject({ message: 'NFC scanning is currently unavailable. Please
ensure the app is properly installed.' }) which only checks for a message
property; update the assertion to use rejects.toThrow('NFC scanning is currently
unavailable. Please ensure the app is properly installed.') so the test verifies
the rejection is an Error instance with the exact message; apply the same change
for the similar assertion at the other occurrence (around the isolatedScan / NFC
failure tests) to enforce Error type + message semantics.
In `@app/tests/src/integrations/nfc/passportReader.test.ts`:
- Around line 31-32: Remove the brittle Function.length assertion for
PassportReader.scanPassport (it only validates mock decoration set in
jest.setup.js) and either delete the
expect(PassportReader.scanPassport.length).toBe(10) line or replace it with a
behavior-driven assertion that verifies parameters are forwarded (e.g., ensure
mockScanPassport is called with the full parameter list); refer to
PassportReader.scanPassport, mockScanPassport, and the existing
nfcScanner.test.ts test for the correct behavioral assertion pattern.
---
Outside diff comments:
In `@specs/projects/sdk/workstreams/native-consolidation/CONTRACTS.md`:
- Line 27: The CONTRACTS.md statement "scan() must call reset() then call
Android scan(...)" overstates the tests; either update the wording in
CONTRACTS.md to say the tests only assert that reset() and Android scan(...)
were both called (without ordering), or add an ordering assertion to the
integration test app/tests/src/integrations/nfc/nfcScanner.test.ts (around the
calls to reset() and Android scan(...)) to assert reset() occurred before
scan(); locate the test that references reset() and scan() and either relax the
CONTRACTS.md language or add a strict ordering check in the test so the contract
matches the verified behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0fa4a0e5-6dc3-438a-8e3d-7611094640bc
📒 Files selected for processing (4)
app/jest.setup.jsapp/tests/src/integrations/nfc/nfcScanner.test.tsapp/tests/src/integrations/nfc/passportReader.test.tsspecs/projects/sdk/workstreams/native-consolidation/CONTRACTS.md
| await expect(isolatedScan(mockInputs)).rejects.toMatchObject({ | ||
| message: | ||
| 'NFC scanning is currently unavailable. Please ensure the app is properly installed.', | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '79,85p' app/src/integrations/nfc/nfcScanner.ts
sed -n '121,128p' app/src/integrations/nfc/nfcScanner.ts
sed -n '412,448p' app/tests/src/integrations/nfc/nfcScanner.test.tsRepository: selfxyz/self
Length of output: 1993
🏁 Script executed:
#!/bin/bash
# Look for other rejects assertions in the test file to see established patterns
rg "rejects\." app/tests/src/integrations/nfc/nfcScanner.test.ts -A 2 | head -40Repository: selfxyz/self
Length of output: 523
🏁 Script executed:
#!/bin/bash
# Check if there are any existing Error type checks in the test file
rg "toBeInstanceOf|toThrow|toEqual.*Error" app/tests/src/integrations/nfc/nfcScanner.test.tsRepository: selfxyz/self
Length of output: 359
🏁 Script executed:
#!/bin/bash
# Also check the full context around those error paths in the source to confirm Error is used
rg "Promise\.reject\(new Error" app/src/integrations/nfc/nfcScanner.ts -B 2 -A 1Repository: selfxyz/self
Length of output: 186
Use rejects.toThrow() to validate Error type and message together.
The test currently uses rejects.toMatchObject({ message: ... }), which only checks if the rejection has a message property—allowing a plain object with that field to pass. Since app/src/integrations/nfc/nfcScanner.ts rejects with Promise.reject(new Error(...)), the test should enforce that contract. The test file already uses the correct pattern elsewhere: rejects.toThrow('native scan failed'). This matcher validates both that the rejection is an Error instance and that the message matches.
Corrected assertions
- await expect(isolatedScan(mockInputs)).rejects.toMatchObject({
- message:
- 'NFC scanning is currently unavailable. Please ensure the app is properly installed.',
- });
+ await expect(isolatedScan(mockInputs)).rejects.toThrow(
+ 'NFC scanning is currently unavailable. Please ensure the app is properly installed.'
+ );- await expect(isolatedScan(mockInputs)).rejects.toMatchObject({
- message: 'NFC scanning is currently unavailable.',
- });
+ await expect(isolatedScan(mockInputs)).rejects.toThrow(
+ 'NFC scanning is currently unavailable.'
+ );Per coding guidelines: "Test value over mock wiring—prefer tests that validate behavior; avoid tests that only assert mocks were called unless that is the behavior being validated."
Also applies to: 446-448
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/tests/src/integrations/nfc/nfcScanner.test.ts` around lines 426 - 429,
The test assertion for isolatedScan currently uses rejects.toMatchObject({
message: 'NFC scanning is currently unavailable. Please ensure the app is
properly installed.' }) which only checks for a message property; update the
assertion to use rejects.toThrow('NFC scanning is currently unavailable. Please
ensure the app is properly installed.') so the test verifies the rejection is an
Error instance with the exact message; apply the same change for the similar
assertion at the other occurrence (around the isolatedScan / NFC failure tests)
to enforce Error type + message semantics.
| // scanPassport should take exactly 10 parameters including sessionId | ||
| expect(PassportReader.scanPassport.length).toBe(10); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "scanPassport\\.length|Object\\.defineProperty\\(mockScanPassport, 'length'" \
app/tests/src/integrations/nfc/passportReader.test.ts \
app/jest.setup.jsRepository: selfxyz/self
Length of output: 351
🏁 Script executed:
sed -n '20,40p' app/tests/src/integrations/nfc/passportReader.test.tsRepository: selfxyz/self
Length of output: 844
🏁 Script executed:
rg -B 5 -A 5 "scanPassport\.length|scanPassport\(" app/tests/src/integrations/nfc/passportReader.test.ts | head -40Repository: selfxyz/self
Length of output: 870
🏁 Script executed:
rg -n "scanPassport.*sessionId|sessionId.*scanPassport" app/tests/src/integrations/nfc/ --context 5Repository: selfxyz/self
Length of output: 1113
🏁 Script executed:
grep -n "scan\(" app/tests/src/integrations/nfc/nfcScanner.test.ts | head -20Repository: selfxyz/self
Length of output: 79
🏁 Script executed:
rg "scanPassport.*\(" app/tests/src/integrations/nfc/nfcScanner.test.ts -A 8 | head -50Repository: selfxyz/self
Length of output: 857
🏁 Script executed:
rg "sessionId" app/tests/src/integrations/nfc/ -B 2 -A 2Repository: selfxyz/self
Length of output: 1639
🏁 Script executed:
rg "scanPassport\(" app/tests/src/integrations/nfc/nfcScanner.test.ts -B 3 -A 12 | head -80Repository: selfxyz/self
Length of output: 38
🏁 Script executed:
rg "PassportReader" app/tests/src/integrations/nfc/nfcScanner.test.ts -B 2 -A 8 | head -100Repository: selfxyz/self
Length of output: 2900
🏁 Script executed:
sed -n '1,50p' app/tests/src/integrations/nfc/nfcScanner.test.tsRepository: selfxyz/self
Length of output: 1718
🏁 Script executed:
sed -n '100,140p' app/tests/src/integrations/nfc/nfcScanner.test.tsRepository: selfxyz/self
Length of output: 1444
🏁 Script executed:
sed -n '200,260p' app/tests/src/integrations/nfc/nfcScanner.test.tsRepository: selfxyz/self
Length of output: 2029
🏁 Script executed:
rg "expect.*toHaveBeenCalledWith" app/tests/src/integrations/nfc/nfcScanner.test.ts -B 2 -A 15 | head -120Repository: selfxyz/self
Length of output: 2416
Remove or replace the Function.length assertion; it validates mock decoration, not behavior.
This test only verifies that jest.setup.js set Object.defineProperty(mockScanPassport, 'length', { value: 10 }) correctly—not actual function behavior. The real behavior validation already exists in nfcScanner.test.ts, which properly asserts all 10 parameters are forwarded (passportNumber, dateOfBirth, dateOfExpiry, canNumber, useCan, skipPACE, skipCA, extendedMode, usePacePolling, sessionId) via toHaveBeenCalledWith().
Per coding guidelines: "Test value over mock wiring—prefer tests that validate behavior; avoid tests that only assert mocks were called unless that is the behavior being validated."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/tests/src/integrations/nfc/passportReader.test.ts` around lines 31 - 32,
Remove the brittle Function.length assertion for PassportReader.scanPassport (it
only validates mock decoration set in jest.setup.js) and either delete the
expect(PassportReader.scanPassport.length).toBe(10) line or replace it with a
behavior-driven assertion that verifies parameters are forwarded (e.g., ensure
mockScanPassport is called with the full parameter list); refer to
PassportReader.scanPassport, mockScanPassport, and the existing
nfcScanner.test.ts test for the correct behavioral assertion pattern.
1 participant
This PR is a follow-up to #1823 and keeps scope limited to contract cleanup.
It corrects drift between the current PassportReader/NFC test baseline and the real app-facing interface, and trims CONTRACTS.md so it only documents verified, test-enforced facts. It also adds exact platform-specific assertions for NFC module-unavailable error text so both iOS and Android
behavior are pinned.
This PR does not change native MRZ code, does not start PassportReader consolidation, and does not alter public native module names.
Summary by CodeRabbit
Bug Fixes
Tests
Chores