test: add duplicate contract_id scenario to initialize.feature (#411)#425
Conversation
…afetrustcr#411) - Add initialize.feature with 4 scenarios: valid callback, missing contract_id, invalid escrow_type, duplicate contract_id - Add seed-test-escrows.sql fixture for trustless_work_escrows test data - Depends on safetrustcr#409 (handler) and safetrustcr#410 (seed fixture)
|
@shogun444 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughThis PR updates the Karate feature file for ChangesEscrow Initialize Duplicate Contract Test
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
![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
🤖 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 `@tests/karate/features/escrows/initialize.feature`:
- Line 71: The assertion in the escrow initialize feature is too broad and
includes fields that are not missing in this request. Update the Karate
expectation in initialize.feature so it checks only the missing `contract_id`
error from the response, keeping the validation aligned with the actual payload
and avoiding false failures or masking validator issues.
🪄 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: 77fe9d74-3d28-4d2a-b999-a04d26fa109b
📒 Files selected for processing (2)
tests/karate/features/escrows/initialize.featuretests/karate/fixtures/seed-test-escrows.sql
| """ | ||
| When method POST | ||
| Then status 400 | ||
| And match response.error == 'Missing required fields: contract_id, marker, approver, releaser, amount, escrow_type' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert only the missing field here.
This request omits contract_id only, so expecting an error that also lists marker, approver, releaser, amount, and escrow_type will either fail against a correct validator or bless a broken one.
Suggested fix
- And match response.error == 'Missing required fields: contract_id, marker, approver, releaser, amount, escrow_type'
+ And match response.error == 'Missing required fields: contract_id'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| And match response.error == 'Missing required fields: contract_id, marker, approver, releaser, amount, escrow_type' | |
| And match response.error == 'Missing required fields: contract_id' |
🤖 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 `@tests/karate/features/escrows/initialize.feature` at line 71, The assertion
in the escrow initialize feature is too broad and includes fields that are not
missing in this request. Update the Karate expectation in initialize.feature so
it checks only the missing `contract_id` error from the response, keeping the
validation aligned with the actual payload and avoiding false failures or
masking validator issues.
Resolve conflicts: keep main's initialize.feature, add deploy.feature for safetrustcr#411
shogun444
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@tests/karate/features/escrows/deploy.feature`:
- Around line 84-85: The deploy.feature scenario currently only checks for a
generic 500 from escrow persistence, so it can pass on unrelated Hasura
failures. Update the assertions in this scenario to also verify a specific value
in response.details that identifies the duplicate contract_id / UNIQUE
constraint path, using the existing deploy.feature response checks so the test
proves the duplicate-key contract instead of any persistence error.
- Around line 66-67: The validation assertion in the deploy escrow Karate
feature is too loose because it only checks a substring, so it can miss
regressions in the exact allowed-values list. Update the matching step in this
scenario to assert the full exact error string returned by the handler instead
of using contains, so the test is tied to the stable validation message for
escrow_type.
- Line 11: The escrow Karate scenarios are targeting the wrong API endpoint, so
they are not covering the implemented callback path. Update the request setup in
the feature that uses Given path under the escrow deploy scenarios to point to
/api/escrows/initialize instead of /api/escrows/deploy, and keep the surrounding
scenario names and assertions aligned with the initialize flow so the tests
exercise the actual route being added.
🪄 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: 083a5a6f-f910-4199-ac83-175da69737ce
📒 Files selected for processing (2)
tests/karate/features/escrows/deploy.featuretests/karate/fixtures/seed-test-escrows.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/karate/fixtures/seed-test-escrows.sql
| Then status 400 | ||
| And match response.error contains 'escrow_type must be one of' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the full validation message here.
The handler returns a stable exact string for this case, and the issue explicitly asks for the correct error message. contains would still pass if the allowed-value list regressed.
Suggested change
- And match response.error contains 'escrow_type must be one of'
+ And match response.error == 'escrow_type must be one of: single_release, multi_release'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Then status 400 | |
| And match response.error contains 'escrow_type must be one of' | |
| Then status 400 | |
| And match response.error == 'escrow_type must be one of: single_release, multi_release' |
🤖 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 `@tests/karate/features/escrows/deploy.feature` around lines 66 - 67, The
validation assertion in the deploy escrow Karate feature is too loose because it
only checks a substring, so it can miss regressions in the exact allowed-values
list. Update the matching step in this scenario to assert the full exact error
string returned by the handler instead of using contains, so the test is tied to
the stable validation message for escrow_type.
There was a problem hiding this comment.
@sotoJ24 It looks like your comment came through empty. Could you clarify your intent here?
If you'd like, I can help by:
- Confirming the exact allowed
escrow_typevalues so we can lock in the full assertion (e.g.,single_release, multi_release), or - Leaving the
containscheck as-is if you'd prefer looser coupling to the exact message.
Just let me know how you'd like to proceed.
| Then status 500 | ||
| And match response.error == 'Failed to persist escrow record' |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Prove this is the duplicate-key path, not just any persistence 500.
Right now a transient Hasura failure would also pass this scenario. Please assert something in response.details that ties the failure to the duplicate contract_id / UNIQUE constraint so the test covers the contract described in the issue.
🤖 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 `@tests/karate/features/escrows/deploy.feature` around lines 84 - 85, The
deploy.feature scenario currently only checks for a generic 500 from escrow
persistence, so it can pass on unrelated Hasura failures. Update the assertions
in this scenario to also verify a specific value in response.details that
identifies the duplicate contract_id / UNIQUE constraint path, using the
existing deploy.feature response checks so the test proves the duplicate-key
contract instead of any persistence error.
There was a problem hiding this comment.
@sotoJ24, it looks like your comment came through empty — could you clarify what you'd like to do here?
If you're planning to address the duplicate-contract_id assertion (tying the 500 response to the actual UNIQUE constraint violation via response.details, rather than any generic persistence failure), let me know once it's updated or if you'd like help drafting the assertion. Alternatively, if you feel the current response.error == 'Failed to persist escrow record' check is sufficient for this PR's scope, just say so and I'll leave it as-is.
(´・ω・`) ?
sotoJ24
left a comment
sotoJ24
left a comment
There was a problem hiding this comment.
Dear @shogun444, could you apply the following feedback, everything it's great and clean well done
| Then status 400 | ||
| And match response.error contains 'escrow_type must be one of' |
| Then status 500 | ||
| And match response.error == 'Failed to persist escrow record' |
Resolved seed-test-escrows.sql conflict by accepting main version. Restores files that were incorrectly removed from the PR branch.
- Remove deploy.feature (wrong endpoint, superseded by initialize.feature) - Extend initialize.feature Background with seed-test-escrows.sql - Add Duplicate contract_id scenario asserting UNIQUE constraint - Fixes per CodeRabbit review: - Use == exact match for escrow_type validation (already correct) - Assert response.details for UNIQUE constraint violation - Point scenarios at /api/escrows/initialize (already correct) Closes safetrustcr#411
shogun444
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/karate/features/escrows/initialize.feature (1)
81-83: 🧹 Nitpick | 🔵 TrivialConsider whether raw DB constraint errors should surface as 500.
The test documents existing behavior where a duplicate
contract_idbubbles up as a500with a raw Postgres constraint message, rather than a409 Conflictwith a sanitized message. Since this is test-only coverage and no app code changes here, just flagging for future hardening — leaking DB error internals in a 500 response is generally undesirable.🤖 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 `@tests/karate/features/escrows/initialize.feature` around lines 81 - 83, The escrow initialization flow is exposing a raw Postgres unique-constraint message through a 500 response; harden the error handling in the escrow persistence path so duplicate contract_id failures are translated to a 409 Conflict with a sanitized response.error instead of surfacing response.details[0].message. Update the API handler/service behind this feature so the duplicate-key case is caught and mapped consistently, and keep the feature test in initialize.feature aligned with the intended contract.
🤖 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 `@tests/karate/features/escrows/initialize.feature`:
- Around line 81-83: The escrow initialization flow is exposing a raw Postgres
unique-constraint message through a 500 response; harden the error handling in
the escrow persistence path so duplicate contract_id failures are translated to
a 409 Conflict with a sanitized response.error instead of surfacing
response.details[0].message. Update the API handler/service behind this feature
so the duplicate-key case is caught and mapped consistently, and keep the
feature test in initialize.feature aligned with the intended contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1b8eb7e-7ec2-46e1-b95b-0b84fff1eed6
📒 Files selected for processing (1)
tests/karate/features/escrows/initialize.feature
|
@sotoJ24 Thanks for the feedback! |
2 participants
Summary
This PR updates the existing
initialize.featureto add the missing duplicatecontract_idtest scenario instead of introducing a separatedeploy.feature.Since #409, #410, #426, #429, and #430 have already been merged, the existing
initialize.featureis now the canonical test suite for the escrow initialization flow. This change fills the remaining coverage gap for duplicate contract handling.Changes
tests/karate/features/escrows/deploy.featuresince it targeted the obsolete/api/escrows/deployendpoint.tests/karate/features/escrows/initialize.featureby:seed-test-escrows.sqlin the background for deterministic test data.contract_idreturns500scenario.response.details[0].messagecontains the expected PostgreSQL/Hasura UNIQUE constraint violation.Test Coverage
200– Escrow record persisted400escrow_type400contract_id500with UNIQUE constraint detailsVerification
main.contract_idhandling without introducing duplicate feature files.Closes #411