-
Notifications
You must be signed in to change notification settings - Fork 11
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
add crosscheck tests for filter function #388
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #388 +/- ##
==========================================
- Coverage 86.94% 86.93% -0.02%
==========================================
Files 43 43
Lines 18232 18232
Branches 18232 18232
==========================================
- Hits 15852 15850 -2
- Misses 1044 1046 +2
Partials 1336 1336 ☔ View full report in Codecov by Sentry. |
|
||
match seq_data { | ||
clarity::vm::types::SequenceData::Buffer(_) => { | ||
let snippet = format!("(define-private (only-zero (byte (buff 1))) (is-eq byte 0x00)) (buff-to-int-be (filter only-zero {seq}))"); |
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.
This test doesn't allow us to check that the resulting buffer contains the right number of 0s. It should return a buffer instead of converting it to int.
I would also not use 0 in the predicate, because this is the default value of uninitialized memory in Wasm. So we don't know if the result is correct because it wrote the correct 0, or if it wrote nothing and we have the default 0 that was there in memory.
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.
Changed the implementation to rely also on other values.
&format!("(define-private (foo (el (list {max_len} {item_ty}))) | ||
(not (is-eq u0 (len el)))) | ||
(filter foo (list {seq}))" |
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.
- Creating an empty list is not a frequent event, so I don't think this test is super reliable.
- It doesn't check that the result of a filter operation leaves the correct number of element in the result list.
- This is a
crosscheck_compare_only
, and we already had issues with this not doing what we expected. I suggest using a realcrosscheck
here.
let v: Vec<u32> = data.as_str().chars().filter_map(|a| a.to_digit(10)).collect(); | ||
let expected: String = v.into_iter().map(|i| i.to_string()).collect::<String>(); | ||
let snippet = format!("(define-private (is-int (char (string-ascii 1))) | ||
(is-eq 0 (* 0 (unwrap! (string-to-int? char) false)))) |
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.
If we replace all elements by a fixed 0
, we are not sure the result of the filter would contain the correct elements.
crosscheck( | ||
&snippet, | ||
Ok(Some(Value::Sequence(SequenceData::String( | ||
CharType::ASCII(ASCIIData { |
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.
I don't understand this test: there is no distinction between ascii and utf8 string in the match clause, but the result is systematically ascii? What did I miss?
I pushed the branch workaround to the repo. Feel free to merge it to yours, or refactor from it. There are a few things to consider though:
|
No description provided.