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(starknet_integration_tests): end to end integration test running as executable (not test) #2536

Closed

Conversation

lev-starkware
Copy link
Contributor

@lev-starkware lev-starkware commented Dec 8, 2024

No description provided.

… as executable (not test)

commit-id:cd232eab
@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.49%. Comparing base (e3165c4) to head (04563d4).
Report is 809 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2536      +/-   ##
==========================================
- Coverage   40.10%   34.49%   -5.62%     
==========================================
  Files          26      276     +250     
  Lines        1895    32075   +30180     
  Branches     1895    32075   +30180     
==========================================
+ Hits          760    11063   +10303     
- Misses       1100    20004   +18904     
- Partials       35     1008     +973     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@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: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @lev-starkware)


crates/starknet_integration_tests/src/bin/e2e_integration_test.rs line 70 at r1 (raw file):

        interval.tick().await;
    }
}

Please move these to a suitable test utils module -- PR#1

Code quote:

/// Reads the latest block number from the storage.
fn get_latest_block_number(storage_reader: &StorageReader) -> BlockNumber {
    let txn = storage_reader.begin_ro_txn().unwrap();
    txn.get_state_marker()
        .expect("There should always be a state marker")
        .prev()
        .expect("There should be a previous block in the storage, set by the test setup")
}

/// Reads an account nonce after a block number from storage.
fn get_account_nonce(storage_reader: &StorageReader, contract_address: ContractAddress) -> Nonce {
    let block_number = get_latest_block_number(storage_reader);
    let txn = storage_reader.begin_ro_txn().unwrap();
    let state_number = StateNumber::unchecked_right_after_block(block_number);
    get_nonce_at(&txn, state_number, None, contract_address)
        .expect("Should always be Ok(Some(Nonce))")
        .expect("Should always be Some(Nonce)")
}

/// Sample a storage until sufficiently many blocks have been stored. Returns an error if after
/// the given number of attempts the target block number has not been reached.
async fn await_block(
    interval_duration: Duration,
    target_block_number: BlockNumber,
    max_attempts: usize,
    storage_reader: &StorageReader,
) -> Result<(), ()> {
    let mut interval = interval(interval_duration);
    let mut count = 0;
    loop {
        // Read the latest block number.
        let latest_block_number = get_latest_block_number(storage_reader);
        count += 1;

        // Check if reached the target block number.
        if latest_block_number >= target_block_number {
            info!("Found block {} after {} queries.", target_block_number, count);
            return Ok(());
        }

        // Check if reached the maximum attempts.
        if count > max_attempts {
            error!(
                "Latest block is {}, expected {}, stopping sampling.",
                latest_block_number, target_block_number
            );
            return Err(());
        }

        // Wait for the next interval.
        interval.tick().await;
    }
}

crates/starknet_integration_tests/src/bin/e2e_integration_test.rs line 131 at r1 (raw file):

    let nonce = get_account_nonce(&batcher_storage_reader, sender_address);
    assert_eq!(nonce, expected_nonce);
}

In PR#2:

  1. move the test content here (i.e., it should be deleted from the test)
  2. in the test, build and run the binary using the shell command fn

Code quote:

#[tokio::main]
async fn main() {
    const EXPECTED_BLOCK_NUMBER: BlockNumber = BlockNumber(15);

    configure_tracing();
    info!("Running integration test setup.");

    // Creating the storage for the test.
    let mut tx_generator = create_integration_test_tx_generator();
    let integration_test_setup = IntegrationTestSetup::new_from_tx_generator(&tx_generator).await;

    info!("Running sequencer node.");
    let node_run_handle = spawn_run_node(integration_test_setup.node_config_path).await;

    // Wait for the node to start.
    match integration_test_setup.is_alive_test_client.await_alive(Duration::from_secs(5), 50).await
    {
        Ok(_) => {}
        Err(_) => panic!("Node is not alive."),
    }

    info!("Running integration test simulator.");

    let send_rpc_tx_fn =
        &mut |rpc_tx| integration_test_setup.add_tx_http_client.assert_add_tx_success(rpc_tx);

    const ACCOUNT_ID_0: AccountId = 0;
    let n_txs = 50;
    let sender_address = tx_generator.account_with_id(ACCOUNT_ID_0).sender_address();
    info!("Sending {n_txs} txs.");
    let tx_hashes = send_account_txs(tx_generator, ACCOUNT_ID_0, n_txs, send_rpc_tx_fn).await;

    info!("Awaiting until {EXPECTED_BLOCK_NUMBER} blocks have been created.");

    let (batcher_storage_reader, _) =
        papyrus_storage::open_storage(integration_test_setup.batcher_storage_config)
            .expect("Failed to open batcher's storage");

    match await_block(Duration::from_secs(5), EXPECTED_BLOCK_NUMBER, 15, &batcher_storage_reader)
        .await
    {
        Ok(_) => {}
        Err(_) => panic!("Did not reach expected block number."),
    }

    info!("Shutting the node down.");
    node_run_handle.abort();
    let res = node_run_handle.await;
    assert!(
        res.expect_err("Node should have been stopped.").is_cancelled(),
        "Node should have been stopped."
    );

    info!("Verifying tx sender account nonce.");
    let expected_nonce_value = tx_hashes.len() + 1;
    let expected_nonce =
        Nonce(Felt::from_hex_unchecked(format!("0x{:X}", expected_nonce_value).as_str()));
    let nonce = get_account_nonce(&batcher_storage_reader, sender_address);
    assert_eq!(nonce, expected_nonce);
}

Copy link
Contributor Author

@lev-starkware lev-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: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @Itay-Tsabary-Starkware)


crates/starknet_integration_tests/src/bin/e2e_integration_test.rs line 70 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please move these to a suitable test utils module -- PR#1

This comment is for refactoring. Let's finish this PR, and then I will make the refactoring.


crates/starknet_integration_tests/src/bin/e2e_integration_test.rs line 131 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

In PR#2:

  1. move the test content here (i.e., it should be deleted from the test)
  2. in the test, build and run the binary using the shell command fn

This comment is for refactoring. Let's finish this PR, and then I will make the refactoring.

Copy link
Contributor

@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: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @lev-starkware)


crates/starknet_integration_tests/src/bin/e2e_integration_test.rs line 131 at r1 (raw file):

Previously, lev-starkware wrote…

This comment is for refactoring. Let's finish this PR, and then I will make the refactoring.

The pr as it is cannot be accepted -- it adds an untested binary that might, and might not, do its intended behavior.
It also duplicates quite a substantial chunk of code.
Please avoid these issues.

Copy link
Contributor

@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: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @lev-starkware)


crates/starknet_integration_tests/src/bin/e2e_integration_test.rs line 131 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

The pr as it is cannot be accepted -- it adds an untested binary that might, and might not, do its intended behavior.
It also duplicates quite a substantial chunk of code.
Please avoid these issues.

The result of this pr, or a series of prs, should be an integration test that runs the above code as binaries.

Copy link
Contributor Author

@lev-starkware lev-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: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @Itay-Tsabary-Starkware)


crates/starknet_integration_tests/src/bin/e2e_integration_test.rs line 131 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

The result of this pr, or a series of prs, should be an integration test that runs the above code as binaries.

This code is the duplicate with really small, like a couple of lines, changes from the end_to_end_integration_test. So, it is as reliable as the end_to_end_integration_test.
And in any case, it is a test, it doesn't matter that we are running it as a binary and not as a cargo test. When we started to write test for test?
For the duplicate code: yes we need to remove the file end_to_end_integration_test.rs from the tests folder. Do you want me to add it to this PR?

Copy link
Contributor

@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: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @lev-starkware)


crates/starknet_integration_tests/src/bin/e2e_integration_test.rs line 131 at r1 (raw file):

Previously, lev-starkware wrote…

This code is the duplicate with really small, like a couple of lines, changes from the end_to_end_integration_test. So, it is as reliable as the end_to_end_integration_test.
And in any case, it is a test, it doesn't matter that we are running it as a binary and not as a cargo test. When we started to write test for test?
For the duplicate code: yes we need to remove the file end_to_end_integration_test.rs from the tests folder. Do you want me to add it to this PR?

Please refactor the pr according to the previous discussion, or in any other way you see fit that avoids code duplication.
As a guideline, functionality that is introduced into this module should either be:

  1. imported from another module
  2. exported to another module

Specifically, when the entire functionality of the current test_end_to_end_integration is within main here, the test_end_to_end_integration test should directly invoke main and assert its success.
This both achieves the task of having the test functionality in the binary, and that it is invoked by the ci.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 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