-
Notifications
You must be signed in to change notification settings - Fork 65
test(starknet_os): hint consistency test with local program objects #6931
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
test(starknet_os): hint consistency test with local program objects #6931
Conversation
|
Benchmark movements: No major performance changes detected. |
TzahiTaub
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.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nimrod-starkware)
crates/starknet_os/src/hints/enum_definition_test.rs line 21 at r1 (raw file):
} fn unknown_hints_for_program(program: &Program, filter: &HashSet<&str>) -> HashSet<String> {
Please doc (this arg specifically), and consider renaming
Suggestion:
ignored_hints: &HashSet<&str>crates/starknet_os/src/hints/enum_definition_test.rs line 27 at r1 (raw file):
.iter_hints() .map(|hint| hint.code.clone()) .filter(|hint_str| !filter.contains(hint_str.as_str()))
Can you use a filter_map and avoide the hint code cloning for the filter set?
Code quote:
.map(|hint| hint.code.clone())
.filter(|hint_str| !filter.contains(hint_str.as_str()))crates/starknet_committer_and_os_cli/src/os_cli/tests/python_tests.rs line 97 at r1 (raw file):
} fn vm_hints() -> HashSet<&'static str> {
Delete this as well in your TODO (maybe move it upwards so it will be together with the rest of the code for deletion).
Code quote:
fn vm_hints() -> HashSet<&'static str> {93979e0 to
d7d7284
Compare
dorimedini-starkware
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.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @nimrod-starkware and @TzahiTaub)
crates/starknet_committer_and_os_cli/src/os_cli/tests/python_tests.rs line 97 at r1 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
Delete this as well in your TODO (maybe move it upwards so it will be together with the rest of the code for deletion).
stale
crates/starknet_os/src/hints/enum_definition_test.rs line 21 at r1 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
Please doc (this arg specifically), and consider renaming
went with a static set instead
a discussion (no related file):
py side
dca637c to
6ce1da8
Compare
d7d7284 to
de2f617
Compare
amosStarkware
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.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @nimrod-starkware and @TzahiTaub)
crates/starknet_os/src/hints/enum_definition_test.rs line 67 at r2 (raw file):
#[test] fn test_all_hints_are_known() {
maybe we should also verify that all hints are used?
Code quote:
fn test_all_hints_are_known() {
TzahiTaub
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.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @nimrod-starkware)
6ce1da8 to
394fb8f
Compare
de2f617 to
2265a80
Compare
dorimedini-starkware
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.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @amosStarkware, @nimrod-starkware, and @TzahiTaub)
crates/starknet_os/src/hints/enum_definition_test.rs line 67 at r2 (raw file):
Previously, amosStarkware wrote…
maybe we should also verify that all hints are used?
Done.
2265a80 to
63043ed
Compare
amosStarkware
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.
Reviewed 1 of 2 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware)
crates/starknet_os/src/hints/enum_definition_test.rs line 88 at r3 (raw file):
.filter(|hint| { // Skip syscalls; they do not appear in the OS code. !matches!(hint, AllHints::DeprecatedSyscallHint(_))
This means the deprecated syscalls aren't tested in either test, right?
isn't that a problem?
Code quote:
!matches!(hint, AllHints::DeprecatedSyscallHint(_))
dorimedini-starkware
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @amosStarkware and @nimrod-starkware)
crates/starknet_os/src/hints/enum_definition_test.rs line 88 at r3 (raw file):
Previously, amosStarkware wrote…
This means the deprecated syscalls aren't tested in either test, right?
isn't that a problem?
how would you test them without a flow test?
we could add a test that compiles a cairo0 contract with all syscalls somewhere in it, and then add it's hints to the list of known hints; is that what you mean?
amosStarkware
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @nimrod-starkware)
crates/starknet_os/src/hints/enum_definition_test.rs line 88 at r3 (raw file):
Previously, dorimedini-starkware wrote…
how would you test them without a flow test?
we could add a test that compiles a cairo0 contract with all syscalls somewhere in it, and then add it's hints to the list of known hints; is that what you mean?
that sounds good. add a TODO?
dorimedini-starkware
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @amosStarkware and @nimrod-starkware)
crates/starknet_os/src/hints/enum_definition_test.rs line 88 at r3 (raw file):
Previously, amosStarkware wrote…
that sounds good. add a TODO?
amosStarkware
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware)
crates/starknet_os/src/hints/enum_definition_test.rs line 68 at r4 (raw file):
#[test] fn test_all_hints_are_known() {
comment here & in test_all_hints_are_used that these tests don't include deprecated syscall hints? non blocking
Code quote:
fn test_all_hints_are_known() {
dorimedini-starkware
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.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @nimrod-starkware)
crates/starknet_os/src/hints/enum_definition_test.rs line 68 at r4 (raw file):
Previously, amosStarkware wrote…
comment here & in
test_all_hints_are_usedthat these tests don't include deprecated syscall hints? non blocking
this test does not care, and the second test includes a comment + there will be explicit tests for the syscalls, so I'm good with how things are
63043ed to
4cd582d
Compare
|
Artifacts upload workflows: |
b5cbf8d to
ef0e32d
Compare
ef0e32d to
168e351
Compare

No description provided.