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

feat: modularize toolchain tests #1102

Merged
merged 28 commits into from
Dec 18, 2024
Merged

feat: modularize toolchain tests #1102

merged 28 commits into from
Dec 18, 2024

Conversation

arayikhalatyan
Copy link
Contributor

@arayikhalatyan arayikhalatyan commented Dec 16, 2024

Resolves INT-2740

Moved the tests in ./crates/toolchain/tests into respective test crates inside the corresponding extensions

Copy link
Contributor

@jonathanpwang jonathanpwang left a comment

Choose a reason for hiding this comment

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

We should update the layout.md doc too

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

@jonathanpwang jonathanpwang left a comment

Choose a reason for hiding this comment

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

Overall refactor and modularization looks good. I had some inline comments but also we should:

  1. Add the cargo nextest to each of the respective github workflows (e.g., ecc.yml), right now the tests won't actually be run.
  2. Can we add for each of the examples linked from the book, the correct openvm.toml to the test programs dir?
  3. After (2), test locally that the cli works, then let's add tests to cli.yml where we run the cli from the test guest program dir exactly as one should do it from the book. This way CI will check that the book is up to date without us having to manually check.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

- run-id=${{ github.run_id }}
- family=m7
- runs-on=${{ github.run_id }}
- runner=32cpu-linux-arm64

steps:
- uses: actions/checkout@v4
- uses: dtolnay/rust-toolchain@stable
Copy link
Contributor

Choose a reason for hiding this comment

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

delete

Copy link
Contributor

@jonathanpwang jonathanpwang left a comment

Choose a reason for hiding this comment

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

LGTM except some minor CI cleanup

Copy link

Benchmarks

group app_log_blowup app_total_cells_used app_total_cycles app_total_proof_time_ms leaf_log_blowup leaf_total_cells_used leaf_total_cycles leaf_total_proof_time_ms max_segment_length instance alloc
ecrecover_program
2
10,251,804
195,066
(+152.0 [+8.1%])
2,029.0
-
-
-
-
1048476 64cpu-linux-arm64 mimalloc
fibonacci_program
2
51,615,800
3,000,274
(-9.0 [-0.2%])
5,519.0
-
-
-
-
1048476 64cpu-linux-arm64 mimalloc
regex_program
2
238,890,449
8,381,808
(-341.0 [-1.9%])
17,147.0
-
-
-
-
1048476 64cpu-linux-arm64 mimalloc
verify_fibair
2
48,127,147
397,164
(-22.0 [-0.7%])
3,153.0
-
-
-
-
1048476 64cpu-linux-arm64 mimalloc

Commit: 00ba22f

Benchmark Workflow

@arayikhalatyan arayikhalatyan merged commit 56caed2 into main Dec 18, 2024
20 checks passed
@arayikhalatyan arayikhalatyan deleted the feat/test_modular branch December 18, 2024 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants