-
Notifications
You must be signed in to change notification settings - Fork 27
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
chore(ci): create sequencer integration test workflow #2508
Conversation
3dc561a
to
377ae68
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2508 +/- ##
==========================================
- Coverage 40.10% 34.02% -6.08%
==========================================
Files 26 276 +250
Lines 1895 32078 +30183
Branches 1895 32078 +30183
==========================================
+ Hits 760 10916 +10156
- Misses 1100 20169 +19069
- Partials 35 993 +958 ☔ View full report in Codecov by Sentry. |
377ae68
to
3113a2f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Itay-Tsabary-Starkware)
crates/starknet_integration_tests/tests/end_to_end_integration_test.rs
line 68 at r1 (raw file):
if !run_integration_test() { return; }
general question:
why not use a feature? instead of an env var?
cargo test --test end_to_end_integration_test --feature run_integration_test
non-blocking
Suggestion:
#[cfg(feature = "run_integration_test")]
async fn test_end_to_end_integration(mut tx_generator: MultiAccountTransactionGenerator) {
.github/workflows/sequencer_integration_tests.yml
line 9 at r1 (raw file):
- main-v[0-9].** tags: - v[0-9].**
this should also run on post-merge? why?
Code quote:
push:
branches:
- main
- main-v[0-9].**
tags:
- v[0-9].**
.github/workflows/sequencer_integration_tests.yml
line 33 at r1 (raw file):
steps: - uses: actions/checkout@v4 - run: cargo build --bin starknet_sequencer_node
what about the bootstrapping? cargo and rust installation, etc?
what version of rust is used here?
Code quote:
build-sequencer-node:
runs-on: starkware-ubuntu-latest-large
steps:
- uses: actions/checkout@v4
- run: cargo build --bin starknet_sequencer_node
.github/workflows/sequencer_integration_tests.yml
line 36 at r1 (raw file):
run-sequencer-integration-tests: needs: build-sequencer-node
why not merge this job with the build-sequencer-node job, and run the test as just another step?
different jobs can (in theory) run concurrently, any other reason to split into two jobs?
Code quote:
needs: build-sequencer-node
.github/workflows/sequencer_integration_tests.yml
line 42 at r1 (raw file):
with: # Fetch the entire history. fetch-depth: 0
why?
Code quote:
with:
# Fetch the entire history.
fetch-depth: 0
crates/starknet_integration_tests/src/utils.rs
line 232 at r1 (raw file):
pub fn run_integration_test() -> bool { std::env::var("SEQUENCER_INTEGRATION_TESTS").is_ok()
use a util from infra utils please :)
all std::env::var
calls should go through there, yes?
Code quote:
std::env::var("SEQUENCER_INTEGRATION_TESTS").is_ok()
3113a2f
to
b905ddd
Compare
Artifacts upload workflows: |
b905ddd
to
cc069c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 3 files reviewed, 5 unresolved discussions (waiting on @dorimedini-starkware)
.github/workflows/sequencer_integration_tests.yml
line 9 at r1 (raw file):
Previously, dorimedini-starkware wrote…
this should also run on post-merge? why?
I copied this part from main.yml
. What should I change here?
.github/workflows/sequencer_integration_tests.yml
line 33 at r1 (raw file):
Previously, dorimedini-starkware wrote…
what about the bootstrapping? cargo and rust installation, etc?
what version of rust is used here?
Added - uses: ./.github/actions/bootstrap
, removed actions/checkout@v4
.github/workflows/sequencer_integration_tests.yml
line 36 at r1 (raw file):
Previously, dorimedini-starkware wrote…
why not merge this job with the build-sequencer-node job, and run the test as just another step?
different jobs can (in theory) run concurrently, any other reason to split into two jobs?
Yes. We intend to have multiple integration tests, all relying on the build stage. I'd like these integration tests to run concurrently.
.github/workflows/sequencer_integration_tests.yml
line 42 at r1 (raw file):
Previously, dorimedini-starkware wrote…
why?
Removed, thanks.
crates/starknet_integration_tests/src/utils.rs
line 232 at r1 (raw file):
Previously, dorimedini-starkware wrote…
use a util from infra utils please :)
allstd::env::var
calls should go through there, yes?
:-)
This is intentional, infra_utils
shouldn't care about "SEQUENCER_INTEGRATION_TESTS"
. My point regarding consolidating env var usage was for envs that the producation code relies on. This is a mere test util that is solely relevant to this crate and its CI.
Should we further discuss the modus operandi in such tests at the weekly sync? There is another instance (which is where I got the inspiration from), please see https://github.com/starkware-libs/sequencer/blob/main/crates/papyrus_base_layer/src/base_layer_test.rs#L11-L13
crates/starknet_integration_tests/tests/end_to_end_integration_test.rs
line 68 at r1 (raw file):
Previously, dorimedini-starkware wrote…
general question:
why not use a feature? instead of an env var?cargo test --test end_to_end_integration_test --feature run_integration_test
non-blocking
Will consider this offline, thanks for the suggestion 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Itay-Tsabary-Starkware)
.github/workflows/sequencer_integration_tests.yml
line 9 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
I copied this part from
main.yml
. What should I change here?
if you don't want this to run in post-merge, delete the entire push:
entry.
search for pull_request:
in our workflows, there should be at least one example for a workflow with only a pull_request
event.
.github/workflows/sequencer_integration_tests.yml
line 33 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
Added
- uses: ./.github/actions/bootstrap
, removedactions/checkout@v4
why remove the checkout? isn't it required? example
crates/starknet_integration_tests/src/utils.rs
line 232 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
:-)
This is intentional,
infra_utils
shouldn't care about"SEQUENCER_INTEGRATION_TESTS"
. My point regarding consolidating env var usage was for envs that the producation code relies on. This is a mere test util that is solely relevant to this crate and its CI.Should we further discuss the modus operandi in such tests at the weekly sync? There is another instance (which is where I got the inspiration from), please see https://github.com/starkware-libs/sequencer/blob/main/crates/papyrus_base_layer/src/base_layer_test.rs#L11-L13
ah, right... nvm
.github/workflows/sequencer_integration_tests.yml
line 39 at r2 (raw file):
runs-on: starkware-ubuntu-latest-large steps: - uses: ./.github/actions/bootstrap
if you already bootstrapped in the build-sequencer-node job, I don't think you need it again here
Code quote:
- uses: ./.github/actions/bootstrap
cc069c3
to
dcbbf7b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware)
.github/workflows/sequencer_integration_tests.yml
line 9 at r1 (raw file):
Previously, dorimedini-starkware wrote…
if you don't want this to run in post-merge, delete the entire
push:
entry.
search forpull_request:
in our workflows, there should be at least one example for a workflow with only apull_request
event.
Ack, done.
.github/workflows/sequencer_integration_tests.yml
line 33 at r1 (raw file):
Previously, dorimedini-starkware wrote…
why remove the checkout? isn't it required? example
Right, I thought it did something else. Got the following error without it, restoring.
Error: Can't find 'action.yml', 'action.yaml' or 'Dockerfile' under '/home/runner/work/sequencer/sequencer/.github/actions/bootstrap'. Did you forget to run actions/checkout before running your local action?
Run cargo build --bin starknet\_sequencer\_node
0s
Complete job
0s
.github/workflows/sequencer_integration_tests.yml
line 39 at r2 (raw file):
Previously, dorimedini-starkware wrote…
if you already bootstrapped in the build-sequencer-node job, I don't think you need it again here
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Itay-Tsabary-Starkware)
dcbbf7b
to
5e67c52
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Itay-Tsabary-Starkware)
.github/workflows/sequencer_integration_tests.yml
line 33 at r4 (raw file):
runs-on: starkware-ubuntu-latest-large steps: - uses: actions/checkout@v4
really? why?
I would expect the checkout in build-sequencer-node to be enough...
if not, isn't the bootstrap phase needed as well?
Code quote:
- uses: actions/checkout@v4
commit-id:55e84a9b
5e67c52
to
63c08f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)
.github/workflows/sequencer_integration_tests.yml
line 33 at r4 (raw file):
Previously, dorimedini-starkware wrote…
really? why?
I would expect the checkout in build-sequencer-node to be enough...
if not, isn't the bootstrap phase needed as well?
Apparently jobs do not share state. Please see DM, and the new unified revision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Itay-Tsabary-Starkware)
commit-id:55e84a9b
Stack: