Rails 8: Make test helpers work with deferred routes

[jeromedalbert]

Fixes #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

devise/lib/devise/rails/routes.rb

Line 243 in a259ff3

mapping = Devise.add_mapping(resource, options)

happens when routes are loaded, and mappings are global. We only really need to consider controller tests because

devise/lib/devise/test/controller_helpers.rb

Lines 7 to 8 in a259ff3

	# quickly sign_in or sign_out a user. Do not use
	# `Devise::Test::ControllerHelpers` in integration tests.

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

devise/lib/devise/mapping.rb

Line 35 in a259ff3

def self.find_scope!(obj)

.

[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:

devise/lib/devise/test/integration_helpers.rb

Lines 4 to 14 in a259ff3

	# Devise::Test::IntegrationHelpers is a helper module for facilitating
	# authentication on Rails integration tests to bypass the required steps for
	# signin in or signin out a record.
	#
	# Examples
	#
	# class PostsTest < ActionDispatch::IntegrationTest
	# include Devise::Test::IntegrationHelpers
	#
	# test 'authenticated users can see posts' do
	# sign_in users(:bob)

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 👍

[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

devise/lib/devise/mapping.rb

Line 50 in 0f514f1

Devise.mappings.each_value { |m| return m if path.include?(m.send(path_type)) }

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.