Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3250 +/- ##
==========================================
+ Coverage 81.73% 81.88% +0.14%
==========================================
Files 242 242
Lines 47884 46827 -1057
Branches 41477 40420 -1057
==========================================
- Hits 39139 38344 -795
+ Misses 6723 6589 -134
+ Partials 2022 1894 -128
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
The fix is one line of python, the legacy workaround is a bunch of Rust (ok!), most of it is test, which is also fine....but I do wonder if there's anything we can do to make the tests less low-level / full of JSON details. Could we test serializing from python and then loading it from Rust, say? Or even just add an artefact of JSON from before this PR?
I've tried this on my test cases - taking my guppy-generated Hugr and deserializing in Rust, which previously failed, does now work if I upgrade/patch either guppy or the deserializer, so ok, yes, looks good to me.
| #[case::populated(vec![ConstUsize::new(42).into()])] | ||
| fn wire_format(#[case] contents: Vec<Value>, #[values(false, true)] named: bool) { | ||
| let expected = ListValue::new(usize_t(), contents.clone()); | ||
| let payload = if named { |
There was a problem hiding this comment.
I mean, if ListValueWire derived serde::Serialize, you could do this without writing these json! in the test, right? Is there any reason not to make ListValueWire serialize? It is private, after all... (ah, you might have to call it ListValue and put it in a submodule?)
Ideal would then be to have some example json as it appears in a file in another test, but you could do just one example like that
acl-cqc
left a comment
acl-cqc
left a comment
There was a problem hiding this comment.
Did get my test cases working, as per previous message, so 🚀 (perhaps tests can be simplified!?)
No description provided.