Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Queue integration test #1364

Merged
merged 10 commits into from Oct 8, 2019

Conversation

@gdbelvin
Copy link
Collaborator

commented Oct 3, 2019

By moving the queue tests from impl/ to core/integration we can ensure that any queue implementation meets the API contract.

@gdbelvin gdbelvin requested a review from google/keytransparency as a code owner Oct 3, 2019
@googlebot googlebot added the cla: yes label Oct 3, 2019
@gdbelvin gdbelvin changed the title Create queue integration test Queue integration test Oct 3, 2019
@gdbelvin gdbelvin requested review from mhutchinson and pavelkalinnikov Oct 3, 2019
core/integration/storagetest/queue.go Outdated Show resolved Hide resolved
core/integration/storagetest/queue.go Outdated Show resolved Hide resolved
@gdbelvin gdbelvin force-pushed the gdbelvin:mutation1 branch 2 times, most recently from 2288370 to 1f61e64 Oct 3, 2019
@codecov

This comment has been minimized.

Copy link

commented Oct 3, 2019

Codecov Report

Merging #1364 into master will increase coverage by 0.13%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1364      +/-   ##
==========================================
+ Coverage   65.44%   65.58%   +0.13%     
==========================================
  Files          48       48              
  Lines        3869     3876       +7     
==========================================
+ Hits         2532     2542      +10     
+ Misses        955      952       -3     
  Partials      382      382
Impacted Files Coverage Δ
core/keyserver/keyserver.go 61.86% <ø> (ø) ⬆️
impl/sql/mutationstorage/queue.go 71.05% <50%> (-2.75%) ⬇️
core/integration/client_tests.go 85.24% <0%> (+0.46%) ⬆️
core/sequencer/server.go 72.01% <0%> (+1.02%) ⬆️
core/sequencer/trillian_client.go 53.01% <0%> (+1.2%) ⬆️
impl/sql/directory/storage.go 69.17% <0%> (+1.5%) ⬆️
impl/sql/mutationstorage/mutations.go 72.5% <0%> (+2.5%) ⬆️

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 7526357...5490bea. Read the comment docs.

@gdbelvin gdbelvin force-pushed the gdbelvin:mutation1 branch from 1f61e64 to a03b5f1 Oct 3, 2019
@gdbelvin

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 3, 2019

At the moment these test suites might look funny because they only have one test, but future PRs will add to them

@gdbelvin gdbelvin requested a review from mhutchinson Oct 3, 2019
Copy link
Contributor

left a comment

This is making a little more sense. I'm still being heavy handed with the comments for which I apologize, but given that this is going to be our conformance tests I think clarity is important here - people investigating failures here are highly likely to be not domain experts on the deeper depths of what caused the failure.

core/integration/storagetest/batch.go Outdated Show resolved Hide resolved
core/integration/storagetest/mutation_log.go Outdated Show resolved Hide resolved
core/integration/storagetest/mutation_log.go Outdated Show resolved Hide resolved
core/integration/storagetest/mutation_log.go Outdated Show resolved Hide resolved
core/integration/storagetest/mutation_log.go Outdated Show resolved Hide resolved
core/integration/storagetest/mutation_log_admin.go Outdated Show resolved Hide resolved
core/integration/storagetest/mutation_log_admin.go Outdated Show resolved Hide resolved
core/integration/storagetest/mutation_log_admin.go Outdated Show resolved Hide resolved
gdbelvin added 3 commits Oct 3, 2019
gdbelvin added 2 commits Oct 4, 2019
* master:
  Bump docker images to go1.13 (#1362)
@gdbelvin gdbelvin requested a review from mhutchinson Oct 7, 2019
@gdbelvin

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 7, 2019

PTAL

Copy link
Contributor

left a comment

nits

impl/sql/mutationstorage/queue_test.go Outdated Show resolved Hide resolved
core/integration/storagetest/mutation_log.go Outdated Show resolved Hide resolved
core/integration/storagetest/mutation_log.go Outdated Show resolved Hide resolved
core/integration/storagetest/mutation_log.go Outdated Show resolved Hide resolved
core/integration/storagetest/mutation_log_admin.go Outdated Show resolved Hide resolved
core/integration/storagetest/mutation_log_admin.go Outdated Show resolved Hide resolved
core/keyserver/keyserver.go Outdated Show resolved Hide resolved
core/keyserver/keyserver.go Show resolved Hide resolved
core/keyserver/keyserver.go Outdated Show resolved Hide resolved
gdbelvin added 2 commits Oct 7, 2019
* master:
  Hermetic set of linters (#1365)
  Remove confusing -tags=mysql build flag (#1363)
@gdbelvin gdbelvin force-pushed the gdbelvin:mutation1 branch from 10616e7 to 3454461 Oct 7, 2019
@gdbelvin

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 7, 2019

Thanks for being so thorough. I took another pass.

Copy link
Contributor

left a comment

LGTM % final nits

core/integration/storagetest/mutation_logs_admin.go Outdated Show resolved Hide resolved
core/keyserver/keyserver.go Outdated Show resolved Hide resolved
Copy link
Contributor

left a comment

Looks good to me. Thanks for your time on this.

I have some concerns about the API batch size argument. It would be surprising to me as a client integrating to call something with a batch size argument and potentially get a lot more results than that max. But that's orthogonal to this PR.

@gdbelvin

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 8, 2019

Agreed that the API could use a revisit.
From a principle of least surprise point of view, you're absolutely right, getting a non-deterministic amount of data back is weird. An API that requests proper units of batches would be much better. But from the perspective of trying to control the throughput of the system, batches of non-deterministic size are also a problem.

@gdbelvin gdbelvin merged commit cc70d02 into google:master Oct 8, 2019
3 checks passed
3 checks passed
GolangCI No issues found!
Details
Travis CI - Pull Request Build Passed
Details
cla/google All necessary CLAs are signed
@gdbelvin gdbelvin deleted the gdbelvin:mutation1 branch Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.