chore: add more tests #361
Conversation
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #361 +/- ##
==========================================
+ Coverage 82.02% 85.07% +3.05%
==========================================
Files 85 85
Lines 17989 18658 +669
==========================================
+ Hits 14755 15874 +1119
+ Misses 3234 2784 -450 ☔ View full report in Codecov by Sentry. |
| // serialization test for vk | ||
| let f = File::create("serialization-test.vk").unwrap(); | ||
| let mut writer = BufWriter::new(f); | ||
| vk.write(&mut writer, SerdeFormat::RawBytes).unwrap(); | ||
| writer.flush().unwrap(); | ||
|
|
||
| let f = File::open("serialization-test.vk").unwrap(); | ||
| let mut reader = BufReader::new(f); | ||
| #[allow(clippy::unit_arg)] | ||
| let vk = vk_read::<G1Affine, _, StandardPlonk>( | ||
| &mut reader, | ||
| SerdeFormat::RawBytes, | ||
| k, | ||
| &circuit, | ||
| compress_selectors, | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| std::fs::remove_file("serialization-test.vk").unwrap(); | ||
|
|
||
| // serialization test for pk |
There was a problem hiding this comment.
IMO is better to use an internal buffer, and when not possible use something like https://crates.io/crates/tempfile
There was a problem hiding this comment.
Hmm, how about leaving the current use of std::fs::File as is?
The goal of this test is to show the file operations and use of pk/vk_read.
There was a problem hiding this comment.
The goal of this test is to show the file operations and use of pk/vk_read.
If we want to show the usage, I think that the proper way is to add it halo2_proofs/examples, or, if there's no File/Networking I/O as a documentation test (like https://doc.rust-lang.org/1.81.0/src/core/array/mod.rs.html#107)
There was a problem hiding this comment.
Maybe we can remove it, since now it's in the examples?
halo2_frontend/src/dev/cost.rs
Outdated
| let cost = CircuitCost::<Eq, MyCircuit<Fp, 2>>::measure(K, &my_circuit); | ||
|
|
||
| let marginal_proof_size = cost.marginal_proof_size(); | ||
| assert_eq!(usize::from(marginal_proof_size), 1376); |
There was a problem hiding this comment.
What is the point in create this complex circuit to check the cost?
Are we checking in any way that 1376 is the expected result for this circuit?
There was a problem hiding this comment.
Hmm, no.
I add this test to show how to use the tool, and the output from tool usage.
There was a problem hiding this comment.
Ditto in #361 (comment), it should go into examples, because there's no proper test here. I think that this could make sense as a test if we want to be absolutely sure that costs does not change due some side effects, but I do not think that this is the case.
Thanks for indication! |
Description
It is needed to add more unit and negative tests for the codebase.
Related issues
Changes
frontend/devmodules