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(starknet_client): add tests for a 0.13.2 block #2181

Closed
wants to merge 1 commit into from

Conversation

ShahakShama
Copy link
Contributor

@ShahakShama ShahakShama commented Jul 2, 2024

  • fix: put none in commitments if they come from a deprecated formula
  • test(starknet_client): add tests for a 0.13.2 block

This change is Reviewable

@ShahakShama ShahakShama changed the base branch from main to shahak/optional_state_diff_commitment_and_none_deprecated_hashes July 2, 2024 06:47
Copy link

codecov bot commented Jul 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.41%. Comparing base (f5265b6) to head (9ff8a6a).
Report is 58 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2181      +/-   ##
==========================================
+ Coverage   65.97%   66.41%   +0.44%     
==========================================
  Files         136      139       +3     
  Lines       18128    18379     +251     
  Branches    18128    18379     +251     
==========================================
+ Hits        11960    12207     +247     
  Misses       4876     4876              
- Partials     1292     1296       +4     

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

matan-starkware
matan-starkware previously approved these changes Jul 2, 2024
Copy link
Contributor

@matan-starkware matan-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 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dan-starkware and @ShahakShama)


crates/starknet_client/resources/reader/block_post_0_13_2.json line 1 at r1 (raw file):

{

IDK how to review this file, so I'm relying on your test making use of it and that's it...

Code quote:

{

crates/starknet_client/src/reader/objects/block_test.rs line 159 at r1 (raw file):

    );

    // TODO(shahak): Find a block with at least 2 transactions, and uncomment the code below.

What is the blocker for this? Why not do it now?

Code quote:

    // TODO(shahak): Find a block with at least 2 transactions, and uncomment the code below.

@ShahakShama ShahakShama force-pushed the shahak/optional_state_diff_commitment_and_none_deprecated_hashes branch from dc9e2fe to e73f058 Compare July 2, 2024 07:03
Base automatically changed from shahak/optional_state_diff_commitment_and_none_deprecated_hashes to main July 2, 2024 08:18
@ShahakShama ShahakShama dismissed matan-starkware’s stale review July 2, 2024 08:18

The base branch was changed.

@ShahakShama ShahakShama force-pushed the shahak/resources_0.13.2_block branch from ae49485 to c1aa4e5 Compare July 2, 2024 08:29
@ShahakShama ShahakShama force-pushed the shahak/resources_0.13.2_block branch from c1aa4e5 to 5f46255 Compare July 15, 2024 06:12
Copy link
Contributor Author

@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: 0 of 2 files reviewed, all discussions resolved (waiting on @dan-starkware and @matan-starkware)


crates/starknet_client/src/reader/objects/block_test.rs line 159 at r1 (raw file):

Previously, matan-starkware wrote…

What is the blocker for this? Why not do it now?

Done

Copy link
Contributor

@matan-starkware matan-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 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dan-starkware and @ShahakShama)


crates/starknet_client/src/reader/objects/block_test.rs line 175 at r3 (raw file):

            }
        )
    );

It looks like you don't care about any of the fields, so why not remove this clutter?

(for the entire test)

Suggestion:

    assert_matches!(
        err,
        ReaderClientError::TransactionReceiptsError(
_        )
    );

@ShahakShama ShahakShama force-pushed the shahak/resources_0.13.2_block branch from 5f46255 to b66a914 Compare July 16, 2024 04:44
Copy link
Contributor Author

@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, 1 unresolved discussion (waiting on @dan-starkware and @matan-starkware)


crates/starknet_client/src/reader/objects/block_test.rs line 175 at r3 (raw file):

Previously, matan-starkware wrote…

It looks like you don't care about any of the fields, so why not remove this clutter?

(for the entire test)

Done.

Copy link
Contributor

@matan-starkware matan-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, 3 unresolved discussions (waiting on @dan-starkware and @ShahakShama)


crates/starknet_client/src/reader/objects/block_test.rs line 175 at r3 (raw file):

Previously, ShahakShama wrote…

Done.

I left comments on the other places "(for the entire test)"


crates/starknet_client/src/reader/objects/block_test.rs line 126 at r4 (raw file):

                num_of_receipts: _,
            }
        )

Suggestion:

        ReaderClientError::TransactionReceiptsError(
_
        )

crates/starknet_client/src/reader/objects/block_test.rs line 141 at r4 (raw file):

                receipt_tx_index: _,
            }
        )

Suggestion:

        ReaderClientError::TransactionReceiptsError(
_
        )

crates/starknet_client/src/reader/objects/block_test.rs line 156 at r4 (raw file):

                receipt_tx_hash: _,
            }
        )

Suggestion:

        ReaderClientError::TransactionReceiptsError(
_
        )

@ShahakShama ShahakShama force-pushed the shahak/resources_0.13.2_block branch from b66a914 to 9ff8a6a Compare July 25, 2024 04:29
Copy link
Contributor Author

@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.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 26, 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.

2 participants