Make golden proof test data available to other languages#171
Make golden proof test data available to other languages#171AlCutter merged 21 commits intotransparency-dev:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
mhutchinson
left a comment
There was a problem hiding this comment.
This is great work, thanks for doing this 🙏
I wanted to check this out locally, and found a couple of issues while doing so. I think they were non-contentious (fixing Fatal -> Fatalf, and deleting files that were no longer generated). LMK if you have issue though, I don't want to step on your toes!
I also added some bits to the presubmit check to make it easier to test that the generated files are still golden. I'll wire that up to github actions in a future PR though.
You'll need to accept the CLA to get this in. Hopefully that's not an issue.
I'll assign this to @AlCutter for final review now that I've left my fingerprints on this :-)
AlCutter
left a comment
There was a problem hiding this comment.
Thanks for taking the time to send this PR!
I'm mostly just pointing out a few Go style bits along with a few suggestions.
I do worry about the special characters in some of the test data filenames (e.g. *, [, ]) - I think from the PoV of the code in here it's fine, but it's a potential "rake in the grass" for folks coming along later and hitting unexpected shell expansion issues. Perhaps we could use mul instead of * (similar to div you've used), and @<num> for the indices?
Ultimately it would be nice to have a README in here to help folks wanting to use the golden data to understand what's going on, clear descriptions of set up & tests, etc., but that's on us rather than you!
| go run ./cmd/proofgen | ||
| fi | ||
|
|
||
| if [[ "${empty_diff}" -eq 1 ]]; then |
There was a problem hiding this comment.
This is a great addition. Thanks @mhutchinson !
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #171 +/- ##
===========================================
- Coverage 89.33% 57.69% -31.64%
===========================================
Files 7 8 +1
Lines 497 773 +276
===========================================
+ Hits 444 446 +2
- Misses 48 322 +274
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for the changes @tannaurus! The linter has caught a bunch of errors which will need to be addressed before we can merge the PR, but I think this is a good step forward in making proofs more widely reusable. |
bcc3f30 to
848a337
Compare
…test data to disk
…nics with Fatalf, only using Fatalf inside main
848a337 to
ac550d5
Compare
|
Thanks again @tannaurus! |
|
Thank YOU for the quick turn around on reviews 🙂 that was a fantastic open source experience. Feel free to mention that on a performance review 😉 |
Summary
I created a new
proofgenbinary writes all of the test cases into json files. Theverify_gotests have been updated to walk the file tree and use each json file as input.Thought Process
Every json file contains a
wantErrflag to communicate to the consumer that this input should result in an error. Similarly, every input contains adescfield that describes what the test is attempting to validate. These two concepts were inconsistently used in the previous test suite so in an effort in standardize the json schema I adopted it everywhere.Because the "desc" field is new to many of these tests, I had to do a bit of reverse engineering to figure out what each test input was trying to test. I'd encourage the reviewers here to validate that these additions are accurate. An example of this can be found in
writeStaticConsistencyTestData. On that note, you'll notice some of these new descriptions are repetitive, because the test cases themselves are repetitive. I'd be happy to remove some of that repetition but I figured I'd defer to the maintainers before doing so.A general issue with the test data that I believe some consumers may struggle with: a lot of these tests are (rightfully) designed for this specific implementation. Depending on the order in which a consumer performs these checks themselves, they may receive false negatives.
Lastly, a general note: I am not a Go developer, so feel free to suggest improvements wherever you see them 😄
Additional Changes
The first commit here removes a test case that was never referenced. The only reference to
inclusionProofsskipped the first index because it was invalid.Related Issue
Resolves: #169
Testing instructions
testdatadirectorygo run ./cmd/proofgenproofdirectory