Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use test config in CI #770

Merged
merged 9 commits into from
Nov 6, 2024
Merged

Use test config in CI #770

merged 9 commits into from
Nov 6, 2024

Conversation

sai-deng
Copy link
Contributor

@sai-deng sai-deng commented Nov 4, 2024

The two_to_one_block test takes approximately 30 seconds to complete on my MacBook in release mode. However, it remains slow in the CI environment because it runs in debug mode by default. To address this, I have added a new CI job that specifically runs longer tests, including two_to_one_block, in release mode. This ensures better performance and faster feedback for these resource-intensive tests.

Before:
Continuous Integration / Test evm_arithmetization (pull_request) Successful in 17m
Jerigon Integration / Native tracer proof generation (pull_request) Successful in 14m
Jerigon Integration / Zero tracer proof generation (pull_request) Successful in 14m

After:
Continuous Integration / Slow evm_arithmetization tests in release mode (pull_request) Successful in 5m (new)
Continuous Integration / Test evm_arithmetization (pull_request) Successful in 8m
Jerigon Integration / Native tracer proof generation (pull_request) Successful in 9m
Jerigon Integration / Zero tracer proof generation (pull_request) Successful in 9m

@github-actions github-actions bot added the crate: evm_arithmetization Anything related to the evm_arithmetization crate. label Nov 5, 2024
@sai-deng sai-deng changed the title [wip] use test config in ci Use test config in CI Nov 5, 2024
@github-actions github-actions bot added the ci label Nov 5, 2024

- name: Run specific ignored tests in release mode
run: |
RUST_LOG=release cargo test --release \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you intend to have RUST_LOG=release? I would expect it to take error,warn,info,debug,trace

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it as it has no effect.

@@ -52,6 +52,7 @@ type C = KeccakGoldilocksConfig;
/// `1337` from address `0x5B38Da6a701c568545dCfcB03FcB875f56beddC4` to address
/// `0xAb8483F64d9C6d1EcF9b849Ae677dD3315835cb2`.
#[test]
#[ignore]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is erc721 taking that long that we're ignoring it on default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I found that the test running on CI is about 4x slower than on my MacBook. The test time in release mode on my MacBook can be found here: #739

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to start doing test selection like this, I think we need a better test runner (cargo-nextest)

It allows you to categorise your tests, have different timeouts, profiles for ci etc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, we only need minimal functionality, so our existing setup works fine. Using cargo-nextest would indeed be beneficial, and we can look at converting to it in a future PR.

@@ -97,6 +97,7 @@ if [[ $8 == "test_only" ]]; then
echo "Proving blocks from ($START_BLOCK) to ($END_BLOCK)"
command="cargo r --release --package zero --bin leader -- \
--test-only \
--use-test-config \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this step is test-only, i.e. no proving but only witness generation so --use-test-config has no effect

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -134,6 +135,7 @@ else
echo "Proving blocks from ($START_BLOCK) to ($END_BLOCK)"
command="cargo r --release --package zero --bin leader -- \
--runtime in-memory \
--use-test-config \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure we want the script to always use the testing config, rather take it as optional argument perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we always use the testing config in CI tests, while our cron jobs use the standard config?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we had said in the dev sync we'd use a mix of both in CI? I'm fine either way though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Keep Execute bash script to generate and verify a proof for a small block. with default config.

scripts/prove_stdio.sh Outdated Show resolved Hide resolved
@sai-deng sai-deng self-assigned this Nov 5, 2024
@sai-deng sai-deng marked this pull request as ready for review November 5, 2024 22:57
@@ -95,6 +95,26 @@ jobs:
CARGO_INCREMENTAL: 1
RUST_BACKTRACE: 1

run_ignored_tests:
name: Run Specific Ignored Tests in Release Mode
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'd just make the job name a tad more explicit, something like this?

Suggested change
name: Run Specific Ignored Tests in Release Mode
name: Slow evm_arithmetization tests in release mode

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

@LindaGuiga LindaGuiga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@sai-deng sai-deng merged commit 70f6116 into develop Nov 6, 2024
24 checks passed
@sai-deng sai-deng deleted the sai/ci_with_test_config branch November 6, 2024 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci crate: evm_arithmetization Anything related to the evm_arithmetization crate.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants