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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.