fix: add some stability improvements to crisp_e2e test#936
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdded the Changes
Sequence Diagram(s)sequenceDiagram
participant DevScript as dev.sh
participant Services as dev_services.sh
participant Server as CRISP server
participant WaitOn as wait-on (tcp)
participant Client as client (pnpm dev-static)
note right of DevScript #DDEEFF: Start orchestration
DevScript->>Services: run dev_services.sh (deploy + services)
Services->>Server: start server
Services->>WaitOn: wait-on tcp:13151
WaitOn-->>Services: port ready
Services->>Client: start client via pnpm dev-static
Client->>Client: Vite reads NO_HOT -> HMR off (logs)
note right of Client #EEFFEE: Dev environment running
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/program-server/src/types.rs (1)
11-15: Consider applying the same Debug customization to ComputeResponse.The
ComputeResponsestruct containsciphertextandprooffields similar toWebhookPayload, but it still uses the standardDebugderive. For consistency and to prevent the same resource starvation issues mentioned in the PR objectives, consider applying the sameDerivativepattern here.Apply this diff to add Derivative support:
-#[derive(Serialize, Deserialize, Debug)] +#[derive(Derivative, Serialize, Deserialize)] +#[derivative(Debug)] pub struct ComputeResponse { + #[derivative(Debug = "ignore")] pub ciphertext: Vec<u8>, + #[derivative(Debug = "ignore")] pub proof: Vec<u8>, }
🧹 Nitpick comments (1)
crates/program-server/src/types.rs (1)
142-154: Consider adding a test for the custom Debug behavior.While
test_webhook_payload_serializationvalidates serialization, there's no test verifying thatciphertextandproofare excluded from Debug output. This would help ensure the intended behavior is preserved.Add a test like this after the existing serialization test:
#[test] fn test_webhook_payload_debug_excludes_sensitive_fields() { let payload = WebhookPayload { e3_id: 12345, ciphertext: vec![0x01, 0x23, 0x45, 0x67, 0x89, 0xab, 0xcd, 0xef], proof: vec![0xde, 0xad, 0xbe, 0xef], }; let debug_output = format!("{:?}", payload); // Should include e3_id assert!(debug_output.contains("12345")); // Should NOT include ciphertext or proof hex values assert!(!debug_output.contains("01")); assert!(!debug_output.contains("de")); assert!(!debug_output.contains("ad")); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockexamples/CRISP/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
crates/program-server/Cargo.toml(1 hunks)crates/program-server/src/types.rs(2 hunks)examples/CRISP/Cargo.toml(1 hunks)examples/CRISP/client/package.json(1 hunks)examples/CRISP/client/vite.config.ts(2 hunks)examples/CRISP/scripts/dev_client.sh(1 hunks)examples/CRISP/server/Cargo.toml(1 hunks)examples/CRISP/server/src/server/models.rs(1 hunks)scripts/run-crisp-test.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-25T10:28:56.174Z
Learnt from: ctrlc03
PR: gnosisguild/enclave#657
File: Cargo.toml:32-34
Timestamp: 2025-08-25T10:28:56.174Z
Learning: The examples/CRISP directory has its own Cargo.toml workspace configuration with members like "server", "wasm-crypto", "program/core", "program/client", etc. The root workspace intentionally excludes "examples/CRISP/server", "examples/CRISP/program", and "examples/CRISP/wasm-crypto" to prevent double workspace membership, which is the correct approach for self-contained example workspaces.
Applied to files:
examples/CRISP/server/Cargo.tomlcrates/program-server/Cargo.tomlexamples/CRISP/Cargo.toml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build_sdk
- GitHub Check: integration_prebuild
- GitHub Check: test_net
- GitHub Check: build_enclave_cli
- GitHub Check: build_e3_support_risc0
- GitHub Check: rust_unit
- GitHub Check: rust_integration
- GitHub Check: test_contracts
- GitHub Check: Build & Push Image
- GitHub Check: Build & Push Image
🔇 Additional comments (9)
examples/CRISP/client/package.json (1)
14-14: LGTM! New static development script added.The
dev-staticscript correctly setsNO_HOT=1to disable hot module reloading, which aligns with the PR objective of reducing flakiness in the e2e test environment.examples/CRISP/client/vite.config.ts (1)
16-16: LGTM! Appropriate logging for NO_HOT mode.The log message correctly indicates when HMR is disabled, which helps with debugging the development environment configuration.
examples/CRISP/scripts/dev_client.sh (1)
7-7: LGTM! Correctly uses the new dev-static script.This change aligns with the new
dev-staticscript added topackage.json, which disables HMR for improved test stability.examples/CRISP/server/Cargo.toml (1)
56-56: LGTM! Correctly adds derivative as workspace dependency.This aligns with the workspace configuration in
examples/CRISP/Cargo.tomlwhich definesderivative = "=2.2.0"in the workspace dependencies.examples/CRISP/Cargo.toml (1)
31-31: LGTM! Workspace dependency added correctly.The
derivativecrate is properly added to workspace dependencies with a pinned version=2.2.0, enabling workspace members to use it viaderivative.workspace = true.examples/CRISP/server/src/server/models.rs (2)
8-8: LGTM! Derivative import added for custom Debug implementation.This import enables the custom Debug behavior that excludes sensitive fields from debug output.
11-21: Excellent! Custom Debug implementation prevents printing large data.The use of
Derivativewith#[derivative(Debug = "ignore")]onciphertextandprooffields effectively prevents these large byte arrays from being printed to output, which aligns perfectly with the PR objective of avoiding resource starvation during proof processing.This maintains serialization behavior while customizing debug output.
crates/program-server/Cargo.toml (1)
18-18: No action required—derivative is properly defined in workspace.The verification confirms that
derivative = "=2.2.0"is defined in the rootCargo.tomlat line 118 within[workspace.dependencies]. The referencederivative.workspace = trueincrates/program-server/Cargo.tomlis correct and will resolve properly.crates/program-server/src/types.rs (1)
27-36: LGTM! Debug customization prevents resource starvation.The
Derivativeimplementation correctly excludesciphertextandprooffields from Debug output while preserving serialization behavior and keepinge3_idvisible. This aligns well with the PR objectives to avoid resource starvation during proof processing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
examples/CRISP/test/crisp.spec.ts (2)
51-51: Approve wait, but consider condition-based alternatives.The 1-second wait after
runCliInitimproves test stability by allowing CLI effects to propagate. However, Playwright's condition-based waits (e.g.,waitForLoadState,waitForSelector) are more robust as they complete immediately when the condition is met rather than always waiting the full duration.Consider refactoring to wait for a specific condition if possible:
await runCliInit(); -await page.waitForTimeout(1000); await page.goto("/"); +await page.waitForLoadState('networkidle');
56-56: Approve wait, but consider waiting for MetaMask dialog.The 1-second wait after clicking MetaMask improves test stability by allowing the MetaMask extension to respond. However, waiting for a specific condition would be more reliable.
Consider waiting for the MetaMask dialog to be ready:
await page.locator('button:has-text("MetaMask")').click(); -await page.waitForTimeout(1000); +await metamaskPage.waitForLoadState('domcontentloaded'); await metamask.connectToDapp();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/CRISP/test/crisp.spec.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ctrlc03
PR: gnosisguild/enclave#657
File: Cargo.toml:32-34
Timestamp: 2025-08-25T10:28:56.174Z
Learning: The examples/CRISP directory has its own Cargo.toml workspace configuration with members like "server", "wasm-crypto", "program/core", "program/client", etc. The root workspace intentionally excludes "examples/CRISP/server", "examples/CRISP/program", and "examples/CRISP/wasm-crypto" to prevent double workspace membership, which is the correct approach for self-contained example workspaces.
Learnt from: ryardley
PR: gnosisguild/enclave#107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
Learnt from: ryardley
PR: gnosisguild/enclave#107
File: packages/ciphernode/sortition/src/distance.rs:1-1
Timestamp: 2024-09-26T03:11:29.311Z
Learning: In `packages/ciphernode/core/src/events.rs`, the import statements use the correct and updated `alloy::primitives` module.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build_sdk
- GitHub Check: integration_prebuild
- GitHub Check: test_contracts
- GitHub Check: build_enclave_cli
- GitHub Check: test_net
- GitHub Check: rust_unit
- GitHub Check: rust_integration
- GitHub Check: Build & Push Image
🔇 Additional comments (2)
examples/CRISP/test/crisp.spec.ts (2)
18-18: LGTM! Trailing commas improve consistency.These trailing comma additions follow JavaScript/TypeScript best practices and will make future diffs cleaner.
Also applies to: 33-33, 47-47
67-71: LGTM! Improved test selectors.The updated selectors use
data-test-idattributes, which is a best practice for E2E tests as they're more stable and less coupled to styling changes than CSS class selectors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
examples/CRISP/scripts/dev_server.sh (1)
7-7: Consider whether building the CLI here is appropriate.The script now builds both the
serverandclibinaries before running the server. While building the server before running makes sense, building the CLI in thedev_server.shscript seems outside its scope since this script only runs the server.If the CLI needs to be available for tests, consider:
- Building it in a separate script or test setup step
- Documenting why the CLI build is included here
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/CRISP/playwright.config.ts(1 hunks)examples/CRISP/scripts/dev_server.sh(1 hunks)examples/CRISP/scripts/dev_services.sh(1 hunks)examples/CRISP/test/crisp.spec.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ctrlc03
PR: gnosisguild/enclave#657
File: Cargo.toml:32-34
Timestamp: 2025-08-25T10:28:56.174Z
Learning: The examples/CRISP directory has its own Cargo.toml workspace configuration with members like "server", "wasm-crypto", "program/core", "program/client", etc. The root workspace intentionally excludes "examples/CRISP/server", "examples/CRISP/program", and "examples/CRISP/wasm-crypto" to prevent double workspace membership, which is the correct approach for self-contained example workspaces.
📚 Learning: 2025-08-25T10:28:56.174Z
Learnt from: ctrlc03
PR: gnosisguild/enclave#657
File: Cargo.toml:32-34
Timestamp: 2025-08-25T10:28:56.174Z
Learning: The examples/CRISP directory has its own Cargo.toml workspace configuration with members like "server", "wasm-crypto", "program/core", "program/client", etc. The root workspace intentionally excludes "examples/CRISP/server", "examples/CRISP/program", and "examples/CRISP/wasm-crypto" to prevent double workspace membership, which is the correct approach for self-contained example workspaces.
Applied to files:
examples/CRISP/scripts/dev_server.sh
📚 Learning: 2024-10-08T19:45:18.209Z
Learnt from: ryardley
PR: gnosisguild/enclave#107
File: tests/basic_integration/test.sh:81-83
Timestamp: 2024-10-08T19:45:18.209Z
Learning: In `tests/basic_integration/test.sh`, it's acceptable to wait indefinitely for the EVM node to start without a timeout, as it's unlikely to fail here.
Applied to files:
examples/CRISP/scripts/dev_services.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build_enclave_cli
- GitHub Check: build_e3_support_dev
- GitHub Check: build_sdk
- GitHub Check: test_net
- GitHub Check: integration_prebuild
- GitHub Check: test_contracts
- GitHub Check: rust_integration
- GitHub Check: rust_unit
- GitHub Check: Build & Push Image
- GitHub Check: Build & Push Image
🔇 Additional comments (6)
examples/CRISP/scripts/dev_services.sh (1)
8-8: Verify port 13151 is correct andwait-onbehavior is as expected.Using
wait-on tcp:13151is a solid improvement over hardcoded delays for startup synchronization. However, there are a couple of details to verify:
- Port correctness: Confirm that port 13151 is indeed the port on which
dev_server.shlistens (it should be listening before or immediately upon startup).wait-ondefaults: Verify thatwait-onhas reasonable timeout defaults. Per the learnings, indefinite waits can be acceptable for local dev setup, but ensure this doesn't create confusing hangs during troubleshooting.- Port already bound: If another process has bound 13151,
wait-onwill pass immediately, butdev_server.shmay still fail. Consider whether the script should fail fast in this scenario or if the concurrently-kflag suffices.To verify these assumptions, you may check:
- The
dev_server.shscript to confirm it listens on 13151- The
wait-onCLI defaults by runningwait-on --helpor checking its documentation- Whether there are any integration tests that validate this startup sequence
The change aligns well with the PR's stability objectives and is an improvement over the previous approach.
examples/CRISP/playwright.config.ts (2)
28-30: Good addition for test stability.Adding
slowMo: 100is a reasonable approach to reduce test flakiness by introducing a 100ms delay between browser actions, giving the application more time to settle between interactions.
11-11: Verify the timeout calculation.The timeout is set to
5 * 60 * 10000, which equals 3,000,000 ms (50 minutes). This seems extremely long. Was this intended to be5 * 60 * 1000(5 minutes) instead?While the test does include long-running operations (220 second wait on line 64 in crisp.spec.ts for proof processing), a 50-minute timeout still seems excessive.
examples/CRISP/test/crisp.spec.ts (3)
14-25: Good error handling addition.Wrapping the
execSynccall in a try-catch block improves error handling and debugging by logging command errors before re-throwing them. This helps diagnose CLI initialization failures during test runs.
51-51: Hard-coded waits added for stability.These 1000ms waits help stabilize the test by allowing time for CLI initialization and MetaMask interaction to settle. While hard-coded waits are generally less ideal than waiting for specific conditions, they can be necessary for handling timing issues in complex e2e scenarios.
Also applies to: 56-56
57-57: Excellent use of condition-based waiting.Using
waitForLoadState("networkidle")is a best practice for e2e tests, as it waits for network activity to settle rather than using arbitrary timeouts. This ensures the page is fully loaded before proceeding with MetaMask connection.
|
Ok this is good to go |
hmzakhalid
left a comment
hmzakhalid
left a comment
There was a problem hiding this comment.
utACK
Thanks a lot for this. I had a quick review of the changes. Seems good to merge.
2 participants
I had a few issues running crisp on my local system this also helped fix CI which was also having issues with running in headless mode. This PR helps with fixing these issues.
pnpm dev-staticscript that does not do hot module reloading../scripts/run-crisp-test.sh --uiand./scripts/run-crisp-test.shxvf-runwhere I finally worked out that not providing a screen size was causing the test to stall both locally on CI.Summary by CodeRabbit
New Features
Improvements
Chores