-
Notifications
You must be signed in to change notification settings - Fork 21
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
chore(blockifier): added tests for get_vm_resources_cost #927
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #927 +/- ##
==========================================
- Coverage 74.18% 70.60% -3.59%
==========================================
Files 359 88 -271
Lines 36240 11347 -24893
Branches 36240 11347 -24893
==========================================
- Hits 26886 8011 -18875
+ Misses 7220 2958 -4262
+ Partials 2134 378 -1756
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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: 0 of 2 files reviewed, 1 unresolved discussion
-- commits
line 2 at r1:
should I add L2 gas test cases to any more tests?
263ec14
to
8425324
Compare
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.
+reviewer:@aner-starkware
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @aner-starkware)
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.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware)
a discussion (no related file):
Blocking until approved by @dorimedini-starkware
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.
Reviewed all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @amosStarkware and @aner-starkware)
a discussion (no related file):
Previously, aner-starkware wrote…
Blocking until approved by @dorimedini-starkware
done
Previously, amosStarkware wrote…
should I add L2 gas test cases to any more tests?
we will extend existing tests when we do this
crates/blockifier/src/fee/fee_test.rs
line 149 at r3 (raw file):
versioned_constants.convert_l1_to_l2_gas_amount_round_up(l1_gas_by_vm_usage), ), };
these lines are duplicated 4 times, please add a tiny test util and use it
Code quote:
let expected_gas_vector = match gas_vector_computation_mode {
GasVectorComputationMode::NoL2Gas => GasVector::from_l1_gas(l1_gas_by_vm_usage),
GasVectorComputationMode::All => GasVector::from_l2_gas(
versioned_constants.convert_l1_to_l2_gas_amount_round_up(l1_gas_by_vm_usage),
),
};
8425324
to
2cb598e
Compare
2cb598e
to
5ddb61d
Compare
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: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @aner-starkware and @dorimedini-starkware)
crates/blockifier/src/fee/fee_test.rs
line 149 at r3 (raw file):
Previously, dorimedini-starkware wrote…
these lines are duplicated 4 times, please add a tiny test util and use it
Done.
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: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @amosStarkware and @dorimedini-starkware)
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.
Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @amosStarkware)
This change is