-
Notifications
You must be signed in to change notification settings - Fork 720
AAC: consensus tests for EarlyReturnError variants
#6736
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
AAC: consensus tests for EarlyReturnError variants
#6736
Conversation
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
federico-stacks
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.
Just a quick note: this isn’t related to the PR itself, but something I noticed while reviewing it.
If I remember correctly, we had agreed to rename the variant ClarityError::Interpreter(VmExecutionError) to ClarityError::Execution(VmExecutionError). However, it’s still present as ClarityError::Interpreter.
Did we change our minds on that at some point (and I’m just forgetting), or did the rename simply never happen (or lost in one of many merge conflicts)?
francesco-stacks
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.
there a problem with rust fmt. Apart from that everything looks good!
The name didn't ring a bell, but I went through old conversations and files, and you are correct. It was once proposed to be renamed that way (by me D: ). I am not sure If I missed that during the renaming or there was another reason behind. Anyway, now that there are no other conflicting variants or enums with the same name, |
Yeah i thought for some reason it had been changed already but apparently not. I think it got lost in a merge as part of the VmExecution rename. But I am good either way. |
Signed-off-by: Jacinta Ferrant <[email protected]>
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (73.22%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #6736 +/- ##
============================================
+ Coverage 26.10% 73.22% +47.12%
============================================
Files 579 580 +1
Lines 360068 360194 +126
============================================
+ Hits 93986 263761 +169775
+ Misses 266082 96433 -169649
... and 517 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
… into chore/aac-early-return-error-consensus-tests
…p test for this case Signed-off-by: Jacinta Ferrant <[email protected]>
brice-stacks
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.
lgtm!
fdf1041
Closes #6702