refactor(test-suite): refactor negative ACL e2e tests into consistent pattern#2101
Conversation
🧪 CI InsightsHere's what we observed from your CI run for e427987. 🟢 All jobs passed!But CI Insights is watching 👀 |
|
@claude /pr-review |
|
Claude finished @eudelins-zama's request. — View run |
There was a problem hiding this comment.
[AUTOMATED]
Code Review
PR: refactor(test-suite): refactor negative ACL e2e tests into consistent pattern
Summary
This PR refactors negative (failure-path) ACL e2e tests across 4 test files into a consistent describe('negative', ...) pattern, adds a negative-acl test type to the CLI, and adds a corresponding CI workflow step. The changes are well-structured and maintain test correctness.
Review Results
| Agent | Focus | Findings |
|---|---|---|
| Bug Hunter (×2) | Bugs & security | ✅ No issues found |
| Guideline Compliance | Convention adherence | ✅ No violations found |
| Error Handling Auditor | Silent failures | ✅ No issues found |
Analysis Highlights
Grep pattern safety (-g "negative"): Verified that the Mocha grep pattern "negative" used by the new negative-acl test type will only match the four newly introduced describe('negative', ...) blocks. No existing test names in the e2e suite contain the substring "negative", so there is no risk of accidentally running unintended tests.
Test state preservation: Each extracted negative test properly sets up its own state or correctly relies on the shared before/beforeEach hooks from the parent describe block. Specifically:
EncryptedERC20.ts: The new standalone test correctly replicates the mint + transfer setup from the original positive testuserDecryption.ts: All four extracted tests re-obtain handles and generate fresh keypairs; shared contract state from thebeforehook is sufficientdelegatedUserDecryption.ts: Pure structural wrap with no logic changeshttpPublicDecrypt.ts: ThexBool→xUint8change is functionally equivalent for testing ACL rejection
Error handling in tests: The try/catch patterns used in negative tests are safe — expect.fail() throws AssertionError with messages that would never match the specific error strings being asserted in catch blocks.
No issues found. Checked for bugs, guideline compliance, and error handling.
|
The review has been posted. Here's a summary: Code Review Summary — PR #2101PR: refactor(test-suite): refactor negative ACL e2e tests into consistent pattern Verdict: ✅ ApprovedAll four specialized review agents (2× Bug Hunter, Guideline Compliance, Error Handling Auditor) found no issues meeting the high-confidence threshold (≥80/100). Key Points Verified
|
f1d61f2
dc2314f to
080ad58
Compare
|
@mergify queue |
Merge Queue Status
This pull request spent 1 hour 34 minutes 56 seconds in the queue, including 34 minutes 44 seconds running CI. Required conditions to merge
|
Closes https://github.com/zama-ai/fhevm-internal/issues/1152