Skip to content
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

New test vector transcript format #1315

Merged
merged 19 commits into from Jul 11, 2019

Conversation

Projects
None yet
4 participants
@gdbelvin
Copy link
Collaborator

commented Jul 11, 2019

This PR makes the test vector file format self contained.

Previously public verification keys and the contents they were verifying were contained in separate files.

gdbelvin added some commits Jul 8, 2019

@gdbelvin gdbelvin requested review from thaidn and google/keytransparency as code owners Jul 11, 2019

@googlebot googlebot added the cla: yes label Jul 11, 2019

@codecov

This comment has been minimized.

Copy link

commented Jul 11, 2019

Codecov Report

Merging #1315 into master will decrease coverage by 0.39%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1315     +/-   ##
=========================================
- Coverage   30.45%   30.06%   -0.4%     
=========================================
  Files          45       46      +1     
  Lines        3769     3815     +46     
=========================================
- Hits         1148     1147      -1     
- Misses       2445     2492     +47     
  Partials      176      176
Impacted Files Coverage Δ
core/testdata/io.go 0% <0%> (ø)
core/integration/client_tests.go 0% <0%> (ø) ⬆️
core/integration/monitor_tests.go 0% <0%> (ø) ⬆️
core/client/client.go 29.55% <0%> (-0.63%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80d34ad...18355a4. Read the comment docs.

Show resolved Hide resolved core/testdata/transcript_go_proto/transcript.proto
Show resolved Hide resolved core/testdata/transcript_go_proto/transcript.proto Outdated
Show resolved Hide resolved core/testdata/transcript_go_proto/gen.go Outdated
Show resolved Hide resolved core/testdata/io.go Outdated
@@ -0,0 +1,53 @@
// Copyright 2019 Google Inc. All Rights Reserved.

This comment has been minimized.

Copy link
@pavelkalinnikov

pavelkalinnikov Jul 11, 2019

Is transcript_go_proto a canonical name for this folder? Technically, transcript.proto can be compiled into other languages (although it's not the case here).

This comment has been minimized.

Copy link
@gdbelvin

gdbelvin Jul 11, 2019

Author Collaborator

This is a tricky one...

  1. It's Google policy to have go proto packages end in *_go_proto
  2. When importing into Bazel, the *.pb.go files get dropped, and only the proto file remains
  3. The import script also modifies every import/path/A -> package/import/path/A/A

By putting the file in this directory, I think everything should be consistent (for go) automatically.

Alternatively, I can put the transcript.proto file on directory up, and then special case it in the import script.

Show resolved Hide resolved core/testdata/TestListHistory.json
Show resolved Hide resolved core/crypto/vrf/p256/p256_test.go Outdated
Show resolved Hide resolved core/crypto/vrf/p256/p256_test.go Outdated
Show resolved Hide resolved core/crypto/vrf/p256/p256_test.go Outdated
@@ -0,0 +1,31 @@
{
"description": "TestMonitor",
"directory": {

This comment has been minimized.

Copy link
@pavelkalinnikov

pavelkalinnikov Jul 11, 2019

This test, as well as TestListHistory and a couple of others, has no RPCs. Is this expected, and they are still different?

This comment has been minimized.

Copy link
@gdbelvin

gdbelvin Jul 11, 2019

Author Collaborator

This is expected.

At the moment they are not generating RPCs because the individual tests are not generating them.
What I mean to say is that, at the moment, the RPCs are not being collected in a consistent way.
They are being synthetically synthesized rather than properly recorded with an interceptor.
This is because we are still injecting client state into the Action. When I remove that, I can properly capture all the RPCs and these files won't be empty.

gdbelvin added some commits Jul 11, 2019

Show resolved Hide resolved core/client/verifier/verifier_test.go Outdated
Show resolved Hide resolved core/client/verifier/verifier_test.go
Show resolved Hide resolved core/crypto/vrf/p256/p256_test.go Outdated
Show resolved Hide resolved core/crypto/vrf/p256/p256_test.go
Show resolved Hide resolved core/crypto/vrf/p256/p256_test.go

gdbelvin added some commits Jul 11, 2019

@gdbelvin gdbelvin force-pushed the gdbelvin:transcript branch from 09e5874 to 69e6cbc Jul 11, 2019

@gdbelvin gdbelvin requested a review from pavelkalinnikov Jul 11, 2019

@gdbelvin

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 11, 2019

Updated, PTAL

@pavelkalinnikov
Copy link

left a comment

Please take another look at err := WriteTranscript comment before submitting.

@gdbelvin

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 11, 2019

Sharp eye. Thanks for that

@gdbelvin gdbelvin merged commit a9cb328 into google:master Jul 11, 2019

3 of 5 checks passed

codecov/patch 0% of diff hit (target 30.45%)
Details
codecov/project 30.06% (-0.4%) compared to 80d34ad
Details
GolangCI No issues found!
Details
Travis CI - Pull Request Build Passed
Details
cla/google All necessary CLAs are signed

@gdbelvin gdbelvin deleted the gdbelvin:transcript branch Jul 11, 2019

gdbelvin added a commit to gdbelvin/keytransparency that referenced this pull request Jul 15, 2019

Merge branch 'master' into errmsg
* master:
  New test vector transcript format (google#1315)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.