add crosscheck tests for filter function #388
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.
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.
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.
- 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 realcrosscheckhere.
| 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.
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.
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.