ar_lazy_preload icon indicating copy to clipboard operation
ar_lazy_preload copied to clipboard

Published 20 hours ago • verified publisher icon DmitryTsepelev

Remove Rails direct dependency

Open taylorthurlow opened this issue 2 years ago • 8 comments

Follow up on #51. Haven't looked through much of the code yet so this might be a super naïve change. Basically looking to see if tests pass before I start writing any tests from scratch for non-Rails environments.

Closes #51.

taylorthurlow avatar Jul 12 '21 23:07 taylorthurlow

This is now working in my non-Rails project and specs are passing, but I haven't added any specs. I'm not really sure how to approach testing a standard Rack app given that the dummy test application is already a Rails application. Any suggestions would be appreciated. Things worth noting include:

  • rails has been moved to be a development dependency as it is still used directly in specs
  • The code path defined?(::Rails) returns false will not currently be executed in specs
  • Non-Rails applications must call ArLazyPreload.install_hooks manually after ActiveRecord has been initialized. I don't really like this method name so suggestions on renaming it are appreciated.

taylorthurlow avatar Jul 13 '21 00:07 taylorthurlow

Pull Request Test Coverage Report for Build 1024595647

  • 0 of 18 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.006%) to 99.254%
Totals Coverage Status
Change from base Build 853918745: 0.006%
Covered Lines: 266
Relevant Lines: 268
💛 - Coveralls

coveralls avatar Jul 16 '21 06:07 coveralls

Pull Request Test Coverage Report for Build 4377308429

  • 18 of 18 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 98.621%
Totals Coverage Status
Change from base Build 3792853907: -0.01%
Covered Lines: 286
Relevant Lines: 290
💛 - Coveralls

coveralls avatar Jul 16 '21 06:07 coveralls

Yeah, the approach looks fine! I guess we could add another dummy app (dummy_rack?), configure it with brand new .install_hooks and add use it for specs instead of dummy when special env var is set (e.g., if RACK=true then we do require_relative "dummy_rack/config/environment" [here](https://github.com/DmitryTsepelev/ar_lazy_preload/blob/master/spec/spec_helper.rb#L23)). Thoughts?

DmitryTsepelev avatar Jul 16 '21 06:07 DmitryTsepelev

Seems like a decent approach, I'll give it a try and see if it works out. 👍

taylorthurlow avatar Jul 16 '21 16:07 taylorthurlow

Hello @DmitryTsepelev :wave:

Thank you for the amazing job! Could you please say if this PR needs any help to be merged?

djezzzl avatar Dec 26 '21 10:12 djezzzl

@djezzzl Looks like the patch is working, but we need to test it somehow. We could try doing something like this

DmitryTsepelev avatar Dec 27 '21 09:12 DmitryTsepelev

Sounds reasonable for me, I'll provide a test :+1:

djezzzl avatar Dec 27 '21 09:12 djezzzl

@DmitryTsepelev Implemented a rack test mode as you suggested.

Additionally worth mentioning is my modification of the Actions definition files to include a RACK=true pass to both the rspec workflow and the coverage workflow. Not sure if I've done this as you might prefer it done.

taylorthurlow avatar Mar 07 '23 00:03 taylorthurlow

Tests ran but failed due to an included trailing comma, fixed that and force pushed.

taylorthurlow avatar Mar 07 '23 08:03 taylorthurlow