[bug/spec fix] fix the error on NOT

[Dingying0410]

Summary

This will fix the build error introduced in #62. The build has passed.

[codecov]

Codecov Report

Merging #65 (e12b031) into main (b1b590d) will increase coverage by 0.02%.
The diff coverage is 96.77%.

❗ Current head e12b031 differs from pull request most recent head 05be08d. Consider uploading reports for the commit 05be08d to get more accurate results

@@            Coverage Diff             @@
##             main      #65      +/-   ##
==========================================
+ Coverage   97.34%   97.37%   +0.02%     
==========================================
  Files          34       34              
  Lines        2186     2209      +23     
==========================================
+ Hits         2128     2151      +23     
  Misses         58       58              

Impacted Files	Coverage Δ
spec/cache_spec.rb	98.90% <95.00%> (+0.18%)	⬆️
lib/redis_memo/cache.rb	98.38% <100.00%> (+0.14%)	⬆️
spec/memoize_query_spec.rb	100.00% <100.00%> (ø)

---

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 b1b590d...05be08d. Read the comment docs.

[donaldong]

Actually, can you help me better understand how this fixes the issue?

Looks like the build somehow passed in rails 6.1 and HEAD
https://github.com/chanzuckerberg/redis-memo/runs/2169583715

Should we expect that?

[Dingying0410]

re Donald's question, I think I know what is going on:

1. there is a change of implementing NOT in rails 6.0 and rails 6.1, see https://bigbinary.com/blog/rails-6-deprecates-where-not-working-as-nor-and-will-change-to-nand-in-rails-6-1, and chained NOT is deprecated in 6.0 (but just a warning), and interpreted as NAND in rails 6.1 and before rials 6, it is interpreted as NOR
2. all the rspecs in [feature] enable caching on queries with NOT #62 failed with

expected: [#<Site id: 1, a: 1, b: 1, not_memoized: 0, my_enum: "x">]
            got: []

this is expected

3. in the specs here, in rails 6.0 it will show up a waring https://travis-ci.com/github/chanzuckerberg/redis-memo/jobs/495029171

I think this will not influence our features as we don't cache the chained NOT in either case, but might affect us in writing the specs.

I have a couple of solutions in mind:

1. don't have the test case for Site.where.not(a: 1, b: 1) since it will be deprecated
2. keep this test case, but only test it does not cache
3. keep this test case, but have the rspec testing different scenario for gem versions, like this approach , I think this might only affect local tests? since we already tested different rails versions in CI

do you guys have any thoughts about supporting different rails versions?

@katyho @donaldong

[donaldong]

Ah makes sense. Thanks for looking into this. To me, I have a slight preference over the 3rd approach since they are still valid rails usage in the earlier versions. We can check Rails.version too in rspec.

[donaldong]

This comment is slightly confusing. The ones after L471 seem to be clearer to me.

[Dingying0410]

changed a bit, let me know if that is clear enough

[donaldong]

🎉