Merge cmd/gen-test-vectors and integration/client_tests.go#1194
Merge cmd/gen-test-vectors and integration/client_tests.go#1194gdbelvin merged 7 commits intogoogle:masterfrom
Conversation
Zyqsempai
force-pushed
the
merge-cmd-and-integration-tests
branch
from
db17077 to
becda60
Compare
|
@gdbelvin Can't say that this is the best implementation. Coz all the tests are pretty messed up and also i wasn't able to test it because of docker mysql issue. |
Codecov Report
@@ Coverage Diff @@
## master #1194 +/- ##
==========================================
- Coverage 65.02% 62.78% -2.24%
==========================================
Files 46 46
Lines 3342 3461 +119
==========================================
Hits 2173 2173
- Misses 811 930 +119
Partials 358 358
Continue to review full report at Codecov.
|
core/integration/client_tests.go
Outdated
| } | ||
|
|
||
| // GenerateTestVectors verifies set/get semantics. | ||
| // TODO(gbelvin): Migrate into core/integration/client_tests to avoid code rot. |
There was a problem hiding this comment.
Thanks for moving this! You can now delete this TODO
| @@ -31,7 +31,6 @@ import ( | |||
|
|
|||
There was a problem hiding this comment.
The separation of impl/integration from core/integration isn't documented, so let me explain here:
The layout of the code is designed to make using Key Transparency in an enterprise environment easy. Everything that might be implementation specific is in the impl directory. In order for the core directory to operate independently of implementation details, it must not have any references to impl. This is why the unit tests that rely only on the interfaces can remain in core, while the actual test that supplies the dependencies lives in impl.
Please keep impl/integration/env.go in impl.
There was a problem hiding this comment.
I moved it there because of the cycle dependecies, so it cannot be solved without moving something, or env.go or all the tests to impl/integration.
There was a problem hiding this comment.
What was the cycle in particular?
There was a problem hiding this comment.
cmd/gen-test-vectors has call to impl/integration
and impl/integration has call to integration/client_tests.go
if we merge cmd/gen-test-vectors into integration/client_tests.go we get cycle.
There was a problem hiding this comment.
I think the link between cmd/gen-test-vectors to impl/integration can be split:
main()incmd/gen-test-vectorsis nowimpl/integration/main_test.goGenerateTestVectorsused to depend onimpl/integration/env.go, but this can be swapped withcore/integration/env.go
There are a few other references to impl in GenerateTestVectors which I've annotated with fixes for.
There was a problem hiding this comment.
Ok, i'l try to do it in this way.
impl/integration/main_test.go
Outdated
| if err != nil { | ||
| t.Fatalf("Could not create Env: %v", err) | ||
| } | ||
| if *generate { |
There was a problem hiding this comment.
In this configuration, GenerateTestVectors is it's own unit test and as such probably belongs outside the for loop, else we will generate the test vectors len(AllTests) times.
There was a problem hiding this comment.
Yeah, but anyway we run NewEnv(ctx) for each loop cycle, maybe we should move it outside too?
gdbelvin
left a comment
gdbelvin
left a comment
There was a problem hiding this comment.
Please keep impl/integration/env.go in place to maintain the invariant of no references to impl code inside of core
core/integration/client_tests.go
Outdated
| "github.com/google/keytransparency/core/testutil" | ||
|
|
||
| "github.com/golang/protobuf/jsonpb" | ||
| "github.com/google/martian/log" |
There was a problem hiding this comment.
Should this import be "log" or "glog"?
core/integration/client_tests.go
Outdated
| desc: "empty_alice", | ||
| wantProfile: nil, | ||
| setProfile: []byte("alice-key1"), | ||
| ctx: authentication.WithOutgoingFakeAuth(ctx, "alice"), |
There was a problem hiding this comment.
TestEmptyGetAndUpdate uses the following to avoid an import reference to impl.
| ctx: authentication.WithOutgoingFakeAuth(ctx, "alice"), | |
| opts: env.CallOpts("alice"), |
Zyqsempai
force-pushed
the
merge-cmd-and-integration-tests
branch
from
cf7f3ee to
8600e5c
Compare
|
@gdbelvin Looks like done, but i have doubts, since i wasn't able to test it and i have strong feeling that i am confused a little bit;)) |
gdbelvin
left a comment
gdbelvin
left a comment
There was a problem hiding this comment.
Thanks looks good, thanks!
* 'metric' of github.com:gdbelvin/keytransparency: Merge cmd/gen-test-vectors and integration/client_tests.go (google#1194)
* 'metric' of github.com:gdbelvin/keytransparency: Merge cmd/gen-test-vectors and integration/client_tests.go (google#1194)
3 participants
Merged
cmd/gen-test-vectorsandintegration/client_tests.goChanged generate command
go test ../../impl/integration --generateAdded bool
gnerateflag toimpl/integration/main_test.goI think we need also change the test command in TravisCI.