Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only load example group modules when they're used #2742

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion lib/rspec/rails.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,24 @@
require 'rspec/core'
require 'rails/version'

module RSpec
module Rails
autoload :RailsExampleGroup, 'rspec/rails/example/rails_example_group'
autoload :ControllerExampleGroup, 'rspec/rails/example/controller_example_group'
autoload :HelperExampleGroup, 'rspec/rails/example/helper_example_group'
autoload :ModelExampleGroup, 'rspec/rails/example/model_example_group'
autoload :RequestExampleGroup, 'rspec/rails/example/request_example_group'
autoload :RoutingExampleGroup, 'rspec/rails/example/routing_example_group'
autoload :ViewExampleGroup, 'rspec/rails/example/view_example_group'
autoload :FeatureExampleGroup, 'rspec/rails/example/feature_example_group'
autoload :SystemExampleGroup, 'rspec/rails/example/system_example_group'
autoload :MailerExampleGroup, 'rspec/rails/example/mailer_example_group'
autoload :JobExampleGroup, 'rspec/rails/example/job_example_group'
autoload :ChannelExampleGroup, 'rspec/rails/example/channel_example_group'
autoload :MailboxExampleGroup, 'rspec/rails/example/mailbox_example_group'
end
end

# Load any of our adapters and extensions early in the process
require 'rspec/rails/adapters'
require 'rspec/rails/extensions'
Expand All @@ -11,8 +29,10 @@
require 'rspec/rails/fixture_support'
require 'rspec/rails/file_fixture_support'
require 'rspec/rails/fixture_file_upload_support'
require 'rspec/rails/example'
require 'rspec/rails/vendor/capybara'
require 'rspec/rails/configuration'
require 'rspec/rails/active_record'
require 'rspec/rails/feature_check'
require 'rspec/rails/view_assigns'
require 'rspec/rails/view_path_builder'
require 'rspec/rails/view_spec_methods'
69 changes: 54 additions & 15 deletions lib/rspec/rails/configuration.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
# minitest was previously always loaded, and users may have come to depend on it.
require "minitest"

# rubocop: disable Metrics/ModuleLength
module RSpec
module Rails
Expand Down Expand Up @@ -45,15 +48,40 @@ class Configuration
#
# @api private
def self.add_test_type_configurations(config)
config.include RSpec::Rails::ControllerExampleGroup, type: :controller
config.include RSpec::Rails::HelperExampleGroup, type: :helper
config.include RSpec::Rails::ModelExampleGroup, type: :model
config.include RSpec::Rails::RequestExampleGroup, type: :request
config.include RSpec::Rails::RoutingExampleGroup, type: :routing
config.include RSpec::Rails::ViewExampleGroup, type: :view
config.include RSpec::Rails::FeatureExampleGroup, type: :feature
config.define_derived_metadata(type: :controller) do
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally wanted to use when_first_matching_example_defined here, but that hook doesn't fire until the first call to it, and some methods from the example group module could be called before then (e.g. render_views).

If this PR is accepted, I propose adding a new when_first_matching_example_group_defined hook to rspec-core and using it here instead.

config.include RSpec::Rails::ControllerExampleGroup, type: :controller
end

config.define_derived_metadata(type: :helper) do
config.include RSpec::Rails::HelperExampleGroup, type: :helper
end

config.define_derived_metadata(type: :model) do
config.include RSpec::Rails::ModelExampleGroup, type: :model
end

config.define_derived_metadata(type: :request) do
config.include RSpec::Rails::RequestExampleGroup, type: :request
end

config.define_derived_metadata(type: :routing) do
require "action_controller/test_case"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is necessary because RSpec::Rails::RoutingExampleGroup depends on ActionDispatch::Assertions::RoutingAssertions, which references ActionController::TestRequest without requiring the file where it's defined:
https://github.com/rails/rails/blob/v7.1.3.2/actionpack/lib/action_dispatch/testing/assertions/routing.rb#L242

I've fixed that upstream in rails/rails@5307976, but for compatibility with all existing Rails versions we need to keep eager loading ActionController::TestCase here.

config.include RSpec::Rails::RoutingExampleGroup, type: :routing
end

config.define_derived_metadata(type: :view) do
config.include RSpec::Rails::ViewExampleGroup, type: :view
end

config.define_derived_metadata(type: :feature) do
config.include RSpec::Rails::FeatureExampleGroup, type: :feature
end

config.include RSpec::Rails::Matchers
config.include RSpec::Rails::SystemExampleGroup, type: :system

config.define_derived_metadata(type: :system) do
config.include RSpec::Rails::SystemExampleGroup, type: :system
end
end

# @private
Expand Down Expand Up @@ -192,27 +220,38 @@ def fixture_path=(path)

if defined?(::Rails::Controller::Testing)
[:controller, :view, :request].each do |type|
config.include ::Rails::Controller::Testing::TestProcess, type: type
config.include ::Rails::Controller::Testing::TemplateAssertions, type: type
config.include ::Rails::Controller::Testing::Integration, type: type
config.define_derived_metadata(type: type) do
config.include ::Rails::Controller::Testing::TestProcess, type: type
config.include ::Rails::Controller::Testing::TemplateAssertions, type: type
config.include ::Rails::Controller::Testing::Integration, type: type
end
end
end

if RSpec::Rails::FeatureCheck.has_action_mailer?
config.include RSpec::Rails::MailerExampleGroup, type: :mailer
config.define_derived_metadata(type: :mailer) do
config.include RSpec::Rails::MailerExampleGroup, type: :mailer
end

config.after { ActionMailer::Base.deliveries.clear }
end

if RSpec::Rails::FeatureCheck.has_active_job?
config.include RSpec::Rails::JobExampleGroup, type: :job
config.define_derived_metadata(type: :job) do
config.include RSpec::Rails::JobExampleGroup, type: :job
end
end

if RSpec::Rails::FeatureCheck.has_action_cable_testing?
config.include RSpec::Rails::ChannelExampleGroup, type: :channel
config.define_derived_metadata(type: :channel) do
config.include RSpec::Rails::ChannelExampleGroup, type: :channel
end
end

if RSpec::Rails::FeatureCheck.has_action_mailbox?
config.include RSpec::Rails::MailboxExampleGroup, type: :mailbox
config.define_derived_metadata(type: :mailbox) do
config.include RSpec::Rails::MailboxExampleGroup, type: :mailbox
end
end
end

Expand Down
16 changes: 3 additions & 13 deletions lib/rspec/rails/example.rb
Original file line number Diff line number Diff line change
@@ -1,13 +1,3 @@
require 'rspec/rails/example/rails_example_group'
require 'rspec/rails/example/controller_example_group'
require 'rspec/rails/example/request_example_group'
require 'rspec/rails/example/helper_example_group'
require 'rspec/rails/example/view_example_group'
require 'rspec/rails/example/mailer_example_group'
require 'rspec/rails/example/routing_example_group'
require 'rspec/rails/example/model_example_group'
require 'rspec/rails/example/job_example_group'
require 'rspec/rails/example/feature_example_group'
require 'rspec/rails/example/system_example_group'
require 'rspec/rails/example/channel_example_group'
require 'rspec/rails/example/mailbox_example_group'
RSpec.warn_deprecation <<~WARNING
Requiring rspec/rails/example.rb no longer has any effect, and it will be removed in RSpec Rails 7.
WARNING
2 changes: 0 additions & 2 deletions lib/rspec/rails/example/helper_example_group.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
require 'rspec/rails/view_assigns'

module RSpec
module Rails
# @api public
Expand Down
4 changes: 0 additions & 4 deletions lib/rspec/rails/example/view_example_group.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,3 @@
require 'rspec/rails/view_assigns'
require 'rspec/rails/view_spec_methods'
require 'rspec/rails/view_path_builder'

module RSpec
module Rails
# @api public
Expand Down
2 changes: 2 additions & 0 deletions spec/rspec/rails/configuration_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ def in_inferring_type_from_location_environment
end

it "metadata `type: :request` sets up request example groups" do
require "rspec/rails/example/request_example_group"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to comment "This could cause order dependent failures, we typically do such requires in a sub process to avoid posioning the env" but its made me think prehaps we should be doing that all over now if we lazy load so that we don't accidentally depend on a module load order that we just happen to have in our specs 😧

a_rails_app = double("Rails application")
the_rails_module = Module.new {
def self.version; end
Expand Down Expand Up @@ -328,6 +329,7 @@ def self.application; end
end

it "metadata `type: :feature` sets up feature example groups" do
require "rspec/rails/example/feature_example_group"
a_rails_app = double("Rails application")
the_rails_module = Module.new {
def self.version; end
Expand Down