-
Notifications
You must be signed in to change notification settings - Fork 65
starknet_os_flow_tests: migrate test_new_class_flow #9518
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
starknet_os_flow_tests: migrate test_new_class_flow #9518
Conversation
|
Benchmark movements: tree_computation_flow performance regressed! tree_computation_flow time: [14.255 ms 14.440 ms 14.678 ms] change: [+1.4730% +3.7079% +5.9993%] (p = 0.00 < 0.05) Performance has regressed. Found 12 outliers among 100 measurements (12.00%) 4 (4.00%) low mild 5 (5.00%) high mild 3 (3.00%) high severe full_committer_flow performance regressed! full_committer_flow time: [15.106 ms 15.227 ms 15.348 ms] change: [+3.9994% +5.2575% +6.4580%] (p = 0.00 < 0.05) Performance has regressed. Found 7 outliers among 100 measurements (7.00%) 5 (5.00%) low mild 2 (2.00%) high mild |
94f9118 to
f4a15c3
Compare
f9019ac to
79bbe29
Compare
f4a15c3 to
d21bc26
Compare
79bbe29 to
ecd4779
Compare
d21bc26 to
c1fd831
Compare
ecd4779 to
2a778ba
Compare
c1fd831 to
5d37d86
Compare
2a778ba to
5b10122
Compare
Yoni-Starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Yoni-Starkware reviewed 3 of 4 files at r1, all commit messages.
Reviewable status: 3 of 4 files reviewed, all discussions resolved (waiting on @AvivYossef-starkware and @meship-starkware)
5d37d86 to
fe85319
Compare
5b10122 to
cdc7e45
Compare
fe85319 to
5433958
Compare
cdc7e45 to
960b14e
Compare
5433958 to
bdaeac4
Compare
960b14e to
ceee170
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware and @meship-starkware)
crates/starknet_os_flow_tests/src/tests.rs line 1039 at r2 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
This is the exact function we defined in test_os_logic. Consider sharing the code here
can be done in a separate PR
Done (in the new PR below this one)
9f83fbb to
20e8e13
Compare
0f89bae to
98871ee
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware and @meship-starkware)
a discussion (no related file):
Previously, meship-starkware (Meshi Peled) wrote…
Is there a reason the new_class test is one test? It is very long and hard to debug. I think that we can split it into two or three different tests. If we want to avoid adding another test, let's at least split its content into helper functions
Done, in PRs above this one (3 small refactors, 3 splits, one for test rename)
20e8e13 to
4a982ca
Compare
98871ee to
4f3af3f
Compare
4a982ca to
d40cde6
Compare
4f3af3f to
e8f2d7a
Compare
e8f2d7a to
6cfbd68
Compare
meship-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@meship-starkware reviewed 1 of 3 files at r3, 1 of 1 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @AvivYossef-starkware)
crates/starknet_os_flow_tests/src/tests.rs line 1318 at r5 (raw file):
nonce: test_manager.next_nonce(*FUNDED_ACCOUNT_ADDRESS), }; let account_declare_tx = declare_tx(declare_tx_args);
Why aren't we using Bootstrap declare here?
Code quote:
let account_declare_tx = declare_tx(declare_tx_args);crates/starknet_os_flow_tests/src/tests.rs line 1357 at r5 (raw file):
&[Felt::from(queried_block_number.0), old_block_hash.0], ); test_manager.add_funded_account_invoke(invoke_tx_args! { calldata });
Do we check the message to L1? If not, should we test it here?
Code quote:
test_manager.add_funded_account_invoke(invoke_tx_args! { calldata });crates/starknet_os_flow_tests/src/tests.rs line 1380 at r5 (raw file):
test_library_call_key, test_library_call_value, );
Suggestion:
update_expected_storage(
&mut expected_storage_updates,
//Library call write to the caller storage
main_contract_address,
test_library_call_key,
test_library_call_value,
);6cfbd68 to
0903cff
Compare
dorimedini-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @AvivYossef-starkware and @meship-starkware)
crates/starknet_os_flow_tests/src/tests.rs line 1318 at r5 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
Why aren't we using Bootstrap declare here?
done. I didn't see a point, but I don't see why not either
crates/starknet_os_flow_tests/src/tests.rs line 1357 at r5 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
Do we check the message to L1? If not, should we test it here?
test_get_block_hash doesn't send an L1 message, this was an error in the original test (note that message_to_l1 is defined twice, once after test_send_message_to_l1 and once again after test_get_block_hash)
meship-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@meship-starkware reviewed 1 of 1 files at r6, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware)

No description provided.