Skip to content
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

Cleanup - test_interpreter_and_jit!() macros #13

Merged
merged 11 commits into from
Jan 3, 2025

Conversation

Lichtso
Copy link
Collaborator

@Lichtso Lichtso commented Jan 2, 2025

No description provided.

@Lichtso Lichtso requested a review from LucasSte January 2, 2025 22:08
@Lichtso Lichtso force-pushed the refactor/test_utils_macros branch from 2a85b2b to 3665510 Compare January 2, 2025 23:09
@@ -3535,46 +3535,6 @@ fn test_invalid_call_imm() {
);
}

#[test]
#[should_panic(expected = "Invalid syscall should have been detected in the verifier.")]
fn test_invalid_exit_or_return() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea of this test was to ensure correct behavior if execution reaches an invalid sys call not detected in the verifier.

If you think that will never happen, we can simply live without this test.

Copy link
Collaborator Author

@Lichtso Lichtso Jan 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I moved it to the verifier tests as these are execution only tests. And things which are supposed to fail verification have no defined execution behavior.

@@ -3501,40 +3501,6 @@ fn test_total_chaos() {
}
}

#[test]
fn test_invalid_call_imm() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was here to make sure one previous bug I introduced in the codebase doesn't exist anymore. The bug was dispatching syscalls with call imm.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added test_call_imm_does_not_dispatch_syscalls()

test_utils/src/lib.rs Outdated Show resolved Hide resolved
test_utils/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 233 to 236
if !$override_budget {
const INSTRUCTION_METER_BUDGET: u64 = 1024;
context_object.remaining = INSTRUCTION_METER_BUDGET;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

override_budget == true will NOT override the budget.
override_budget == false WILL override the budget.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, we should invert the flag and its use sites.

@Lichtso Lichtso force-pushed the refactor/test_utils_macros branch from 3665510 to 0fe538f Compare January 3, 2025 12:58
@@ -374,10 +374,27 @@ fn test_verifier_err_unknown_opcode() {
}

#[test]
#[should_panic(expected = "InvalidFunction(1811268607)")]
#[should_panic(expected = "UnknownOpCode(6, 0)")]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we testing the invalid exit? Shouldn't we be throwing an error for 0x95?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test_verifier_err_invalid_return appears to have the same issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, will rewrite these two tests.

Comment on lines 394 to 397
#[should_panic(expected = "InvalidFunction(3)")]
fn test_verifier_unknown_sycall() {
let prog = &[
0x85, 0x00, 0x00, 0x00, 0xfe, 0xc3, 0xf5, 0x6b, // call 0x6bf5c3fe
0x85, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, // call_imm 2
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0x85 is not a sys call in V3.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, also fixed.

@Lichtso Lichtso force-pushed the refactor/test_utils_macros branch from 0fe538f to fc1e828 Compare January 3, 2025 13:39
fn test_verifier_unknown_sycall() {
let prog = &[
0x85, 0x00, 0x00, 0x00, 0xfe, 0xc3, 0xf5, 0x6b, // call 0x6bf5c3fe
0x95, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, // call_imm 2
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
0x95, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, // call_imm 2
0x95, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, // syscall 2

@Lichtso Lichtso force-pushed the refactor/test_utils_macros branch from fc1e828 to f5e5e30 Compare January 3, 2025 14:10
@Lichtso Lichtso merged commit bd81cd3 into main Jan 3, 2025
11 checks passed
@Lichtso Lichtso deleted the refactor/test_utils_macros branch January 3, 2025 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants