chore: refactor tests and simplify ci#1423
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis PR consolidates duplicated test helper functions across Rust and TypeScript test suites into centralized modules, and restructures CI workflows by splitting rust_tests into unit and integration variants, adding per-job timeout budgets, and enhancing setup steps with caching and artifact management. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
📝 Coding Plan
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.
🧹 Nitpick comments (1)
packages/enclave-contracts/test/fixtures/operators.ts (1)
48-48: RedundantaddCiphernodecall — safe but unnecessary.The
registerOperator()call on line 40 already invokesregistry.addCiphernode(operatorAddress)internally (viaBondingRegistry.registerOperatorat line 309 of the contract). The explicit call here is redundant sinceCiphernodeRegistryOwnable.addCiphernodehas an early-return guard when the node is already enabled.This doesn't cause issues, but removing it would clarify the setup sequence.
♻️ Suggested simplification
await bondingRegistry.connect(operator).addTicketBalance(ticketAmount); - - await registry.addCiphernode(operatorAddress); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enclave-contracts/test/fixtures/operators.ts` at line 48, Remove the redundant explicit call to registry.addCiphernode(operatorAddress) in the fixture setup: registerOperator() already calls BondingRegistry.registerOperator which in turn invokes CiphernodeRegistryOwnable.addCiphernode, so simply delete the await registry.addCiphernode(operatorAddress); line and rely on registerOperator() to perform the registration to keep the setup sequence clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/enclave-contracts/test/fixtures/operators.ts`:
- Line 48: Remove the redundant explicit call to
registry.addCiphernode(operatorAddress) in the fixture setup: registerOperator()
already calls BondingRegistry.registerOperator which in turn invokes
CiphernodeRegistryOwnable.addCiphernode, so simply delete the await
registry.addCiphernode(operatorAddress); line and rely on registerOperator() to
perform the registration to keep the setup sequence clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7f1c7e80-9fa7-4504-b304-db9a30c947f9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.github/workflows/ci.ymlcrates/test-helpers/src/lib.rscrates/tests/tests/integration.rscrates/zk-prover/Cargo.tomlcrates/zk-prover/tests/common/helpers.rspackages/enclave-contracts/artifacts/contracts/verifier/DkgPkVerifier.sol/DkgPkVerifier.jsonpackages/enclave-contracts/artifacts/contracts/verifier/DkgPkVerifier.sol/ZKTranscriptLib.jsonpackages/enclave-contracts/package.jsonpackages/enclave-contracts/test/E3Lifecycle/E3Integration.spec.tspackages/enclave-contracts/test/Enclave.spec.tspackages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.tspackages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.tspackages/enclave-contracts/test/Slashing/SlashingManager.spec.tspackages/enclave-contracts/test/fixtures/attestation.tspackages/enclave-contracts/test/fixtures/index.tspackages/enclave-contracts/test/fixtures/operators.ts
ctrlc03
force-pushed
the
chore/test-refactoring
branch
from
0f3392c to
bfb17c2
Compare
Contributor
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)
.github/workflows/ci.yml (1)
107-118:⚠️ Potential issue | 🟠 MajorDon't let SDK/runtime changes bypass the template E2E job.
template_integrationconsumes freshly built SDK/support-script/CLI artifacts, buttemplatesis now derived from$TEMPLATES/$CIonly andbuild_enclave_clistill omits$SDK. A PR that only changes the SDK, contracts, circuits, or Rust runtime can now skip the one job that exercisestemplates/defaultagainst those new artifacts.Suggested filter adjustment
- echo "templates=$(any $FORCE $TEMPLATES $CI)" >> $GITHUB_OUTPUT + echo "templates=$(any $FORCE $TEMPLATES $SDK $RUST $CONTRACTS $CIRCUITS $CI)" >> $GITHUB_OUTPUT ... - echo "build_enclave_cli=$(any $FORCE $RUST $CONTRACTS $CIRCUITS $INTEGRATION $TEMPLATES $CRISP $CI)" >> $GITHUB_OUTPUT + echo "build_enclave_cli=$(any $FORCE $RUST $CONTRACTS $CIRCUITS $INTEGRATION $TEMPLATES $SDK $CRISP $CI)" >> $GITHUB_OUTPUT🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 107 - 118, The templates output currently only depends on $TEMPLATES and $CI so SDK/runtime changes can skip template E2E; update the echo that sets "templates=" to include the SDK and runtime-related flags (e.g., add $SDK $RUST $CONTRACTS $CIRCUITS, and $INTEGRATION if relevant) so it reads something like any $FORCE $TEMPLATES $SDK $RUST $CONTRACTS $CIRCUITS $CI, and also add $SDK to the "build_enclave_cli=" any(...) invocation so build_enclave_cli includes SDK artifacts; modify the lines that set the templates and build_enclave_cli outputs accordingly.
🧹 Nitpick comments (2)
crates/zk-prover/tests/common/helpers.rs (1)
50-56: Consider adding error context tounwrap()calls for easier debugging.Multiple
.unwrap()calls throughoutsetup_compiled_circuitwill panic without context if any fs operation fails. For test helpers,expect()with a brief message improves failure diagnostics:fs::create_dir_all(&evm_dir).await.expect("failed to create evm circuit dir");This is optional given the test-helper context, but helpful when debugging CI failures. Based on learnings: test utilities can use simpler error handling, so this is not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/zk-prover/tests/common/helpers.rs` around lines 50 - 56, The unwrap() calls in setup_compiled_circuit (specifically on fs::create_dir_all and the two fs::copy calls) provide no failure context; replace them with expect() (or use .await.expect(...)) and add short descriptive messages like "failed to create evm circuit dir", "failed to copy circuit json", and "failed to copy vk file" so test failures surface actionable diagnostics when those async fs operations fail.crates/test-helpers/src/lib.rs (1)
73-90: Consider returningOption<PathBuf>for API consistency.
find_bb()returnsOption<PathBuf>whilefind_anvil()returnsbool. If callers ever need the resolved anvil path (e.g., to spawn a custom instance), they'd need a separate lookup. ReturningOption<PathBuf>would unify the API and allow callers to use.is_some()for existence checks.That said, if current usage only requires a boolean check, this is fine as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/test-helpers/src/lib.rs` around lines 73 - 90, Change find_anvil() to return Option<PathBuf> instead of bool: attempt to resolve the anvil executable path from the "which" command (use output.stdout trimmed and converted to PathBuf) and return Some(path) if it exists and is executable; if that fails, check the HOME fallback path "{HOME}/.foundry/bin/anvil" and return Some(PathBuf) when found; otherwise return None. Update the function signature and callers accordingly, preserving async tokio::process::Command use and referencing the find_anvil function and the HOME fallback logic in the body.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 1020-1023: The workflow's timeout-minutes is set to 30 which is
shorter than the integration test budget
(templates/default/tests/integration.spec.ts runs pnpm test:integration and can
wait three sequential 700s which alone is ~35 minutes plus setup/compile), so
update the timeout-minutes in the CI job (change the timeout-minutes entry in
.github/workflows/ci.yml for the job containing needs/if/runs-on) to a value >=
60 (or at least >35, recommended 60) so legitimate slow-but-valid runs are not
killed.
- Around line 749-766: The cache step identified by id "cache-bb" is writing to
/usr/local/bin/bb which causes permission errors on restore; change the cache
path to a user-writable location like ~/.cache/barretenberg/${{ env.BB_VERSION
}} and update the cache key to include env.BB_VERSION, update the "Install
Barretenberg (bb)" logic to extract the bb binary into that cache directory
(create it if missing) instead of /usr/local/bin, and ensure the job exports or
prepends that directory to PATH (e.g., export
PATH="${HOME}/.cache/barretenberg/${{ env.BB_VERSION }}:$PATH") before the
"Verify bb" step so bb --version can run; apply the same changes to both job
instances that contain the cache-bb/Install/Verify sequence.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 107-118: The templates output currently only depends on $TEMPLATES
and $CI so SDK/runtime changes can skip template E2E; update the echo that sets
"templates=" to include the SDK and runtime-related flags (e.g., add $SDK $RUST
$CONTRACTS $CIRCUITS, and $INTEGRATION if relevant) so it reads something like
any $FORCE $TEMPLATES $SDK $RUST $CONTRACTS $CIRCUITS $CI, and also add $SDK to
the "build_enclave_cli=" any(...) invocation so build_enclave_cli includes SDK
artifacts; modify the lines that set the templates and build_enclave_cli outputs
accordingly.
---
Nitpick comments:
In `@crates/test-helpers/src/lib.rs`:
- Around line 73-90: Change find_anvil() to return Option<PathBuf> instead of
bool: attempt to resolve the anvil executable path from the "which" command (use
output.stdout trimmed and converted to PathBuf) and return Some(path) if it
exists and is executable; if that fails, check the HOME fallback path
"{HOME}/.foundry/bin/anvil" and return Some(PathBuf) when found; otherwise
return None. Update the function signature and callers accordingly, preserving
async tokio::process::Command use and referencing the find_anvil function and
the HOME fallback logic in the body.
In `@crates/zk-prover/tests/common/helpers.rs`:
- Around line 50-56: The unwrap() calls in setup_compiled_circuit (specifically
on fs::create_dir_all and the two fs::copy calls) provide no failure context;
replace them with expect() (or use .await.expect(...)) and add short descriptive
messages like "failed to create evm circuit dir", "failed to copy circuit json",
and "failed to copy vk file" so test failures surface actionable diagnostics
when those async fs operations fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 04a486f8-4041-4f6d-9b9c-4e57f4dd44a3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.github/workflows/ci.ymlcrates/test-helpers/src/lib.rscrates/tests/tests/integration.rscrates/zk-prover/Cargo.tomlcrates/zk-prover/tests/common/helpers.rspackages/enclave-contracts/package.jsonpackages/enclave-contracts/test/E3Lifecycle/E3Integration.spec.tspackages/enclave-contracts/test/Enclave.spec.tspackages/enclave-contracts/test/Registry/CiphernodeRegistryOwnable.spec.tspackages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.tspackages/enclave-contracts/test/Slashing/SlashingManager.spec.tspackages/enclave-contracts/test/fixtures/attestation.tspackages/enclave-contracts/test/fixtures/index.tspackages/enclave-contracts/test/fixtures/operators.tstemplates/default/tests/integration.spec.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/zk-prover/Cargo.toml
- packages/enclave-contracts/package.json
- packages/enclave-contracts/test/fixtures/operators.ts
- packages/enclave-contracts/test/E3Lifecycle/E3Integration.spec.ts
- crates/tests/tests/integration.rs
- packages/enclave-contracts/test/fixtures/index.ts
- packages/enclave-contracts/test/fixtures/attestation.ts
- packages/enclave-contracts/test/Enclave.spec.ts
- packages/enclave-contracts/test/Slashing/CommitteeExpulsion.spec.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
1113-1116: Inconsistent pnpm version pinning.This job pins pnpm to
10.7.1, but other jobs (e.g.,rust_unit_tests,rust_integration_tests) usepnpm/action-setup@v4without a version pin. Consider aligning version handling across jobs—either pin everywhere or let the action resolve the version frompackageManagerinpackage.json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 1113 - 1116, The workflow contains inconsistent pnpm version handling: the "Install pnpm" step uses pnpm/action-setup@v4 with version: 10.7.1 while other jobs (e.g., rust_unit_tests, rust_integration_tests) call pnpm/action-setup@v4 without a pinned version; pick one approach and make all jobs consistent—either remove the explicit version: 10.7.1 from the "Install pnpm" step so every pnpm/action-setup@v4 invocation will resolve the pnpm version from package.json (packageManager), or add version: 10.7.1 to the pnpm/action-setup@v4 steps in rust_unit_tests and rust_integration_tests to pin them all to the same version; update the "Install pnpm" step or the rust_* job steps accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 1113-1116: The workflow contains inconsistent pnpm version
handling: the "Install pnpm" step uses pnpm/action-setup@v4 with version: 10.7.1
while other jobs (e.g., rust_unit_tests, rust_integration_tests) call
pnpm/action-setup@v4 without a pinned version; pick one approach and make all
jobs consistent—either remove the explicit version: 10.7.1 from the "Install
pnpm" step so every pnpm/action-setup@v4 invocation will resolve the pnpm
version from package.json (packageManager), or add version: 10.7.1 to the
pnpm/action-setup@v4 steps in rust_unit_tests and rust_integration_tests to pin
them all to the same version; update the "Install pnpm" step or the rust_* job
steps accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 684ef42a-2e20-4020-9517-a2bfbcd550fb
📒 Files selected for processing (1)
.github/workflows/ci.yml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/ci.yml (2)
1036-1036:⚠️ Potential issue | 🟠 Major
template_integrationtimeout is likely too low for valid slow runs.Line 1036 sets 30 minutes, but this job includes compile + integration waits and can exceed that on slower runners.
Timeout bump
- timeout-minutes: 30 + timeout-minutes: 60🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml at line 1036, The CI job timeout for template_integration is set too low (timeout-minutes: 30) and can expire during compile + integration waits; update the workflow to raise the timeout-minutes for the template_integration job (or the job-level/default timeout) to a higher value (e.g., 60 or 90) so slow runners can finish, by modifying the timeout-minutes field for the template_integration job in .github/workflows/ci.yml.
757-774:⚠️ Potential issue | 🟠 Major
bbcache path still targets root-owned location and can fail on restore.Line 761 and Line 831 cache
/usr/local/bin/bb.actions/cacherestores as the runner user, so cache hits can fail or produce unusable binaries.Safer cache/install pattern (apply in both job blocks)
- name: Cache Barretenberg binary id: cache-bb uses: actions/cache@v4 with: - path: /usr/local/bin/bb - key: bb-bin-${{ env.BB_VERSION }}-amd64-linux + path: ~/.cache/barretenberg/${{ env.BB_VERSION }} + key: bb-bin-${{ env.BB_VERSION }}-${{ runner.arch }}-linux + - name: Add bb to PATH + run: echo "$HOME/.cache/barretenberg/${{ env.BB_VERSION }}" >> "$GITHUB_PATH" + - name: Install Barretenberg (bb) if: steps.cache-bb.outputs.cache-hit != 'true' run: | curl -fsSL "https://github.com/AztecProtocol/aztec-packages/releases/download/v${{ env.BB_VERSION }}/barretenberg-amd64-linux.tar.gz" -o bb.tar.gz mkdir -p bb_extract && tar -xzf bb.tar.gz -C bb_extract BB_BIN=$(find bb_extract -name bb -type f | head -n 1) - sudo mv "$BB_BIN" /usr/local/bin/bb - chmod +x /usr/local/bin/bb + install -Dm755 "$BB_BIN" "$HOME/.cache/barretenberg/${{ env.BB_VERSION }}/bb"Also applies to: 827-850
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 757 - 774, The cache step (id cache-bb) is targeting the root-owned path /usr/local/bin/bb which the runner cannot safely restore; change the cache path to a runner-writable location (e.g. $RUNNER_TOOL_CACHE/bb or $HOME/.cache/bb) and update the cache key accordingly, then in the Install Barretenberg step extract the tar into that writable cache dir (find BB binary there) and only use sudo to move or install the binary into /usr/local/bin/bb after a successful cache miss/installation; ensure the Verify bb step still calls bb --version and apply the same pattern to the other job block mentioned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Line 107: The templates trigger is too narrow: update the line that sets the
outputs (the "templates=$(any $FORCE $TEMPLATES $CI)" echo) so it also considers
backend/contract/runtime changes that can break templates (e.g., include
additional CI flags like $RUST, $CONTRACTS, $SDK or replace with a git-diff
check). Modify the echo that defines templates to include those extra variables
(or a boolean computed from a git diff of Rust/contracts/SDK areas) so the
template_integration job runs when underlying code changes, not just when
template files or $CI are set.
---
Duplicate comments:
In @.github/workflows/ci.yml:
- Line 1036: The CI job timeout for template_integration is set too low
(timeout-minutes: 30) and can expire during compile + integration waits; update
the workflow to raise the timeout-minutes for the template_integration job (or
the job-level/default timeout) to a higher value (e.g., 60 or 90) so slow
runners can finish, by modifying the timeout-minutes field for the
template_integration job in .github/workflows/ci.yml.
- Around line 757-774: The cache step (id cache-bb) is targeting the root-owned
path /usr/local/bin/bb which the runner cannot safely restore; change the cache
path to a runner-writable location (e.g. $RUNNER_TOOL_CACHE/bb or
$HOME/.cache/bb) and update the cache key accordingly, then in the Install
Barretenberg step extract the tar into that writable cache dir (find BB binary
there) and only use sudo to move or install the binary into /usr/local/bin/bb
after a successful cache miss/installation; ensure the Verify bb step still
calls bb --version and apply the same pattern to the other job block mentioned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 39d8de27-94df-4071-b8a6-ab463b3a9be9
📒 Files selected for processing (2)
.github/workflows/ci.ymlpackages/enclave-contracts/test/E3Lifecycle/E3Integration.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/enclave-contracts/test/E3Lifecycle/E3Integration.spec.ts
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
2 participants
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 by CodeRabbit
Release Notes
e3:enabletoenclave:enableE3.