Skip to content

split spec bootstapping into separate files #1719

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

split spec bootstapping into separate files #1719

wants to merge 4 commits into from

Conversation

maxcal
Copy link

@maxcal maxcal commented Oct 4, 2016 •
edited
Loading

This splits the spec bootstrapping process into spec_helper.rb, rails_helper.rb and generator_helper.rb to avoid loading the entire rails stack when not needed.

I mostly picked the low hanging fruit - further optimizations could likely be gained by only loading the needed parts of the rails stack with config.include_context and loading files explicitly from spec/support.

JonRowe
JonRowe requested changes Oct 4, 2016
View reviewed changes
Copy link
Member

@JonRowe JonRowe left a comment

Choose a reason for hiding this comment

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

If this helps spec up working on rspec-rails I'm all for it but theres a few project stylistic issues to work through and one major point about requiring rspec/rails

.rspec
@@ -1,3 +1,2 @@
--warnings
--color
--require spec_helper
Copy link
Member

Choose a reason for hiding this comment

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

Please add this back, it's our project standard to require this here and not in spec files.

Copy link
Author

Choose a reason for hiding this comment

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

Fair enough - the point was really to be explicit about which bootstrapper is used for a given spec.

Copy link
Member

Choose a reason for hiding this comment

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

Our convention is that spec_helper contains the bare minimum required to configure the spec suite and everything else assumes its presence

spec/generators/rspec/controller/controller_generator_spec.rb
# Generators are not automatically loaded by Rails
require 'generators/rspec/controller/controller_generator'
require 'support/generators'

Copy link
Member

Choose a reason for hiding this comment

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

Please remove the excess whitespace introduced here and in similar places

spec/rspec/rails/assertion_delegator_spec.rb
@@ -1,4 +1,4 @@
require "spec_helper"
require 'spec_helper'
Copy link
Member

Choose a reason for hiding this comment

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

This can be removed as spec_helper should be required by .rspec otherwise please don't change our quotes we don't standardise them and would prefer less git churn.

spec/spec_helper.rb
end
I18n.enforce_available_locales = true if I18n.respond_to?(:enforce_available_locales)

require 'rspec/support/spec'
require 'rspec/rails'
Copy link
Member

Choose a reason for hiding this comment

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

Theres no point splitting the helpers if you leave this here.

@maxcal
Copy link
Author

maxcal commented Oct 4, 2016

Changes done

@maxcal maxcal changed the title split spec bootstapping into separate files #1023 split spec bootstapping into separate files Oct 4, 2016
@JonRowe
Copy link
Member

JonRowe commented Oct 5, 2016

Does this run locally for you? Seems to be failing significantly on CI

@maxcal
Copy link
Author

maxcal commented Oct 5, 2016

I get two failures from sanity_check_spec.rb but they where failing even before I made any changes.

@JonRowe
Copy link
Member

JonRowe commented Oct 5, 2016

If you check the build at least one spec is failing due to not having the correct things loaded

@maxcal
Copy link
Author

maxcal commented Oct 5, 2016

Which build?

@JonRowe
Copy link
Member

JonRowe commented Oct 5, 2016

Travis, see "Details" on the failure on this PR

@maxcal
Copy link
Author

maxcal commented Oct 5, 2016

This one seems to be failing due to a missing constant - so should have require 'rails_helper'. Funny that I did not catch that one locally.

https://travis-ci.org/rspec/rspec-rails/jobs/165104825

[Running spec/rspec/rails/example/feature_example_group_spec.rb
An error occurred while loading ./spec/rspec/rails/example/feature_example_group_spec.rb.
Failure/Error:
    describe FeatureExampleGroup do
      it_behaves_like "an rspec-rails example group mixin", :feature,
        './spec/features/', '.\\spec\\features\\'

      it "includes Rails route helpers" do
        with_isolated_stderr do
          Rails.application.routes.draw do
            get "/foo", :as => :foo, :to => "foo#bar"
          end
        end
NameError:
  uninitialized constant RSpec::Rails::FeatureExampleGroup](url)

A few of the other builds are failing due to dependency incompatibilities - which is not related to this PR since it does not touch the gemspec.

https://travis-ci.org/rspec/rspec-rails/jobs/165104820

Bundler could not find compatible versions for gem "actionpack":
  In Gemfile:
    rails was resolved to 3.2.22.5, which depends on
      actionpack (= 3.2.22.5)
    ammeter (~> 1.1.2) was resolved to 1.1.2, which depends on
      railties (>= 3.0) was resolved to 5.0.0.1, which depends on
        actionpack (= 5.0.0.1)
    rspec-rails was resolved to 3.6.0.pre, which depends on
      actionpack (>= 3.0)

@maxcal
Copy link
Author

maxcal commented Oct 5, 2016

Is the current master passing on Travis?

@JonRowe
Copy link
Member

JonRowe commented Oct 5, 2016

Bar the dependency issues which are being resolved in #1713 (hopefully)

@maxcal
Copy link
Author

maxcal commented Oct 5, 2016

I'm going to try to run the builds locally and track down any remaining issues and will author a new PR

@maxcal maxcal closed this Oct 5, 2016
@JonRowe
Copy link
Member

JonRowe commented Oct 5, 2016

Hm, would prefer you kept it to this PR... we regularly triage builds until they pass in PRs, better to keep the discussion in one place.

@maxcal
Copy link
Author

maxcal commented Oct 5, 2016

Ok

@maxcal maxcal reopened this Oct 5, 2016
@JonRowe
Copy link
Member

JonRowe commented Oct 5, 2016

If you fix the non dependancy related failures I'll probably merge this

@JonRowe
Copy link
Member

JonRowe commented Oct 5, 2016

You might need to run each spec file individually to find which ones are broken, we do this during the Travis build to ensure there aren't load order dependencies which we've missed, which I think is the case here.

@maxcal
Copy link
Author

maxcal commented Oct 6, 2016

Ok will do when I have time.

@JonRowe
Copy link
Member

JonRowe commented Oct 6, 2016

No rush :) If you rebase off master you should find the dependency build failures go away too

@mikegee
Copy link

mikegee commented Oct 31, 2018

This PR still looks pretty good to a casual observer like me. @maxcal do you think you could rebase it on the current master and see how that goes?

@JonRowe JonRowe closed this Oct 31, 2018
@JonRowe
Copy link
Member

JonRowe commented Oct 31, 2018

Closing as stale for now, if theres any interest in this let me know and I'll reopen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JonRowe JonRowe JonRowe requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maxcal @JonRowe @mikegee