Refactor testings code #191

vqhuy · 2017-10-25T21:16:23Z

Part of #18:

Remove some redundant tests in /protocol level.
Use deterministic tests.
Refactor tests in /merkletree based on work of @liamsi

Note:

Tests in auditlog_test.go and consistencychecks_test.go should be rewritten and integrated with the directory tests, after our APIs are complete.

masomel

I like where this PR is going. My two main comments are (1) many new helper functions need additional documentation/clearer naming to help a reader understand their use in tests, and (2) There's currently still a lot of repeated test infrastructure used across so many of our tests, so I think it would be worth creating a general testutil module in our protocol library which contains these commonly used testing patterns.

masomel · 2017-10-30T17:31:52Z

merkletree/pad_test.go

@@ -407,22 +366,31 @@ func benchPADLookup(b *testing.B, entries uint64) {
 // The STR will get updated every epoch defined by every multiple of
 // `updateEvery`. If `updateEvery > N` createPAD won't update the STR.
 func createPad(N uint64, keyPrefix string, valuePrefix []byte, snapLen uint64,
-	updateEvery uint64) (*PAD, error) {
-	pad, err := NewPAD(TestAd{""}, signKey, vrfPrivKey1, snapLen)
+	afterCreateCB func(pad *PAD),


Can you please add an explanation for the use of callbacks in the createPad documentation?

masomel · 2017-10-30T17:36:59Z

merkletree/proof_test.go

-	if proof.Verify([]byte(key3), val3, m.hash) != nil {
-		t.Error("Proof of inclusion verification failed.")
-	}
+const letterBytes = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"


Please add some documentation for the use of these constants.

masomel · 2017-10-30T17:38:50Z

merkletree/proof_test.go

+var src = rand.NewSource(time.Now().UnixNano())
+
+// Credit: https://stackoverflow.com/a/31832326
+func RandStringBytesMaskImprSrc(n int) string {


Please add documentation for RandStringBytesMaskImprSrc.

masomel · 2017-10-30T17:43:49Z

merkletree/proof_test.go

-	}
+var N uint64 = 3 // number of inclusions.
+
+func setup(t *testing.T) (*MerkleTree, []*mockTest) {


setup seems to fulfill a similar role to other setup-functions like createPad or newTestTree. Can you find a more meaningful name for this function? Maybe something like setupTestKeys? It might be good, at this point, to make sure we use more consistent naming for these different setup functions (e.g. can we just change createPad to newTestPad?)

Renamed to setupTestProofs.

masomel · 2017-10-30T17:48:30Z

merkletree/proof_test.go

+	m := newTestTree(t)
+
+	tuple := []*mockTest{}
+	for i := uint64(0); i < uint64(N); i++ {


It looks like we have several such loops for doing inserts and lookups in our data structures in various tests. I think it might be worth refactoring them into their own helper functions.

Insertion and lookups depends on test cases, so IMO we don't really need to refactor them into smaller functions.

That makes sense. But there still is a lot of repeated test structure, and that's what I'm suggesting we refactor. It will make the tests cleaner and a lot easier to write new ones. I think it would be more valuable and helpful to us and other developers in the future if we could abstract away most of the internals of testing and just had to pass in the test cases we want to test. If it doesn't take too much effort, we should take care of that in this PR.

masomel · 2017-10-30T18:03:52Z

protocol/directory/directory_test.go

-		EndEpoch:   uint64(2)})
+func TestDirectoryKeyLookupInEpochBadEpoch(t *testing.T) {
+	d := NewTestDirectory(t)
+	tests := []struct {


See my comment above about the mockTest struct.

masomel · 2017-10-30T18:23:11Z

protocol/directory/directory_test.go

+		{"bad end epoch", 4, 2, protocol.ErrMalformedMessage},
+		{"out-of-bounds", 6, d.LatestSTR().Epoch, protocol.ErrMalformedMessage},
+	}
+	for _, tt := range tests {


I like this structure for our error condition testing. Since so many of our tests follow this structure, is it possible to further refactor this into a generic function that we can use everywhere instead of having to copy/paste this code everywhere (this will make writing future tests easier, too). Here's what I have in mind: We have a generic TestError function:

func TestError(t *testing.T, tests, testFunction, testRequestType) { for _, tt := range tests { res := testFunction(newTestRequest(testRequestType, tt)) if res.Error != tt.want { t.Errorf("Expect %s for %s", tt.want, tt.name) } } }

Which then gets called as TestError(t, tests, d.GetSTRHistory, protocol.STRType) in TestBadRequestGetSTRHistory. Writing new tests is then only a matter of filling the appropriate test parameter struct and calling TestError with the right parameters. What do you think?

I'm not sure about this since I think it doesn't really save us much. Besides we can sometimes run sub-tests in parallel (see https://golang.org/pkg/testing/#hdr-Subtests_and_Sub_benchmarks) using t.Run.

See #191 (comment). If we have cases in which we run parallel sub-tests, I think we could still write two separate functions, one for parallel testing and one for sequential testing, but still abstract away all the common testing internals.

masomel · 2017-10-30T18:27:53Z

crypto/testutil.go

+}
+
+// StaticSigning returns a static private signing key for _tests_.
+func StaticSigning(t *testing.T) sign.PrivateKey {


I think we should use consistent naming and include the word Test in these function names just to make it absolutely clear to the reader that these are used for testing.

I still prefer to keep this name since it reflects exactly what the function does. Besides, the docstring states it clearly and the parameter requires testing.T so I don't think this will cause any trouble.

My point was a bit more about consistency in naming throughout all of our tests, we should agree on a convention. You're definitely right, if the docstring states the test clearly, the name doesn't need to include Test.

Test is used as prefix only for testcases in _test.go files, so I think this doesn't violate our convention ;)

This is true for the test cases. At the same time, for this function I would have expected a name more like NewTestSigningKey since most other similar testutil helpers follow the NewTest* convention.

What do you think about e06784b?

This definitely works. Thanks!

masomel · 2017-10-30T18:29:09Z

merkletree/testutil.go

+var valuePrefix = []byte("value")
+
+// StaticPAD returns a pad with a static initial STR for _tests_.
+func StaticPAD(t *testing.T, ad AssocData) *PAD {


See my comment above regarding crypto.StaticVRF and such.

masomel · 2017-10-30T18:32:15Z

protocol/tests/client_auditlog_test.go

@@ -0,0 +1 @@
+package tests


I'm assuming you plan to move tests from auditlog_test etc into this module later on, and replace tests like consistencychecks_test with this module?

Yes, for instance, please see https://github.com/coniks-sys/coniks-go/pull/192/files#diff-4b27042e76fbd702dc90612e2d9e9372.

vqhuy · 2017-10-31T00:31:10Z

Thanks for your first round of reviews! I will push documentation/naming changes tomorrow.

…ng_code branch

vqhuy · 2017-12-16T23:28:37Z

This is ready for another review. @masomel

masomel

LGTM! I'll leave a note about further refactoring of testing functionality and test case definition so I don't forget to get back to that.

vqhuy · 2017-12-20T01:02:43Z

Merged in c9491a4.

vqhuy requested review from arlolra, masomel and liamsi October 25, 2017 21:16

vqhuy self-assigned this Oct 25, 2017

vqhuy added maintainability optimization testing labels Oct 25, 2017

vqhuy added this to the 0.2.0 milestone Oct 25, 2017

vqhuy mentioned this pull request Oct 25, 2017

[WIP] Registration should happen in epoch #189

Open

vqhuy force-pushed the refactor-tests branch from dbcbe38 to 31c2259 Compare October 25, 2017 21:18

vqhuy force-pushed the refactor-tests branch 2 times, most recently from a4bc91b to 3bbce60 Compare October 28, 2017 18:48

masomel requested changes Oct 30, 2017

View reviewed changes

masomel modified the milestones: 0.2.0, 0.2.1 Dec 1, 2017

vqhuy modified the milestones: 0.2.1, 0.2.0 Dec 15, 2017

vqhuy force-pushed the refactor-tests branch from 298c703 to f222500 Compare December 15, 2017 23:29

vqhuy added 6 commits December 17, 2017 00:24

Refactor tests (comment some tests which will be rewritten later)

8ef5986

Rewrite merkletree tests based on work of @liamsi from refactor_testi…

46bb1ac

…ng_code branch

Fix comments and refactor more

39ef1c3

Fix naming

fb80a78

Make travis happy

4c0ce31

Update doc.go

64bf084

vqhuy force-pushed the refactor-tests branch from 1644b01 to 64bf084 Compare December 16, 2017 23:24

masomel approved these changes Dec 19, 2017

View reviewed changes

masomel mentioned this pull request Dec 19, 2017

Refactor testing code #18

Open

vqhuy merged commit 5348e1e into master Dec 20, 2017

vqhuy deleted the refactor-tests branch December 20, 2017 01:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor testings code #191

Refactor testings code #191

vqhuy commented Oct 25, 2017 •

edited

Loading

masomel left a comment

masomel Oct 30, 2017

vqhuy Dec 15, 2017

masomel Oct 30, 2017

masomel Oct 30, 2017

masomel Oct 30, 2017

vqhuy Dec 15, 2017 •

edited

Loading

masomel Oct 30, 2017

vqhuy Oct 31, 2017

masomel Oct 31, 2017

masomel Oct 30, 2017

masomel Oct 30, 2017 •

edited

Loading

vqhuy Oct 31, 2017

masomel Oct 31, 2017 •

edited

Loading

masomel Oct 30, 2017

vqhuy Dec 15, 2017

masomel Dec 15, 2017

vqhuy Dec 15, 2017

masomel Dec 15, 2017 •

edited

Loading

vqhuy Dec 15, 2017

masomel Dec 15, 2017

masomel Oct 30, 2017

vqhuy Dec 15, 2017

masomel Oct 30, 2017 •

edited

Loading

vqhuy Oct 31, 2017

vqhuy commented Oct 31, 2017

vqhuy commented Dec 16, 2017

masomel left a comment

vqhuy commented Dec 20, 2017

Refactor testings code #191

Refactor testings code #191

Conversation

vqhuy commented Oct 25, 2017 • edited Loading

masomel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vqhuy Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masomel Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masomel Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masomel Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masomel Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vqhuy commented Oct 31, 2017

vqhuy commented Dec 16, 2017

masomel left a comment

Choose a reason for hiding this comment

vqhuy commented Dec 20, 2017

vqhuy commented Oct 25, 2017 •

edited

Loading

vqhuy Dec 15, 2017 •

edited

Loading

masomel Oct 30, 2017 •

edited

Loading

masomel Oct 31, 2017 •

edited

Loading

masomel Dec 15, 2017 •

edited

Loading

masomel Oct 30, 2017 •

edited

Loading