Allow log server to return consistency proofs where first=second #819
Conversation
This is for additional compatiblity with the older C++ log server code.
Codecov Report
@@ Coverage Diff @@
## master #819 +/- ##
=======================================
Coverage 53.47% 53.47%
=======================================
Files 7 7
Lines 374 374
=======================================
Hits 200 200
Misses 156 156
Partials 18 18Continue to review full report at Codecov.
|
| } | ||
| } | ||
|
|
||
| func TestGetConsistencyProof(t *testing.T) { |
There was a problem hiding this comment.
Seems like a lot of these tests could be collapsed into a table-driven one which includes a wantError flag, no?
Perhaps that could be done in a follow-up, though.
There was a problem hiding this comment.
Yes it could be done but seemed out of scope for this change.
server/validate.go
Outdated
| @@ -51,7 +51,7 @@ func validateGetConsistencyProofRequest(req *trillian.GetConsistencyProofRequest | |||
| if req.SecondTreeSize <= 0 { | |||
| return status.Errorf(codes.InvalidArgument, "GetConsistencyProofRequest.SecondTreeSize: %v, want > 0", req.SecondTreeSize) | |||
| } | |||
| if req.SecondTreeSize <= req.FirstTreeSize { | |||
| if req.SecondTreeSize < req.FirstTreeSize { | |||
| return status.Errorf(codes.InvalidArgument, "GetConsistencyProofRequest.FirstTreeSize: %v < GetConsistencyProofRequest.SecondTreeSize: %v, want > ", req.FirstTreeSize, req.SecondTreeSize) | |||
|
Finally got Travis to run. PTAL. |
|
@daviddrysdale can you take a quick look? I've got a followup in progress to address the test comment Al made. |
|
|
||
| response, err := server.GetConsistencyProof(context.Background(), &testCases[i]) | ||
| if err != nil { | ||
| t.Fatalf("failed to get consistency proof: %v", err) |
There was a problem hiding this comment.
t.Errorf and continue so the loop continues
(OK to defer if you've got a follow-up PR that makes things more table-driven overall)
There was a problem hiding this comment.
Yes, upcoming PR moves the consistency proof tests into one table test.
| testCases := []trillian.GetConsistencyProofRequest{getConsistencyProofRequest7, getConsistencyProofRequest44} | ||
| nodeIDs := [][]storage.NodeID{nodeIdsConsistencySize4ToSize7, {}} | ||
| hashes := [][][]byte{{[]byte("nodehash")}, {}} | ||
| nodes := [][]storage.Node{{{NodeID: stestonly.MustCreateNodeIDForTreeCoords(2, 1, 64), NodeRevision: 3, Hash: []byte("nodehash")}}, {}} |
There was a problem hiding this comment.
Better to put all the per-test case info into a single struct:
vars tests = []struct {
req trillian.GetConsistencyProofRequest
nodeIDs []storage.NodeID
hashes [][]byte
nodes []storage.Node
}{
{
req: &getConsistencyProofRequest7,
nodeIDs: nodeIdsConsistencySize4ToSize7,
hashes: [][]byte{{[]byte("nodehash")},
nodes: []storage.Node{{NodeID: stestonly.MustCreateNodeIDForTreeCoords(2, 1, 64), NodeRevision: 3, Hash: []byte("nodehash")}},
},
{
req: &getConsistencyProofRequest44,
},
}
for _, test := range tests {
...(drop if this is coming in follow-up PR)
There was a problem hiding this comment.
Yeah I've got this done but the branch is based off this one.
This is for additional compatiblity with the older C++ log server code. Needs a corresponding change to CTFE, which will follow:
google/certificate-transparency-go#74