feat: add test_helpers module (error_utils, test_utils) behind test_utlis flag#2381
feat: add test_helpers module (error_utils, test_utils) behind test_utlis flag#2381naor-starkware wants to merge 1 commit intomainfrom
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2381 +/- ##
========================================
Coverage 96.17% 96.18%
========================================
Files 105 107 +2
Lines 37770 37885 +115
========================================
+ Hits 36326 36438 +112
- Misses 1444 1447 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Benchmark Results for unmodified programs 🚀
|
YairVaknin-starkware
left a comment
There was a problem hiding this comment.
@YairVaknin-starkware reviewed 9 files and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on naor-starkware).
vm/src/test_helpers/error_utils.rs line 44 at r2 (raw file):
/// Type alias for check functions that validate test results. pub type VmCheck<T> = fn(&std::result::Result<T, CairoRunError>);
can just be Result, right? pls looks for similar instances where you can shorten.
Code quote:
&std::result::Result<T, CairoRunError>vm/src/test_helpers/error_utils.rs line 62 at r2 (raw file):
} /// Asserts that the result is `HintError::AssertNotEqualFail`.
This funcs have very repetitive boiler plate. Pls extract to a single func which these func will invoke that will get the res and the predicate u check.
Code quote:
/// Asserts that the result is `HintError::AssertNotEqualFail`.vm/src/test_helpers/error_utils.rs line 220 at r2 (raw file):
} /// `expect_hint_assert_not_zero` does not panic on `HintError::AssertNotZero`.
these error tests could be parameterized using rtest to reduce alot of repetitive boilerplate.
vm/src/test_helpers/test_utils.rs line 48 at r2 (raw file):
Ok(v) => v, Err(e) => panic!("conversion to MaybeRelocatable failed: {e:?}"), };
pls factor out the conversion logic of both cases into a single func which also enforces coercion into MaybeRelocatable (currently it will work for any right that is able to be try_into'd left's type).
Code quote:
let right_mr = match ($right).try_into() {
Ok(v) => v,
Err(e) => panic!("conversion to MaybeRelocatable failed: {e:?}"),
};
naor-starkware
left a comment
There was a problem hiding this comment.
@naor-starkware made 4 comments.
Reviewable status: 7 of 10 files reviewed, 4 unresolved discussions (waiting on YairVaknin-starkware).
vm/src/test_helpers/error_utils.rs line 44 at r2 (raw file):
Previously, YairVaknin-starkware wrote…
can just be Result, right? pls looks for similar instances where you can shorten.
Done.
vm/src/test_helpers/error_utils.rs line 62 at r2 (raw file):
Previously, YairVaknin-starkware wrote…
This funcs have very repetitive boiler plate. Pls extract to a single func which these func will invoke that will get the res and the predicate u check.
Done.
vm/src/test_helpers/error_utils.rs line 220 at r2 (raw file):
Previously, YairVaknin-starkware wrote…
these error tests could be parameterized using rtest to reduce alot of repetitive boilerplate.
Done.
vm/src/test_helpers/test_utils.rs line 48 at r2 (raw file):
Previously, YairVaknin-starkware wrote…
pls factor out the conversion logic of both cases into a single func which also enforces coercion into MaybeRelocatable (currently it will work for any right that is able to be try_into'd left's type).
Done.
OmriEshhar1
left a comment
There was a problem hiding this comment.
@OmriEshhar1 reviewed 7 files and all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on YairVaknin-starkware).
67106b9 to
dc5490c
Compare
OmriEshhar1
left a comment
There was a problem hiding this comment.
@OmriEshhar1 reviewed 2 files and all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on YairVaknin-starkware).
OmriEshhar1
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on YairVaknin-starkware).

TITLE
Description
Description of the pull request changes and motivation.
Checklist
This change is