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

Go modules support and missing unit tests #18

Merged
merged 4 commits into from
Aug 15, 2019
Merged

Go modules support and missing unit tests #18

merged 4 commits into from
Aug 15, 2019

Conversation

swithek
Copy link
Collaborator

@swithek swithek commented Aug 13, 2019

This PR adds go modules support and missing unit tests, completely removes integration tests. I think it would be appropriate, after merging this branch into master, to tag the latest commit as v1.0.0

@swithek swithek requested a review from qustavo August 13, 2019 09:57
Copy link
Owner

@qustavo qustavo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks great!
Though, why did you remove the integration test?

@swithek
Copy link
Collaborator Author

swithek commented Aug 13, 2019

Since the unit tests fully cover all aspects of dotsql by utilizing a mocked db layer, it is redundant to try to cover the same parts of code with integration tests whose only difference is real db usage instead of a mocked one. In this case, tests are not only slower, but they also introduce an unnecessary dependency (sqllite).

@qustavo
Copy link
Owner

qustavo commented Aug 13, 2019

I see your point, but I still do think that testing against a real database gives you more confidence than mocks

@qustavo
Copy link
Owner

qustavo commented Aug 13, 2019

Additionally, the fact that they run "slow" shouldn't be a problem cause they by default they don't run, unless you tags=integration

@swithek
Copy link
Collaborator Author

swithek commented Aug 13, 2019

Extra layer of testing sounds reasonable, though I feel that integration tests wouldn't add any benefit mostly because neither the input (SQL queries) nor the execution (db implementation / driver) are influenced in any way by dotsql, we only need to ensure that those queries are delivered correctly to the db and that is easily checked via mocks.

@swithek
Copy link
Collaborator Author

swithek commented Aug 13, 2019

But having those tests doesn't hurt nobody (sqlite will only be added as a dependency to go.mod and the less dependencies the better), so I could return integration tests.

@swithek
Copy link
Collaborator Author

swithek commented Aug 14, 2019

I modified the commits of this branch to preserve the integration tests. Everything else is the same.

@swithek swithek merged commit 23aa1e8 into master Aug 15, 2019
@swithek swithek deleted the modules branch August 15, 2019 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants