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

Fix config ordering. #1459

Closed

Conversation

DougPuchalski
Copy link

use_fixtures setting must be declared before RSpec::Rails::FeatureExampleGroup

Already fixed on master via 33c3c73.

use_fixtures setting must be declared before RSpec::Rails::FeatureExampleGroup

Already fixed on master via 33c3c73.
@soulcutter
Copy link
Member

Can you further describe the problem? I'm not sure I understand.

:use_fixtures is used as a metadata tag filter so that RSpec::Rails::FixtureSupport will be included when an example group is tagged with :use_fixtures => true . I don't think of it as a setting, but we could just be using different terminology.

I see that RSpec::Rails::FeatureExampleGroup includes RailsExampleGroup, which in turn includes FixtureSupport - so isn't it included for :type => :feature specs in any ordering?

@DougPuchalski
Copy link
Author

@soulcutter The problem I am seeing is that fixture_support.rb references RSpec.configuration.fixture_path, which bombs without config.add_setting :fixture_path being hit first.

@soulcutter
Copy link
Member

I can't reproduce this problem. I tried defining an example, then requiring rspec-rails so that it would attempt to apply the include on RSpec.world.all_example_groups before defining the setting, but no matter what I tried I never hit this problem. Can you supply an example of when this fails?

@DougPuchalski
Copy link
Author

Never mind then. I thought the reasoning was clear, I guess not. I'll use a local patch or master instead.

@soulcutter
Copy link
Member

@aceofbassgreg I'm sorry if my questions frustrated you into closing this PR, that was not my intention. I want to address this issue if it's something you're encountering in practice.

I believe I follow your reasoning, and have tried to come up with scenarios that would demonstrate the problem but have been unsuccessful. Having a reproducible test case would help to prevent regressions in this behavior in the future (even though it's currently fixed for you in master).

I appreciate the time you've taken to make this contribution, and I hope this conversation doesn't put a damper on you contributing in the future.

@DougPuchalski
Copy link
Author

@soulcutter No worries, to me it visually looked wrong but I trust your judgment.

@aceofbassgreg
Copy link
Contributor

Hi, no frustrations from me either ;-).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants