DatabaseCleaner / database_cleaner-active_record

Strategies for cleaning databases using ActiveRecord. Can be used to ensure a clean state for testing.
MIT License
64 stars 63 forks source link

Fully support Active Record 6.1 #50

Closed dsantosmerino closed 3 years ago

dsantosmerino commented 3 years ago

As @Kuchick mentioned #48, the current code doesn't support Active Record 6.1. The main change is about retrieving the database config, while being backward compatible (related Rails PR - https://github.com/rails/rails/pull/37253). Also added Rails 6.1 in the CI matrix to be sure that is working as expected.

This is my first attempt to contribute here, so please let me know if you need anything to be adapted :pray:

codecov[bot] commented 3 years ago

Codecov Report

Merging #50 (6a72ecf) into master (24cc517) will decrease coverage by 0.76%. The diff coverage is 92.98%.

Impacted file tree graph

@@             Coverage Diff             @@
##            master      #50      +/-   ##
===========================================
- Coverage   100.00%   99.23%   -0.77%     
===========================================
  Files           12       12              
  Lines          516      523       +7     
===========================================
+ Hits           516      519       +3     
- Misses           0        4       +4     
Impacted Files Coverage Δ
lib/database_cleaner/active_record/base.rb 92.00% <76.47%> (-8.00%) :arrow_down:
spec/database_cleaner/active_record/base_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 24cc517...6a72ecf. Read the comment docs.

botandrose commented 3 years ago

Hi @dsantosmerino! This looks really good. Thanks for putting this together. I'm not immediately certain why Codecov is complaining about loss of test coverage. Can you see if you can figure that out so we can stay at 100% coverage? Once that's clear I'll be happy to merge and release this!

botandrose commented 3 years ago

@dsantosmerino Yo, I found myself with some free time today, and I want to get this merged and released ASAP, so I figured out what was going on with the missing test coverage. Turns out that the refactor exposed missing coverage that was there all along! Before the refactor, all of this was on one line:

models.select(&:connection_pool).detect { |m| m.connection_pool.spec.config[:database] == database_name }

models was always an empty array in the existing tests, though. So the line was technically covered, but the select and detect blocks were never actually executed! Your refactoring split those block off into a new line and with a new extracted method, and exposed this issue.

This is great! I've written a few tests to resolve this missing coverage, so I'm going to merge this PR, and then push the new tests on top to get back to 100%.

Thanks for your great work here!

kuchick commented 3 years ago

@dsantosmerino and @botandrose thanks! @botandrose Can these changes be added to database_cleaner v1.8 (as 1.8.6) to have Rails 6.1 support because of 2.0 version is in beta?

botandrose commented 3 years ago

Just released v2.0 of all gems!

On Sun, Jan 31, 2021 at 3:17 PM Seryozhka notifications@github.com wrote:

@dsantosmerino https://github.com/dsantosmerino and @botandrose https://github.com/botandrose thanks! @botandrose https://github.com/botandrose Can these changes be added to database_cleaner v1.8 (as 1.8.6) to have Rails 6.1 support because of 2.0 version is in beta?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DatabaseCleaner/database_cleaner-active_record/pull/50#issuecomment-770469461, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAEH376NCS4EIZPA3SFR3DS4XQILANCNFSM4WWTAV2A .