MONGOID-4521 Ensure that QueryCache doesn't cache multi-batch results #4490
Conversation
spec/mongoid/query_cache_spec.rb
Outdated
| @@ -414,15 +414,15 @@ | |||
| context "when querying a very large collection" do | |||
|
|
|||
| before do | |||
| 123.times { Band.create! } | |||
There was a problem hiding this comment.
What's the reason for this change?
There was a problem hiding this comment.
The purpose of this pull request is to disable the query cache for queries greater than one batch. Since the default batch size is less than 123, this test wouldn't actually test the query cache.
There was a problem hiding this comment.
The context is labeled "very large collection", sounds like it should be changed to "a collection with size less than batch size" and possibly there is already a context like that elsewhere in the file.
|
|
||
| context 'when the initial query does not exhaust the results' do | ||
| before do | ||
| Mongoid::QueryCache.enabled = true |
There was a problem hiding this comment.
Usually it is good idea to temporarily enable config option and restore original state once test is done (for example using around block) to not leak this change to another test env (often resulting into random failing tests).
mongoid/spec/mongoid/query_cache_spec.rb
Lines 308 to 313 in b3a5918
There was a problem hiding this comment.
Note that at the top of the description block, there is already an around block defined:
around do |spec|
Mongoid::QueryCache.clear_cache
Mongoid::QueryCache.cache { spec.run }
endThe implementation of QueryCache.cache already saves the value from before the passed block and restores it after.
|
|
||
| it 'does not cache the result' do | ||
| expect(Band.all.map(&:id).size).to eq(10) | ||
| end |
There was a problem hiding this comment.
This test is good but there should be another one setting batch size to 4 at query time.
There was a problem hiding this comment.
I'm not sure I understand what that has to do with the query cache; is the idea to do a query with the same batch size as the number of results of the previous query?
There was a problem hiding this comment.
Done, as discussed offline
No description provided.