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): pass test scenarios from the test body #2724

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yair-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@yair-starkware yair-starkware marked this pull request as ready for review December 17, 2024 12:30
Copy link

graphite-app bot commented Dec 17, 2024

Graphite Automations

"Yair - Auto-assign" took an action on this PR • (12/17/24)

1 assignee was added to this PR based on Yair's automation.

Copy link
Collaborator

@alonh5 alonh5 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, 3 unresolved discussions (waiting on @Itay-Tsabary-Starkware and @yair-starkware)


crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 70 at r1 (raw file):

    let test_tx_hashes_scenarios =
        [test_tx_hashes_for_integration_test, test_tx_hashes_for_integration_test];

Why do you need these?

Code quote:

    let create_rpc_txs_scenarios =
        [create_txs_for_integration_test, create_txs_for_integration_test];

    let test_tx_hashes_scenarios =
        [test_tx_hashes_for_integration_test, test_tx_hashes_for_integration_test];

crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 73 at r1 (raw file):

    // Build multiple heights to ensure heights are committed.
    for (height, expected_content_id, create_rpc_txs_fn, test_tx_hashes_fn) in itertools::izip!(

I think actually it is important to assert they are all the same length. Otherwise some of the test might be missing without us noticing


crates/starknet_integration_tests/src/utils.rs line 207 at r1 (raw file):

    create_rpc_txs_fn: impl Fn(&mut MultiAccountTransactionGenerator) -> Vec<RpcTransaction>,
    send_rpc_tx_fn: &'a mut dyn FnMut(RpcTransaction) -> Fut,
    test_tx_hashes_fn: impl Fn(&[TransactionHash]) -> Vec<TransactionHash>,

Should these two be coupled?

Code quote:

    create_rpc_txs_fn: impl Fn(&mut MultiAccountTransactionGenerator) -> Vec<RpcTransaction>,
    send_rpc_tx_fn: &'a mut dyn FnMut(RpcTransaction) -> Fut,
    test_tx_hashes_fn: impl Fn(&[TransactionHash]) -> Vec<TransactionHash>,

Copy link
Contributor Author

@yair-starkware yair-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 3 files reviewed, 3 unresolved discussions (waiting on @alonh5 and @Itay-Tsabary-Starkware)


crates/starknet_integration_tests/src/utils.rs line 207 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Should these two be coupled?

WDYM?


crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 70 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Why do you need these?

When adding deploy_account I need different test txs


crates/starknet_integration_tests/tests/end_to_end_flow_test.rs line 73 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

I think actually it is important to assert they are all the same length. Otherwise some of the test might be missing without us noticing

Done.

@yair-starkware yair-starkware force-pushed the yair/changing_test_scenarios branch from 471426d to 10faaa6 Compare December 19, 2024 10:50
Copy link
Collaborator

@alonh5 alonh5 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Itay-Tsabary-Starkware and @yair-starkware)


crates/starknet_integration_tests/src/utils.rs line 207 at r1 (raw file):

Previously, yair-starkware (Yair) wrote…

WDYM?

create_rpc_txs_fn and test_tx_hashes_fn.

@yair-starkware yair-starkware force-pushed the yair/changing_test_scenarios branch from 10faaa6 to 9eca0c3 Compare December 22, 2024 12:14
@yair-starkware
Copy link
Contributor Author

crates/starknet_integration_tests/src/utils.rs line 207 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

create_rpc_txs_fn and test_tx_hashes_fn.

Added a TODO because it is a bit tricky task

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.

3 participants