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 class sync fail cases tests #1943

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

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 11, 2024

Copy link

codecov bot commented Nov 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 26.56%. Comparing base (e3165c4) to head (9439cf3).
Report is 606 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1943       +/-   ##
===========================================
- Coverage   40.10%   26.56%   -13.55%     
===========================================
  Files          26      105       +79     
  Lines        1895    12634    +10739     
  Branches     1895    12634    +10739     
===========================================
+ Hits          760     3356     +2596     
- Misses       1100     8690     +7590     
- Partials       35      588      +553     

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

@noamsp-starkware noamsp-starkware marked this pull request as ready for review November 11, 2024 13:00
@noamsp-starkware noamsp-starkware force-pushed the noam.s/p2p_class_sync_test_1 branch from 82219ff to e902b6e Compare November 18, 2024 15:01
@noamsp-starkware noamsp-starkware force-pushed the noam.s/p2p_class_sync_test_2 branch from 96ea17d to b71daee Compare November 18, 2024 15:01
@noamsp-starkware noamsp-starkware force-pushed the noam.s/p2p_class_sync_test_1 branch from e902b6e to aeed8cc Compare November 18, 2024 15:28
@noamsp-starkware noamsp-starkware force-pushed the noam.s/p2p_class_sync_test_2 branch from b71daee to 3f8115e Compare November 18, 2024 15:28
@noamsp-starkware noamsp-starkware force-pushed the noam.s/p2p_class_sync_test_1 branch 2 times, most recently from 405ad78 to ea85b72 Compare November 21, 2024 13:41
@noamsp-starkware noamsp-starkware force-pushed the noam.s/p2p_class_sync_test_2 branch from 3f8115e to f165321 Compare November 21, 2024 13:41
@noamsp-starkware noamsp-starkware force-pushed the noam.s/p2p_class_sync_test_1 branch from ea85b72 to b63c651 Compare November 26, 2024 13:40
@noamsp-starkware noamsp-starkware force-pushed the noam.s/p2p_class_sync_test_2 branch from f165321 to 01a090f Compare November 26, 2024 13:45
@noamsp-starkware noamsp-starkware force-pushed the noam.s/p2p_class_sync_test_1 branch from b63c651 to 83532ab Compare November 26, 2024 16:52
@noamsp-starkware noamsp-starkware force-pushed the noam.s/p2p_class_sync_test_2 branch from 01a090f to 0aa1baf Compare November 26, 2024 16:52
@noamsp-starkware noamsp-starkware force-pushed the noam.s/p2p_class_sync_test_1 branch from f35d949 to 7329d44 Compare November 27, 2024 10:26
@noamsp-starkware noamsp-starkware force-pushed the noam.s/p2p_class_sync_test_2 branch from 0aa1baf to 7506641 Compare November 27, 2024 10:26
@noamsp-starkware noamsp-starkware changed the base branch from noam.s/p2p_class_sync_test_1 to main November 27, 2024 13:01
@noamsp-starkware noamsp-starkware force-pushed the noam.s/p2p_class_sync_test_2 branch from 7506641 to 9439cf3 Compare November 27, 2024 13:03
Copy link
Contributor

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


-- commits line 2 at r2:

Suggestion:

 negative flow
 fail cases 

crates/papyrus_p2p_sync/src/client/class_test.rs line 316 at r2 (raw file):

            panic!("P2P sync aborted with no failure.");
        }
        _ = parse_queries_future => {}

i recommend having a timeout clause to make sure this test ends

Code quote:

        sync_result = p2p_sync.run() => {
            sync_result.unwrap();
            panic!("P2P sync aborted with no failure.");
        }
        _ = parse_queries_future => {}

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 all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @noamsp-starkware)


crates/papyrus_p2p_sync/src/client/class_test.rs line 199 at r2 (raw file):

        vec![2],
        vec![
            Some(StateDiffChunk::DeclaredClass(DeclaredClass::default())),

These two will have the same hash and should also result in a conflict error. Please change one of them to a non default value


crates/papyrus_p2p_sync/src/client/class_test.rs line 218 at r2 (raw file):

// was returned from parse_data_for_block. We currently dont have a way to check this.
#[tokio::test]
async fn class_not_in_state_diff() {

The class_hash and class_hash_in_state_diff in the test are the same. The name of this test should be "class_is_cairo0_and_cairo1_in_state_diff" or something like that. Plus, add a new test that does what the name of this test suggests


crates/papyrus_p2p_sync/src/client/class_test.rs line 239 at r2 (raw file):

        vec![2],
        vec![
            Some(StateDiffChunk::DeclaredClass(DeclaredClass::default())),

This is an invalid state diff (see comment above). Change it to contain different hashes


crates/papyrus_p2p_sync/src/client/class_test.rs line 239 at r2 (raw file):

        vec![2],
        vec![
            Some(StateDiffChunk::DeclaredClass(DeclaredClass::default())),

This test contains two errors (conflicting hashes and missing hashes).
I think what we can do is change it to have 3 hashes in the state diff and 2 classes sent, then we can test that it fails and doesn't wait for the 3rd class to be sent

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.

4 participants