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

chore(ci): create sequencer integration test workflow #2508

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

Itay-Tsabary-Starkware
Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware commented Dec 8, 2024

commit-id:55e84a9b


Stack:

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link

codecov bot commented Dec 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 34.02%. Comparing base (e3165c4) to head (63c08f8).
Report is 776 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a 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()

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware changed the base branch from spr/main/9ebcfa97 to main December 8, 2024 11:19
Copy link

github-actions bot commented Dec 8, 2024

Artifacts upload workflows:

Copy link
Contributor Author

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a 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 :)
all std::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 🙏

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a 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, removed actions/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

Copy link
Contributor Author

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a 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 for pull_request: in our workflows, there should be at least one example for a workflow with only a pull_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.

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Itay-Tsabary-Starkware)

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a 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

Copy link
Contributor Author

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a 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.

Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Itay-Tsabary-Starkware)

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware merged commit 5952876 into main Dec 9, 2024
11 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants