Skip to content

Warn if a fixture method is called from a before(:context) block. #1501

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

Merged
merged 2 commits into from
Dec 15, 2015
Merged
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
2 changes: 2 additions & 0 deletions Changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ Bug fixes:

* Make it possible to write nested specs within helper specs on classes that are
internal to helper classes. (Sam Phippen, Peter Swan, #1499).
* Warn if a fixture method is called from a `before(:context)` block, instead of
crashing with a `undefined method for nil:NilClass`. (Sam Phippen, #1501)

### 3.4.0 / 2015-11-11
[Full Changelog](http://github.com/rspec/rspec-rails/compare/v3.3.3...v3.4.0)
Expand Down
1 change: 1 addition & 0 deletions example_app_generator/generate_stuff.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ def setup_tasks

def final_tasks
copy_file 'spec/verify_active_record_spec.rb'
copy_file 'spec/verify_fixture_warning_spec.rb'
run('bin/rake db:migrate')
if ::Rails::VERSION::STRING.to_f < 4.1
run('bin/rake db:migrate RAILS_ENV=test')
Expand Down
64 changes: 64 additions & 0 deletions example_app_generator/spec/verify_fixture_warning_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
require 'rails_helper'

RSpec.describe "Fixture warnings" do
def generate_fixture_example_group(hook_type)
RSpec.describe do
include RSpec::Rails::RailsExampleGroup
fixtures :things

before(hook_type) do
things :a
end

it "" do

end
end
end

it "Warns when a fixture call is made in a before :context call" do
expect(RSpec).to receive(:warn_with).with(match(/Calling fixture method in before :context/))

generate_fixture_example_group(:context).run
end

it "Does not warn when a fixture call is made in a before :each call" do
expect(RSpec).not_to receive(:warn_with)

generate_fixture_example_group(:each).run
end

end

RSpec.describe "Global fixture warnings" do
def generate_fixture_example_group(hook_type)
RSpec.describe do
include RSpec::Rails::RailsExampleGroup

before(hook_type) do
things :a
end

it "" do

end
end
end
around do |ex|
RSpec.configuration.global_fixtures = [:things]
ex.call
RSpec.configuration.global_fixtures = []
end

it "warns when a global fixture call is made in a before :context call" do
expect(RSpec).to receive(:warn_with).with(match(/Calling fixture method in before :context/))

generate_fixture_example_group(:context).run
end

it "does not warn when a global fixture call is made in a before :each call" do
expect(RSpec).not_to receive(:warn_with)

generate_fixture_example_group(:each).run
end
end
22 changes: 22 additions & 0 deletions lib/rspec/rails/fixture_support.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,28 @@ module FixtureSupport
self.use_transactional_fixtures = RSpec.configuration.use_transactional_fixtures
end
self.use_instantiated_fixtures = RSpec.configuration.use_instantiated_fixtures

def self.fixtures(*args)
orig_methods = private_instance_methods
super.tap do
new_methods = private_instance_methods - orig_methods
new_methods.each do |method_name|
proxy_method_warning_if_called_in_before_context_scope(method_name)
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Could we make this a name module (I'm ok with dynamically creating it) so that it's clear from the ancestor chain we've inserted something?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

end

def self.proxy_method_warning_if_called_in_before_context_scope(method_name)
orig_implementation = instance_method(method_name)
define_method(method_name) do |*args, &blk|
if inspect.include?("before(:context)")
RSpec.warn_with("Calling fixture method in before :context ")
Copy link
Member Author

Choose a reason for hiding this comment

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

This message needs to be better. @cupakromer @JonRowe can you post some suggestions in here?

Copy link
Member

Choose a reason for hiding this comment

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

Using fixtures from before(#{type}) hook? Also what happens with before(:group) here? And before(:all) / before(:suite)?

Copy link
Member Author

Choose a reason for hiding this comment

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

do you think we should interpolate the fixture method that was called?

Copy link
Member

Choose a reason for hiding this comment

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

We return the line number / file right? If so no, otherwise yes.

else
orig_implementation.bind(self).call(*args, &blk)
end
end
end

fixtures RSpec.configuration.global_fixtures if RSpec.configuration.global_fixtures
end
end
Expand Down