Rails 8: Make test helpers work with deferred routes

[jeromedalbert]

Fixes https://github.com/heartcombo/devise/issues/5705.

Notes:

• The sign_in and sign_out controller and integration test helpers all call Devise::Mapping.find_scope!, so that method seemed like a good place to lazy load routes if they were not already loaded.
• Used try to keep backwards compatibility since reload_routes_unless_loaded only exists in Rails 8.

[jeromedalbert]

Closing, as the original issue is not an issue any more.

[jeromedalbert]

Reopening, as the issue reappeared on the latest Rails main.

[jeromedalbert]

Sorry for the ping @gmcgibbon, mentioning you just in case you recommend a better fix.

[gmcgibbon]

Ideally, we shouldn't need to load routes manually with the latest approach. There's no stacktrace in the issue. Please provide one and I'll see what I can do.

[jeromedalbert]

Ok, I have updated the issue with the fullest backtrace I could produce.

[jeromedalbert]

Here is my understanding of what the problem is.

With deferred route drawing, routes are lazy loaded for Rails environments that have config.eager_load disabled, which is the case by default for the development and test environments.

Basically by default for the test environment, whenever a route is visited, routes get lazy loaded. Devise's mappings aren't loaded when the test starts, they are only loaded when the test visits a route, because Devise's entry point is defined in the route files with devise_for or devise_scope.

But in integration and controller tests you typically call the Devise sign_in helper before visiting the route. At this point Devise mappings and scopes aren't loaded yet, so Devise errors out.

---

This seems like a particular case that is very specific to Devise. I don't think many gems define their helper methods on route load.

[gmcgibbon]

Yeah it is basically that https://github.com/heartcombo/devise/blob/a259ff3c28912a27329727f4a3c2623d3f5cb6f2/lib/devise/rails/routes.rb#L243 happens when routes are loaded, and mappings are global. We only really need to consider controller tests because https://github.com/heartcombo/devise/blob/a259ff3c28912a27329727f4a3c2623d3f5cb6f2/lib/devise/test/controller_helpers.rb#L7-L8 integration tests aren't meant to use this API.

I think it makes sense to move the mapping management to the route set, or to add a route load line to https://github.com/heartcombo/devise/blob/a259ff3c28912a27329727f4a3c2623d3f5cb6f2/lib/devise/mapping.rb#L35.

[jeromedalbert]

We only really need to consider controller tests

I think we need to consider both controller tests and integration tests because sign_in is defined similarly in integration_helpers.rb:

https://github.com/heartcombo/devise/blob/a259ff3c28912a27329727f4a3c2623d3f5cb6f2/lib/devise/test/integration_helpers.rb#L4-L14

And as shown in the attached issue, the test fails for an ActionDispatch::IntegrationTest integration test (I also separately checked that it fails for controller tests).

I think it makes sense to move the mapping management to the route set, or to add a route load line

This PR implements your second suggestion as a quick/easy fix ~by doing Rails.application.try(:reload_routes_unless_loaded). Although it is marked as :nodoc: in the Rails source code, which suggests that it is a private API and that it is not OK to use it outside of Rails. I guess this is on purpose? If so I can update the PR to use the slightly longer Rails.application.routes_reloader.try(:execute_unless_loaded) instead, which is a public API. I guess Rails doesn't need too many public APIs for this anyways, one is good enough, if at all.~ _(Edit: this PR now uses execute_unless_loaded from the public API to be on the safe side)_

Otherwise your first suggestion is to move the mapping management to the route set, but I am not familiar with this part and it seems like a more involved fix/refactor for Devise. But if this is the preferred way, it's all good if someone else wants to make a new PR.

[jeromedalbert]

Update: I have changed this PR to use the public execute_unless_loaded API to be on the safe side.

[AlanFoster]

I can't comment on the implementation, but as a datapoint - applying this patch locally fixed test our suite regressions after upgrading to rails 8.0.0.beta1 :+1:

[rafaelfranca]

Devise::Mapping.find_scope! is called in many other places that have to do with tests. Would not this code make those other methods slower when they don't need to load paths?

[jeromedalbert]

Would not this code make those other methods slower

Yes, the first call to find_scope! would be slower if you don't need routes (although see the paragraph below, maybe you do need routes after all). This PR is more of a quick/easy fix and may be not the best way. Gannon suggested either this solution or moving the mapping management to the route set, which seems like a more involved fix/refactor and I am not familiar with route sets. I'd be happy to close this PR if someone has a better solution.

when they don't need to load paths?

Maybe they need load paths after all. Global search results of find_scope! show that it seems to be called mostly by route path/url related methods (which need routes) or test helper methods like sign_in (which also need routes). So I guess places that call Devise::Mapping.find_scope! would need to load paths anyways, and this wouldn't be a concern? I am not familiar with the Devise codebase, but what is a scope really? My guess is that they are things defined in your routes by calls to e.g. devise_scope :users or devise_for :users (which internally calls devise_scope). If that is the case, then methods that call find_scope! are concerning themselves with routes and would need them to be loaded in order to find the scopes defined by those routes. This is just a guess.

[ciihla]

I had to do the similar patch locally:

` module DeviseHelpersPatch def sign_in(resource, deprecated = nil, scope: nil) Rails.application.routes_reloader.try(:execute_unless_loaded) super end

def sign_out(resource_or_scope) Rails.application.routes_reloader.try(:execute_unless_loaded) super end end

Devise::Test::ControllerHelpers.prepend(DeviseHelpersPatch) `

[nashby]

@jeromedalbert thank you for the PR! Even though it makes sense to call Rails.application.routes_reloader.try(:execute_unless_loaded) in Devise::Mapping.find_scope! I think we need to cover more places where Devise.mappings is used (places like https://github.com/heartcombo/devise/blob/0f514f1413e0f0ff2eb7e9b6f2c7644058c52c6d/lib/devise/mapping.rb#L50 or when users call Devise.mappings directly (for any reason))

So I was thinking about something like https://github.com/heartcombo/devise/compare/lazy-routes-fix This approach covers all possible scenarios where Devise.mappings is used. What do you think? /cc @carlosantoniodasilva

[jeromedalbert]

@nashby Thanks for looking into this! Sounds good to me.

I have tested your branch on a local Rails app and Devise test helpers like sign_in seem to work. And upon further inspection it looks like your Devise.mappings wrapper method is only called for my tests that visit a route, not my other tests, which is good as routes are loaded only as needed and this keeps the intent of lazy loading routes.