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

Add support for repeatable assertions. #148

Closed
wants to merge 2 commits into from

Conversation

coderanger
Copy link

Implements #82.

The syntax looks like:

	mock.ExpectQuery("SELECT (.+) FROM articles WHERE id = ?").
		WithArgs(5).
		WillReturnRows(rs).
		AnyNumberOfTimes()

My specific use case is testing some polling APIs that might run any number of times during the test based on exact timing, but this seemed like a generally useful thing to have :-)

@codecov-io
Copy link

codecov-io commented Nov 10, 2018

Codecov Report

Merging #148 into master will decrease coverage by 5.46%.
The diff coverage is 42.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #148      +/-   ##
==========================================
- Coverage   91.02%   85.56%   -5.47%     
==========================================
  Files          13       13              
  Lines         691      769      +78     
==========================================
+ Hits          629      658      +29     
- Misses         43       85      +42     
- Partials       19       26       +7
Impacted Files Coverage Δ
rows.go 87.01% <100%> (+0.71%) ⬆️
sqlmock.go 83.66% <35.48%> (-10.34%) ⬇️
expectations.go 81.52% <52.63%> (-3.98%) ⬇️

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 e4e10dd...fbb34b2. Read the comment docs.

This only matters for repeatable expectations but it's a no-op on non-repeatables.
@l3pp4rd
Copy link
Member

l3pp4rd commented Nov 16, 2018

Hi @coderanger , I'll have a look later, need to review it carefully

@coderanger
Copy link
Author

👍 I can also write a bunch more tests to get the diff coverage up :)

@l3pp4rd
Copy link
Member

l3pp4rd commented Dec 11, 2018

I was thinking about it lately and I'm afraid this may lead to some very tricky issues, like:

  1. if we expect prepared statement to be closed and it is repeatable. Means that your code may prepare same statement few times and we would expect all of those to be closed, but in current implementation we would endup with only 1 close check. We could for example used a counter, how much times close was called on that statement, but then, what if user was not expecting some of those statements to be closed since the unit he was testing, was closing only some. Same will go for Rows.Close, Database.Close and maybe more.
  2. another thing is about the ordering of expectations. if we have a repeatable expectation, it checks order only the first time, later on, we cannot ensure an order any longer. So again this brings the inconsistency in the codebase.
  3. I think there may be other misbehaviors, which I cannot think of straight at this time, but this change is very risky, it may lead to misunderstandings. In result, this will be a leaky abstraction.

Could you explain the part of code which you needed this behavior for? because I think that was not a proper unit test.

I understand that there is plenty of boilerplate in sqlmock, but it is consistent and yields expected behavior, I do not want to introduce something too complex, which may cause the unsolvable issues like mentioned above and I would rather force users to repeat those expectations if it tests larger piece of code. Otherwise it is better to use functional integration tests.

@coderanger
Copy link
Author

The specific case is testing a Kubernetes operator. Basically it's a function running in the background on a tight loop, so the number of times the loop runs is based on exactly how fast the tests run in that particular moment. Unfortunately the SQL syntax I'm using isn't compatible with SQLite, so running a more functional-y test would require actually downloading and spinning up Postgres for each test :-/

@l3pp4rd
Copy link
Member

l3pp4rd commented Dec 12, 2018

well maybe you can extract that single call in this loop into a separate function in order to test it as unit. also you can make that call a functional interface, so you could mock it and test the whole behavior, for example calling it a third time it returns some failure result or something worth testing as an use case.

What you describe, does not look like an appropriate use case worth adding this kind of feature.

@coderanger
Copy link
Author

I do that too (testing single calls) and indeed those test cases don't require it, but tests for the reconciliation process as a whole need the loop running normally to simulate normal processing. I could also build my own Postgres interface but it would mean basically duplicating this project :)

@frioux
Copy link

frioux commented Apr 25, 2020

Love this added feature! I'm gonna use it!

@coderanger coderanger closed this Jan 4, 2021
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.

None yet

4 participants