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

test: add p2p transaction sync happy flow test #2154

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

noamsp-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

noamsp-starkware commented Nov 18, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

@noamsp-starkware noamsp-starkware force-pushed the noam.s/p2p_class_sync_test_0 branch 2 times, most recently from f1b4ed5 to 612c480 Compare November 21, 2024 10:49
@noamsp-starkware noamsp-starkware force-pushed the noam.s/p2p_transaction_test_1 branch from 4aa3a3d to 467a96a Compare November 21, 2024 13:34
@noamsp-starkware noamsp-starkware changed the base branch from noam.s/p2p_class_sync_test_0 to main November 21, 2024 13:35
Copy link

Artifacts upload triggered. View details here

@noamsp-starkware noamsp-starkware marked this pull request as ready for review November 21, 2024 13:37
@noamsp-starkware noamsp-starkware force-pushed the noam.s/p2p_transaction_test_1 branch from 467a96a to c7ca838 Compare November 21, 2024 13:42
Copy link

codecov bot commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 23.66%. Comparing base (e3165c4) to head (4731dbd).
Report is 593 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #2154       +/-   ##
===========================================
- Coverage   40.10%   23.66%   -16.44%     
===========================================
  Files          26      105       +79     
  Lines        1895    12642    +10747     
  Branches     1895    12642    +10747     
===========================================
+ Hits          760     2992     +2232     
- Misses       1100     9098     +7998     
- Partials       35      552      +517     

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

Copy link
Contributor

@ShahakShama ShahakShama 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, 4 unresolved discussions (waiting on @noamsp-starkware)


crates/papyrus_test_utils/src/lib.rs line 1130 at r1 (raw file):

}

// TODO: Match memeber types for ExecutionResources and protobuf::receipt::ExecutionResources.

Match makes people think of rust's match. Rephrase to "fix mismatch of ..."


crates/papyrus_test_utils/src/lib.rs line 1130 at r1 (raw file):

}

// TODO: Match memeber types for ExecutionResources and protobuf::receipt::ExecutionResources.

for -> between


crates/papyrus_p2p_sync/src/client/transaction_test.rs line 90 at r1 (raw file):

        .await;

        // Simulate time has passed so that state diff sync will resend query after it waited for

Change the comment to match transactions


crates/papyrus_p2p_sync/src/client/transaction_test.rs line 97 at r1 (raw file):

        let num_transaction_queries = HEADER_QUERY_LENGTH.div_ceil(TRANSACTION_QUERY_LENGTH);
        for transaction_querie in 0..num_transaction_queries {

Change this index name into start_block_number and add a step to the iteration

Copy link
Contributor Author

@noamsp-starkware noamsp-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: all files reviewed, 4 unresolved discussions (waiting on @ShahakShama)


crates/papyrus_p2p_sync/src/client/transaction_test.rs line 90 at r1 (raw file):

Previously, ShahakShama wrote…

Change the comment to match transactions

Ack


crates/papyrus_p2p_sync/src/client/transaction_test.rs line 97 at r1 (raw file):

Previously, ShahakShama wrote…

Change this index name into start_block_number and add a step to the iteration

What do you mean by adding a step to the iteration?


crates/papyrus_test_utils/src/lib.rs line 1130 at r1 (raw file):

Previously, ShahakShama wrote…

Match makes people think of rust's match. Rephrase to "fix mismatch of ..."

Ack


crates/papyrus_test_utils/src/lib.rs line 1130 at r1 (raw file):

Previously, ShahakShama wrote…

for -> between

Ack

Copy link
Contributor

@ShahakShama ShahakShama 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: all files reviewed, 4 unresolved discussions (waiting on @noamsp-starkware)


crates/papyrus_p2p_sync/src/client/transaction_test.rs line 97 at r1 (raw file):

Previously, noamsp-starkware wrote…

What do you mean by adding a step to the iteration?

https://stackoverflow.com/questions/27893223/how-do-i-iterate-over-a-range-with-a-custom-step

Copy link
Contributor Author

@noamsp-starkware noamsp-starkware left a comment

Choose a reason for hiding this comment

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

@noamsp-starkware noamsp-starkware force-pushed the noam.s/p2p_transaction_test_1 branch from c7ca838 to 4731dbd Compare November 26, 2024 12:04
Copy link
Contributor

@ShahakShama ShahakShama 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 r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @noamsp-starkware)

@noamsp-starkware noamsp-starkware merged commit 13693b0 into main Nov 26, 2024
17 checks passed
@noamsp-starkware noamsp-starkware deleted the noam.s/p2p_transaction_test_1 branch November 26, 2024 17:05
@github-actions github-actions bot locked and limited conversation to collaborators Nov 29, 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