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

Don't automatically require rails_helper #11352

Merged
merged 6 commits into from
Jul 11, 2019
Merged

Don't automatically require rails_helper #11352

merged 6 commits into from
Jul 11, 2019

Conversation

monfresh
Copy link
Contributor

@monfresh monfresh commented Jul 10, 2019

Why: Our "rails_helper" file contains a lot of intensive work that
heavily slows down tests. By automatically requiring this file in
.rspec, it unnecessarily slows down tests that don't need it.

As an example, none of our specs in mappers require rails_helper.
When we run specs the old way, the specs run in 16.77 seconds. When we
stop requiring rails_helper, they run in 0.03745 seconds. That's
448x faster!

The recommended best practice is to only require rails_helper when
it is needed. The next step is to break down the different parts of
rails_helper into separate files in spec/support so that we can
further speed up tests. For example, some of the setup in rails_helper
only applies to feature specs.

@va-bot
Copy link
Collaborator

va-bot commented Jul 10, 2019

1 Error
🚫 focus: true is left in test

Generated by 🚫 Danger

@codeclimate
Copy link

codeclimate bot commented Jul 10, 2019

Code Climate has analyzed commit fbc7ec1 and detected 0 issues on this pull request.

View more on Code Climate.

**Why**: Our "rails_helper" file contains a lot of intensive work that
heavily slows down tests. By automatically requiring this file in
`.rspec`, it unnecessarily slows down tests that don't need it.

As an example, none of our specs in `mappers` require `rails_helper`.
When we run specs the old way, the specs run in 16.77 seconds. When we
stop requiring `rails_helper`, they run in 0.03745 seconds. That's
448x faster!

The recommended best practice is to only require `rails_helper` when
it is needed. The next step is to break down the different parts of
`rails_helper` into separate files in `spec/support` so that we can
further speed up tests. For example, some of the setup in `rails_helper`
only applies to feature specs.
Copy link
Contributor

@pkarman pkarman left a comment

Choose a reason for hiding this comment

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

I'm cool with this change, though I'm not sure about the focus: true warning, which I assume is why the CI tests fail to finish.

@monfresh
Copy link
Contributor Author

I don't think it's related. The specs passed earlier. Danger is complaining because spec_helper contains the line config.filter_run focus: true.

@monfresh monfresh added the Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master label Jul 10, 2019
@monfresh monfresh merged commit 9b581d4 into master Jul 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants