-
Notifications
You must be signed in to change notification settings - Fork 164
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
[Cairo 1 Run] Refactor integration tests + check that return values taken from output segment are correct #1741
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1741 +/- ##
==========================================
- Coverage 94.81% 94.80% -0.01%
==========================================
Files 101 101
Lines 38720 38689 -31
==========================================
- Hits 36711 36680 -31
Misses 2009 2009 ☔ View full report in Codecov by Sentry. |
Benchmark Results for unmodified programs 🚀
|
This reverts commit ef59dd8.
check_append_ret_values_to_output_segment
test
pefontana
approved these changes
Apr 30, 2024
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.
nice one!
igaray
approved these changes
Apr 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After PR #1686 the return values are now fetched from the output segment instead of the execution segment when either the
append_return_values
orproof_mode
flags are enabled, this makes ourcheck_append_ret_values_to_output_segment
not as useful as it no longer checks that the correct values are being copied to the output segment.A way to fix it would be to instead compare the values from the execution segment to the ones on the output segment, but after PR #1721 when the segment arena is used the execution segment now contains the values produced by the arena validation where the return values used to be found, so we can no longer use it for comparison.
As a result of these two changes, the best way to test that the correct values are copied to the output segment are to 1: test that the output segment indeed has the return values outputted and no more values after them (current behaviour), and 2: test that the return values outputted when running with
--append_return_values
match the ones outputted by a normal run (this can be accomplished by adding a third case with--append_return_values
enabled to our in integration tests.This PR adds this third test case and also refactors the integration tests to into one test with multiple cases & values so adding new checks and argument combinations is easier.