Add UnitTest for PaginateHistory #968

gdbelvin · 2018-05-01T13:26:35Z

This PR adds a long overdue unit test and unittesting framework for client operations.

Included:

A mock KeyTransparency server
An interface for mocking out the verifier
Removed a global variable pageSize. It's now a client variable.

codecov-io · 2018-05-01T13:48:06Z

Codecov Report

Merging #968 into master will increase coverage by 2.21%.
The diff coverage is 80.76%.

@@            Coverage Diff             @@
##           master     #968      +/-   ##
==========================================
+ Coverage   47.34%   49.55%   +2.21%     
==========================================
  Files          28       28              
  Lines        2034     2030       -4     
==========================================
+ Hits          963     1006      +43     
+ Misses        894      843      -51     
- Partials      177      181       +4

Impacted Files	Coverage Δ
core/client/mutations.go	`0% <0%> (ø)`	⬆️
core/keyserver/keyserver.go	`33.16% <100%> (ø)`	⬆️
core/client/get_and_verify.go	`28.94% <100%> (+28.94%)`	⬆️
core/client/verify.go	`57.74% <81.81%> (ø)`	⬆️
core/client/client.go	`28.07% <85.71%> (+12.72%)`	⬆️

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 29dfa03...0f6b485. Read the comment docs.

gdbelvin · 2018-05-02T09:23:27Z

More unittests for Key Transparency. PTAL

pav-kv · 2018-05-02T09:32:02Z

core/client/client.go


+// Verifier is used to verify specific outputs from Key Transparency.
+type Verifier interface {
+	// Index computes the index from a VRF proof.


What is VRF?

VRF means Verifiable Random Function

KT has a VRF implementation in core/crypto/vrf/

Ack, thank you.

pav-kv

Some nits.

pav-kv · 2018-05-02T09:33:14Z

core/client/client.go

+type Verifier interface {
+	// Index computes the index from a VRF proof.
+	Index(vrfProof []byte, domainID, appID, userID string) ([]byte, error)
+	// VerifyGetEntryResponse verifies everything about a GetEntryResponse:


pav-kv · 2018-05-02T09:42:21Z

core/client/client.go


+// Verifier is used to verify specific outputs from Key Transparency.
+type Verifier interface {
+	// Index computes the index from a VRF proof.


Ack, thank you.

pav-kv · 2018-05-02T09:45:06Z

core/client/client.go

-	epochsWant := end - start + 1
-	for epochsReceived < epochsWant {
-		count := min(int32((end-start)+1), pageSize)
+	epochsWant := int(end - start + 1)


I am slightly nervous about this shrinking cast. How do you feel about doing the opposite - comparing int64s?

Good idea. It produces a slightly nasty set of casts below, but it's probably safer.

pav-kv · 2018-05-02T09:55:28Z

core/testutil/mockserver.go

+	Addr   string
+}
+
+// NewMockKT returns a new mock Key Transparency server listening on a random port .


nit: remove space before period

pav-kv · 2018-05-02T09:59:29Z

core/testutil/mockserver.go

+
+// NewMockKT returns a new mock Key Transparency server listening on a random port .
+// Returns the started server, the listener it's using for connection and a
+// close function that must be defer-called on the scope the server is meant to


I'd drop the "defer-" bit because it doesn't look like a must (could simply call the function directly?), likewise the "scope" detail:
... that must be called to stop the server.

pav-kv · 2018-05-02T10:10:57Z

core/testutil/mockserver.go

+}
+
+// NewMockKT returns a new mock Key Transparency server listening on a random port .
+// Returns the started server, the listener it's using for connection and a


The pure Listener object is not there, only the server wrapper.
Returns the started server, a client connected to it, and a close ...

pav-kv · 2018-05-02T10:20:37Z

core/client/verify.go

-	mapVerifier *tclient.MapVerifier
-	logVerifier *tclient.LogVerifier
+// Verify is a client helper library for verifying request and responses.
+type Verify struct {


Is there a way to name this with a noun? Some ideas:

Call this Verifier and the interface - IVerifier. Not sure whether Go naming conventions allows this.

Verification(Client), VerifierImpl, VerifiedClient ...

I struggled with the naming conventions too.
is RealVerifier appropriate? The interface exists so we can have a FakeVerifier

pav-kv · 2018-05-02T10:24:58Z

core/client/client_test.go

+			}
+		})
+	}
+


nit: drop empty line?

pav-kv · 2018-05-02T10:26:36Z

core/client/client_test.go

+					Start:    r.wantStart,
+					PageSize: r.wantSize,
+				})).
+					Return(&pb.ListEntryHistoryResponse{


nit: maybe merge this line with the previous one?

That does look better.

pav-kv · 2018-05-02T14:45:54Z

core/client/client.go

-		count := min(int32((end-start)+1), pageSize)
+	for int64(len(allProfiles)) < epochsWant {
+		log.Printf("pageSize: %v", c.pageSize)
+		count := min(int32(epochsWant-int64(len(allProfiles))), c.pageSize)


Hm, this bit is just as problematic as the previous version due to shrinking cast. Probably making a min function for int64 is too much hassle. Instead, how about rewinding this back, and simply checking end-start+1 for an overflow before casting it down to int32 in line 175?

Making min accept int64 is actually the better solution. It defers the downcast till the end when we expect the value to be small.

Oh, I agree. Wasn't aware that the min function was used only here.

pav-kv · 2018-05-02T15:50:59Z

This now LGTM.
@taknira do you want to do a final proofread?

One general question. I noticed you use both Vlog and glog.V for verbose logging. I wonder why you don't use just glog.

pav-kv · 2018-05-02T15:52:41Z

core/client/verify.go

-	vrf         vrf.PublicKey
-	mapVerifier *tclient.MapVerifier
-	logVerifier *tclient.LogVerifier
+// RealVerifier is a client helper library for verifying request and responses.


Maybe you could make a note that this implements Verifier? That would explain the seemingly unclear Real bit.

gdbelvin · 2018-05-04T13:29:52Z

Vlog provides visual logging on the command line during interactive operations.
It is separate from glog which provides debugging and general info.

Kat, would you mind taking a look?

taknira · 2018-05-08T12:02:20Z

core/client/client.go


+// Verifier is used to verify specific outputs from Key Transparency.
+type Verifier interface {
+	// Index computes the index from a VRF proof.


Index computes the index of what? Add to the comment to make it really clear :)

Index computes the index of an appID, userID pair from a VRF proof, obtained from the server.

taknira · 2018-05-08T12:06:21Z

core/client/client.go

+	// VerifyGetEntryResponse verifies everything about a GetEntryResponse.
+	VerifyGetEntryResponse(ctx context.Context, domainID, appID, userID string, trusted types.LogRootV1, in *pb.GetEntryResponse) (*types.MapRootV1, *types.LogRootV1, error)
+	// VerifyEpoch verifies that epoch is correctly signed and included in the append only log.
+	// VerifyEpoch also verifies that epoch.LogRoot is consistent with the last trusted SignedLogRoot.


Did the *pb.Epoch parameter used to be called 'epoch' or something? Maybe capitalize in these comments to make it clear you're talking about the type rather than the variable name - 'VerifyEpoch verifies that the Epoch is correctly...', 'VerifyEpoch also verifies that Epoch.LogRoot is ....'

I'll just rename in to epoch :-)

…y into test/listhistory * 'test/listhistory' of github.com:gdbelvin/keytransparency: Use DomainID rather than mapID during authorization (google#970) Strengthen CreateDomain (google#969)

taknira · 2018-05-08T14:56:38Z

core/client/client_test.go

+				}, nil)
+			}
+
+			if _, _, err = c.PaginateHistory(ctx, appID, userID, tc.start, tc.end); err != tc.wantErr {


This method of testing PaginateHistory seems odd, given you could test based on state - i.e. just test PaginateHistory based on its inputs and outputs, rather than checking that it's calling the correct server endpoints with the right arguments.... In fact, by doing the way you currently are explicitly doesn't test some of the logic within PaginateHistory outside of what it sends/receives to/from the server.

In addition to this, (and as discussed separately from this PR,) use of gomock (and mocks in general) tends to be discouraged from a readability standpoint, preferring real objects or fakes.

If you were to test PaginateHistory based on inputs and outputs (and compare them to expected outputs) you could just fake the one rpc it calls (ListEntryHistory is it?), which would remove the need for mocking, making the test code much simpler. (Although incidentally it looks like the PaginateHistory code doesn't call that directly, but instead calls client.VerifiedListHistory, which then calls ListEntryHistory - I couldn't find a test for VerifiedListHistory, so using this method of testing could also be appropriate for that function too.)

This method of testing would be specifically testing the PaginateHistory code, and assuming the underlying functions that it calls work as expected.

…y into test/listhistory * 'test/listhistory' of github.com:gdbelvin/keytransparency: Missing masterPassword input in post.go (google#971)

gdbelvin · 2018-05-11T13:52:36Z

Yay! the mocks have been removed. Thanks @taknira for helping keep up the code excellence.
PTAL.

taknira

Looks good, and easier to follow now :)

taknira · 2018-05-11T14:08:41Z

core/client/client_test.go

+	}
+	s, stop, err := testutil.NewFakeKT(srv)
+	if err != nil {
+		t.Fatalf("NewMockKT(): %v", err)


It's not a mock anymore :P

taknira · 2018-05-11T14:12:31Z

core/client/client_test.go

 			}
+			if got, want := values, tc.wantValues; !reflect.DeepEqual(got, want) {
+				t.Errorf("PaginateHistory().values: \n%#v, want \n%#v, diff: \n%v",
+					got, want, pretty.Compare(got, want))


(Optional) Is there any chance at all that pretty.Compare and reflect.DeepEqual would return different results? Might be worth using pretty.Compare for the initial comparison too, and checking the diff is empty of something? Completely up to you though, as the two funcs will likely be in agreement the majority of the time. Just a musing :)

If the client always asks for the maximum amount from the server, 1. The total number of requests will be smaller. 2. The logic for determining the number of items to return is simpler.

* master: Move authentication to grpc interceptor (google#973) Add UnitTest for PaginateHistory (google#968) Remove unused `UserProfile` message (google#972) Update default paths (google#910)

gdbelvin added 8 commits May 1, 2018 12:38

Generate KT mocks

bb4355a

Rename Verifier->Verify to make way for interface

853b044

Create KT verifier interface

095305b

Rename ListHistory to PaginateHistory for clarity

917d15c

Move pageSize to Client variable

3377a66

wip: PaginateHistoryTest

517d3fd

mockserver implementaiton

4d0975e

Add unittest for PaginateHistory

5efe977

gdbelvin force-pushed the test/listhistory branch from e43281f to 5efe977 Compare May 1, 2018 13:26

Add pageSize test

9c5d9a8

gdbelvin requested review from pav-kv and taknira May 1, 2018 13:48

gdbelvin added this to the Productionize milestone May 1, 2018

pav-kv reviewed May 2, 2018

View reviewed changes

Reviewer edits

0affbc8

pav-kv reviewed May 2, 2018

View reviewed changes

Avoid downcast

d6864cb

pav-kv reviewed May 2, 2018

View reviewed changes

Add implements comment

2957c43

gdbelvin added 2 commits May 4, 2018 17:16

Merge branch 'master' into test/listhistory

7c2a081

Merge branch 'master' into test/listhistory

5817c4c

taknira reviewed May 8, 2018

View reviewed changes

gdbelvin and others added 3 commits May 8, 2018 13:38

Clearer interface documentation

22a7c65

Merge branch 'test/listhistory' of github.com:gdbelvin/keytransparenc…

1b3bedd

…y into test/listhistory * 'test/listhistory' of github.com:gdbelvin/keytransparency: Use DomainID rather than mapID during authorization (google#970) Strengthen CreateDomain (google#969)

Merge branch 'master' into test/listhistory

24893bd

taknira reviewed May 8, 2018

View reviewed changes

pav-kv mentioned this pull request May 11, 2018

Use glog instead of standard log in tools and Fetcher google/certificate-transparency-go#238

Merged

gdbelvin added 3 commits May 11, 2018 14:48

Test outputs rather than interactions

6e0f45d

Remove mocks

a05daf8

Merge branch 'test/listhistory' of github.com:gdbelvin/keytransparenc…

bcd74f9

…y into test/listhistory * 'test/listhistory' of github.com:gdbelvin/keytransparency: Missing masterPassword input in post.go (google#971)

taknira approved these changes May 11, 2018

View reviewed changes

gdbelvin force-pushed the test/listhistory branch from d0cb53c to cdee1f7 Compare May 11, 2018 18:06

Remove client page size

ff25cbf

If the client always asks for the maximum amount from the server, 1. The total number of requests will be smaller. 2. The logic for determining the number of items to return is simpler.

gdbelvin force-pushed the test/listhistory branch from cdee1f7 to ff25cbf Compare May 11, 2018 18:12

gdbelvin and others added 2 commits May 11, 2018 19:13

Merge branch 'master' into test/listhistory

25c8b95

remove unused functions

0f6b485

gdbelvin merged commit ae6690b into google:master May 11, 2018

gdbelvin deleted the test/listhistory branch May 11, 2018 18:46

Add UnitTest for PaginateHistory #968

Add UnitTest for PaginateHistory #968

Uh oh!

Conversation

gdbelvin commented May 1, 2018

Uh oh!

codecov-io commented May 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gdbelvin commented May 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pav-kv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pav-kv commented May 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gdbelvin commented May 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gdbelvin commented May 11, 2018

Uh oh!

taknira left a comment

codecov-io commented May 1, 2018 •

edited

Loading