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

Use and test the same MySQL connection that main.go uses #1370

Merged
merged 9 commits into from Oct 25, 2019

Conversation

@gdbelvin
Copy link
Collaborator

gdbelvin commented Oct 25, 2019

"Test what we use"

By testing against a real MySQL database instead of SQLite, I discovered a subtle implementation difference in the behavior of AffectedRows, requiring a MySQL driver flag ClientFoundRows to be set to achieve the desired behavior

@gdbelvin gdbelvin requested a review from google/keytransparency as a code owner Oct 25, 2019
@googlebot googlebot added the cla: yes label Oct 25, 2019
@gdbelvin gdbelvin added the bug label Oct 25, 2019
@gdbelvin gdbelvin requested review from mhutchinson and pavelkalinnikov Oct 25, 2019
@gdbelvin gdbelvin force-pushed the gdbelvin:mysql branch from 8f063e6 to f625797 Oct 25, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 25, 2019

Codecov Report

Merging #1370 into master will decrease coverage by 0.14%.
The diff coverage is 62.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1370      +/-   ##
==========================================
- Coverage   65.57%   65.43%   -0.15%     
==========================================
  Files          49       50       +1     
  Lines        3881     3897      +16     
==========================================
+ Hits         2545     2550       +5     
- Misses        948      953       +5     
- Partials      388      394       +6
Impacted Files Coverage Δ
impl/integration/env.go 76.14% <ø> (ø) ⬆️
impl/sql/testdb/testdb.go 62.5% <62.5%> (ø)
core/sequencer/trillian_client.go 58.57% <0%> (-2.86%) ⬇️
core/integration/client_tests.go 84.77% <0%> (-0.47%) ⬇️
core/sequencer/server.go 70.74% <0%> (-0.35%) ⬇️

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 575344f...2c68db8. Read the comment docs.

@dep dep bot added the dependent label Oct 25, 2019
gdbelvin added 3 commits Oct 25, 2019
Test with the same database we use in prod.

- Each test gets its own database
- Drop test databases after they are done.
By using the same database open logic everywhere, we can test it
@gdbelvin gdbelvin force-pushed the gdbelvin:mysql branch from f625797 to 1c00c83 Oct 25, 2019
@dep dep bot removed the dependent label Oct 25, 2019
Copy link
Contributor

pavelkalinnikov left a comment

Looks good, but nits.

impl/sql/open.go Outdated Show resolved Hide resolved
impl/sql/open.go Show resolved Hide resolved
impl/sql/testdb/testdb.go Show resolved Hide resolved
impl/sql/testdb/testdb.go Outdated Show resolved Hide resolved
impl/sql/testdb/testdb.go Outdated Show resolved Hide resolved
core/integration/storagetest/mutation_logs_admin.go Outdated Show resolved Hide resolved
core/integration/storagetest/mutation_logs_admin.go Outdated Show resolved Hide resolved
core/integration/storagetest/mutation_logs.go Outdated Show resolved Hide resolved
core/integration/storagetest/batch.go Outdated Show resolved Hide resolved
gdbelvin added 4 commits Oct 25, 2019
core/integration/storagetest/batch.go Outdated Show resolved Hide resolved
core/integration/storagetest/mutation_logs.go Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
impl/sql/directory/storage_test.go Outdated Show resolved Hide resolved
impl/sql/testdb/testdb.go Show resolved Hide resolved
core/integration/storagetest/batch.go Show resolved Hide resolved
core/integration/storagetest/batch.go Show resolved Hide resolved
core/integration/storagetest/batch.go Show resolved Hide resolved
impl/sql/directory/storage_test.go Outdated Show resolved Hide resolved
impl/sql/directory/storage_test.go Outdated Show resolved Hide resolved
impl/sql/directory/storage_test.go Outdated Show resolved Hide resolved
@gdbelvin gdbelvin force-pushed the gdbelvin:mysql branch from abc6c30 to 1127409 Oct 25, 2019
@gdbelvin gdbelvin force-pushed the gdbelvin:mysql branch from 1127409 to 2c68db8 Oct 25, 2019
@gdbelvin gdbelvin merged commit 90e1d58 into google:master Oct 25, 2019
3 of 5 checks passed
3 of 5 checks passed
codecov/patch 62.5% of diff hit (target 65.57%)
Details
codecov/project 65.43% (-0.15%) compared to 575344f
Details
GolangCI No issues found!
Details
Travis CI - Pull Request Build Passed
Details
cla/google All necessary CLAs are signed
@gdbelvin gdbelvin deleted the gdbelvin:mysql branch Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.