Refactor get_tree_public_key test helper methods #1247
Conversation
Moves `setFlag` and `createLog` to `setup.SetFlag` and `setup.CreateLog`, which would make it easier to reuse those helpers in other tests as well.
Codecov Report
@@ Coverage Diff @@
## master #1247 +/- ##
======================================
Coverage 58.1% 58.1%
======================================
Files 113 113
Lines 9573 9573
======================================
Hits 5562 5562
Misses 3439 3439
Partials 572 572Continue to review full report at Codecov.
|
| ) | ||
|
|
||
| // SetFlag updates a flag value, failing the test if something goes wrong. | ||
| func SetFlag(t *testing.T, name, value string) { |
There was a problem hiding this comment.
Do you want to revert the flag after the test is finished?
There was a problem hiding this comment.
Reverting is done in bulk using defer flagsaver.Save().Restore()
testonly/setup/trees.go
Outdated
| "github.com/golang/protobuf/ptypes" | ||
| "github.com/google/trillian" | ||
| "github.com/google/trillian/client" | ||
| ktestonly "github.com/google/trillian/crypto/keys/testonly" |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
testonly/setup/trees.go
Outdated
| // | ||
| // It optionally allows specifying an overrides argument to override some | ||
| // properties of the tree to be created. | ||
| func CreateLog(t *testing.T, logEnv *integration.LogEnv, maxRootDuration time.Duration) *trillian.Tree { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
testonly/setup/trees.go
Outdated
| func CreateLog(t *testing.T, logEnv *integration.LogEnv, maxRootDuration time.Duration) *trillian.Tree { | ||
| t.Helper() | ||
|
|
||
| ctx := context.Background() |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
testonly/setup/trees.go
Outdated
| // | ||
| // It optionally allows specifying an overrides argument to override some | ||
| // properties of the tree to be created. | ||
| func CreateLog(t *testing.T, logEnv *integration.LogEnv, maxRootDuration time.Duration) *trillian.Tree { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
It looks like the test helpers are fairly test specific.
Consider moving the test helper functions into the test files themselves. See the map_test's
newTreeWithHasher helper function.
|
The reason I'm putting those helper functions in a separate module is that I want to use them in multiple test files. They are currently being used in |
testonly/setup/trees.go
Outdated
| // It requires the caller to specify a maxRootDuration for the Trillian tree, | ||
| // which will be used as the interval after which a new signed root is produced | ||
| // despite no submissions. | ||
| func CreateLog(ctx context.Context, t *testing.T, logClient trillian.TrillianLogClient, adminClient trillian.TrillianAdminClient, maxRootDuration time.Duration) *trillian.Tree { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
testonly/setup/trees.go
Outdated
| // It requires the caller to specify a maxRootDuration for the Trillian tree, | ||
| // which will be used as the interval after which a new signed root is produced | ||
| // despite no submissions. | ||
| func CreateLog(ctx context.Context, t *testing.T, logClient trillian.TrillianLogClient, adminClient trillian.TrillianAdminClient, maxRootDuration time.Duration) *trillian.Tree { |
There was a problem hiding this comment.
Existing tests call CreateAndInitTree directly. Please follow that convention in your tests.
Example
It looks like the main utility of this function is a helper for creating a trillian.Tree.
a) There are some convenient, prepopulated test trees that you can use, copy, and modify in your tests.
b) If you prefer, this function could be reduced to CreateTreeRequest with a suite of With* modifiers.
What do you think?
There was a problem hiding this comment.
I didn't know about CreateAndInitTree. I'll switch to using it.
Thank you.
|
It looks like there's an easier way to get test trees setup. What do you think about using that approach? We'd prefer to reduce code duplication. |
|
Switched to using |
maljub01
changed the title
|
Thanks for the review. All checks have passed, but I don't have the write permissions needed to merge this. |
setFlagtosetup.SetFlag.CreateLogin favor of using the existingCreateAndInitTree.