Strengthen CreateDomain #969
Conversation
* Handle the AlreadyExists Case * Handle a mid-tree creation failure by deleting other tree. * Add unit testing with mocks.
Codecov Report
@@ Coverage Diff @@
## master #969 +/- ##
==========================================
+ Coverage 46.92% 47.34% +0.41%
==========================================
Files 28 28
Lines 2018 2034 +16
==========================================
+ Hits 947 963 +16
- Misses 892 894 +2
+ Partials 179 177 -2
Continue to review full report at Codecov.
|
|
More unit tests for Key Transparency. PTAL |
|
PTAL |
| } | ||
|
|
||
| type miniEnv struct { | ||
| s *testonly.MockServer |
There was a problem hiding this comment.
's' is a bit ambiguous given that it's a MockServer and just below you have an actual Server.
How about 'ms' or 'mockSrv'?
There was a problem hiding this comment.
And below you call something associated with it stopFakeServer.
So is it a fake server?
There was a problem hiding this comment.
I'll rename to ms. It's a mock server.
core/adminserver/admin_server.go
Outdated
| if _, delErr := s.logAdmin.DeleteTree(ctx, &tpb.DeleteTreeRequest{TreeId: logTree.TreeId}); delErr != nil { | ||
| return nil, status.Errorf(codes.Internal, "adminserver: CreateAndInitTree(map): %v, DeleteTree(%v): %v ", err, logTree.TreeId, delErr) | ||
| } | ||
| if _, delErr := s.mapAdmin.DeleteTree(ctx, &tpb.DeleteTreeRequest{TreeId: mapTree.TreeId}); delErr != nil { |
There was a problem hiding this comment.
I wonder if in this case you ought to try deleting both the log and the map, and return a single error from which ever one fails?
i.e. if deleting the log fails, note that but also try deleting the map.
There was a problem hiding this comment.
Great idea. Fewer if statements is always better.
| e.s.Admin.EXPECT().CreateTree(gomock.Any(), gomock.Any()).Return(&tpb.Tree{TreeType: tpb.TreeType_MAP}, nil) | ||
| e.s.Map.EXPECT().InitMap(gomock.Any(), gomock.Any()).Return(&tpb.InitMapResponse{}, fmt.Errorf("init map failure")).MinTimes(1) | ||
| e.s.Map.EXPECT().InitMap(gomock.Any(), gomock.Any()).Return(&tpb.InitMapResponse{}, nil) | ||
| e.s.Map.EXPECT().GetSignedMapRootByRevision(gomock.Any(), gomock.Any()).Return(&tpb.GetSignedMapRootResponse{}, fmt.Errorf("not found")).MinTimes(1) |
There was a problem hiding this comment.
The indentation looks screwy here but it's probably Github's fault, I can't tell.
There was a problem hiding this comment.
There's some long lines, I'll wrap them.
| e.s.Log.EXPECT().GetLatestSignedLogRoot(gomock.Any(), gomock.Any()).Return(&tpb.GetLatestSignedLogRootResponse{}, nil) | ||
| e.s.Admin.EXPECT().CreateTree(gomock.Any(), gomock.Any()).Return(&tpb.Tree{TreeType: tpb.TreeType_MAP}, nil) | ||
| e.s.Map.EXPECT().InitMap(gomock.Any(), gomock.Any()).Return(&tpb.InitMapResponse{}, nil) | ||
| e.s.Map.EXPECT().GetSignedMapRootByRevision(gomock.Any(), gomock.Any()).Return(&tpb.GetSignedMapRootResponse{}, nil).MinTimes(1) |
| }, | ||
| }, | ||
| { | ||
| desc: "init fails", |
There was a problem hiding this comment.
the desc doesn't really describe what you're testing.
In the circumstance where init fails you try to delete the log & the map.
That's what the test is checking.
…y into test/listhistory * 'test/listhistory' of github.com:gdbelvin/keytransparency: Use DomainID rather than mapID during authorization (google#970) Strengthen CreateDomain (google#969)
3 participants
This PR addresses some mid-CreateDomain issues that were cropping up in production and seeks to increase test cases coverage.