Fix forge scheduled tests #3003
Conversation
ddoktorski
force-pushed
the
fix-scheduled-tests
branch
from
4506aa3 to
2b83d6d
Compare
| @@ -288,7 +288,7 @@ pub fn assert_gas(result: &[TestTargetSummary], test_case_name: &str, asserted_g | |||
| } | |||
| AnyTestCaseSummary::Single(case) => match case { | |||
| TestCaseSummary::Passed { gas_info: gas, .. } => { | |||
| *gas == asserted_gas | |||
| asserted_gas.abs_diff(*gas) <= 1 | |||
There was a problem hiding this comment.
Can we add a different assert_gas method that uses diff and choose which one to use according to the feature? Then the scheduled tests on version other than scarb from .tool-version could use it.
E.g.
if cfg!(feature = "non_exact_gas") {
assert_gas_range(...)
else {
assert_gas(...)
}I'd prefer if we still kept exact gas assertions when developing code. I know difference of 1 is not that big but we should assert exact values here.
There was a problem hiding this comment.
As discussed offline, for now leaving it as it is
There was a problem hiding this comment.
I guess one thing we are losing with these changes is that we no longer assert if the max steps is sufficiently close to the default of 10m. Maybe with some logic changes we could still retain this check?
There was a problem hiding this comment.
Or we could hide this check behind a feature as well, what do you think
There was a problem hiding this comment.
This has already been changed a few times and it is unmaintainable. Unfortunately, I don't have any idea for a test that will have 10m steps and for sure remain unchanged with future Cairo versions.
There was a problem hiding this comment.
I modified those tests to get the following steps values:
- Scarb
2.8.5:8250085and10200085 - Scarb
2.10.1:9350088and11560088
It seems to be close enough to test the desired behavior (failing for >10m steps - tested in should_default_to_10m test) and hopefully it won't need to be updated with the new Scarb version 🙏
| @@ -76,7 +76,7 @@ fn fuzzed_while_loop() { | |||
|
|
|||
| // TODO (#2926) | |||
| assert_eq!(gas_info.min, 2); | |||
| assert_eq!(gas_info.max, 23); | |||
| assert!(gas_info.max.abs_diff(23) <= 1); | |||
Closes #2998
Introduced changes