Added configuration to allow for custom application engine. #539

Merged
merged 4 commits into from Jun 23, 2012

Conversation

Projects
None yet
9 participants
Contributor

danrasband commented May 10, 2012

After seeing the pull request at #471, I took it upon myself to push this along.

I've replicated the changes made in the above pull request, adding a few more changes for consistency, and also included tests.

To rehash what is in the above pull request, basically it makes it possible to run controller, mailer, requests, and routing specs when your site uses a rails engine to handle its routing. So, if you're using an engine like Spree, for example, you can add the following to your spec_helper.rb:

RSpec.configure do |config|
  config.application = MyApp::Engine
end

or, in the case of Spree:

RSpec.configure do |config|
  config.application = Spree::Core::Engine
end

Please let me know if you need any more information or if more work needs to be done.

This pull request fails (merged 83e24bc into 0677052).

Contributor

danrasband commented May 11, 2012

None of the rspec tests fail for me on this one, but about half of the cucumber tests do. I tried running the cucumber tests on the unchanged master in my fork with the same result, though, so I think my changes are good.

Contributor

justinko commented May 11, 2012

Thanks for the pull! Tried it on Rails 3.0.12 and got this error:

/Users/justinko/code/rspec-dev/repos/rspec-rails/spec/support/engine_example.rb:9:in `<top (required)>': undefined method `routes' for RSpec::EngineExample:Class (NoMethodError)

Let me know if you have trouble setting up to test with different Rails versions.

Contributor

danrasband commented May 11, 2012

Ah, that makes sense. This was my first pull request ever And I'm still pretty new to rails, so I didn't even think of testing across multiple versions of rails. My guess is that I'll somehow have to write version-specific code for rack applications. Is that an accepted practice for gems?

Contributor

justinko commented May 13, 2012

Should be pretty simple to support Rails 3.0.x. You'll have to dig in and find the differences - you can do Rails.version to get the Rails version. I'd like to make it very explicit that "this code is for Rails 3.0.x" instead of just using a respond_to? or something - so we know to remove it when the time comes.

@danrasband danrasband Make code version specific for rails 3.1 and up
This commit adds Rails version checking to the RSpec application
configuration to handle Rails 3.0.x. The Rails::Engine class, while
available in Rails 3.0, does not have the capability to draw routes as
it does in 3.1 and above.
acd46c2

This pull request passes (merged acd46c2 into 0677052).

Contributor

justinko commented May 17, 2012

Just to be clear, this feature will not work on Rails 3.0.x?

@justinko justinko and 1 other commented on an outdated diff May 17, 2012

lib/rspec/rails/engine_support.rb
@@ -0,0 +1,11 @@
+if Gem::Version.new(Rails.version) >= Gem::Version.new('3.1.0')
+ module RSpec
+ module Rails
+ module EngineSupport
+ RSpec::configure do |config|
+ config.add_setting :application, :default => ::Rails.application
+ end
+ end
+ end
+ end
+end
@justinko

justinko May 17, 2012

Contributor

Let's change this file name (and module name) to "application" - it really has nothing to do with "engines".

Also, let's always load this file (I know I said not to in a previous comment). We can then override the writer method to raise an error if it's not supported:

RSpec.configuration.add_setting :application, :default => ::Rails.application

unless rails_3_0?
  def Rails.configuration.application=(*)
    raise "Setting the application is only support on Rails 3.1 and above"
  end
end
@danrasband

danrasband Jun 1, 2012

Contributor

Should I add the override method to the application support file (I have now changed it to application.rb)?

It seems like you can't override a method this way, because I'm getting this error:

rspec-rails/lib/rspec/rails/application.rb:7: syntax error, unexpected '.', expecting ';' or '\n' (SyntaxError)
        def Rails.configuration.application=(*)

It doesn't seem to like this form of overriding...

@justinko justinko commented on an outdated diff May 17, 2012

lib/rspec/rails/example/controller_example_group.rb
@@ -120,7 +120,11 @@ def method_missing(method, *args, &block)
metadata[:type] = :controller
before do
- @routes = ::Rails.application.routes
+ if Gem::Version.new(Rails.version) >= Gem::Version.new('3.1.0')
+ @routes = RSpec.configuration.application.routes
+ else
+ @routes = ::Rails.application.routes
+ end
@justinko

justinko May 17, 2012

Contributor

With my comment above, it will git rid of all these conditionals. Conditionals are bad! :)

@justinko justinko commented on an outdated diff May 17, 2012

spec/rspec/rails/configuration_spec.rb
+
+ describe "#application" do
+
+ context "default" do
+
+ it "is Rails.application by default" do
+ if Gem::Version.new(Rails.version) >= Gem::Version.new('3.1.0')
+ RSpec.configuration.application.should eq(::Rails.application)
+ else
+ expect { RSpec.configuration.application }.should raise_error
+ end
+ end
+
+ end
+
+ context "custom rack application" do
@justinko

justinko May 17, 2012

Contributor

You can avoid conditionals in the specs by using "exclusion filters" (RSpec won't run them) with the rails_3_0? method.

https://www.relishapp.com/rspec/rspec-core/v/2-10/docs/filtering/exclusion-filters

Contributor

justinko commented May 17, 2012

BTW, thanks for your patience! I know we'll emerge with some clean code :)

Contributor

danrasband commented May 17, 2012

Thank you for reviewing all of my code!!! All of your suggestions are extremely helpful. I'll work on implementing them as soon as I can.

And yes, I'm pretty sure that we'd have to make the application configuration option only available for Rails 3.1+.

zamith commented May 25, 2012

I solved this problem with

before(:each) { @routes = MyApp::Engine.routes }

which I don't feel is really nice, would love to have this pulled and documented, since I found it hard to get working.

Keep up the good work. ;)

Contributor

danrasband commented May 30, 2012

Unfortunately I found some bugs with the most recent commit (acd46c2). Using the configuration option makes some controller tests fail. I'll do some more research when I have the time to figure out what's going on.

Contributor

danrasband commented Jun 1, 2012

I've gotten everything working (and tests passing) except for one thing: overriding the RSpec.configuration.application= method. I also opted to name the method for determining rails version compatibility as follows:

at_least_rails_3_1?

I put this method in lib/rspec/rails.rb as a global method, but I'm not sure if this is a good idea:

# ... some code ...

# METHOD TO BE ADDED HERE
def at_least_rails_3_1?
  Gem::Version.new(Rails.version) >= Gem::Version.new('3.1.0')
end

require 'rspec/rails/extensions'
# ... some more code ...

Any suggestions?

Contributor

justinko commented Jun 2, 2012

@danrasband can you push the code please?

@danrasband danrasband Tweak alternate app integration
This commit removes most if-statements from the example groups,
consolidating to a global method called `at_least_rails_3_1`. It
also adds filters to the rspec tests and changes engine_support.rb
to application.rb.
1c451f8

This pull request passes (merged 1c451f8 into 0677052).

Contributor

justinko commented Jun 6, 2012

I've gotten everything working (and tests passing) except for one thing: overriding the RSpec.configuration.application= method.

What problem is it giving you? Looks like your implementation should work.

@danrasband danrasband Raise error for Rails 3.0
Adds an override of Rails.configuration.application=(*) setter to
raise an error when Rails.configuration.application is set in Rails
3.0. Also moves at_least_rails_3_1? method to the RSpec::Rails module
as a public module method.
b694543
Contributor

danrasband commented Jun 22, 2012

I got everything working now (I think). Thanks for all the help!

This pull request passes (merged b694543 into 0677052).

Contributor

justinko commented Jun 22, 2012

Awesome! Going to merge this in a bit.

justinko merged commit 248ce66 into rspec:master Jun 23, 2012

@dchelimsky dchelimsky commented on the diff Jun 26, 2012

lib/rspec/rails/application.rb
@@ -0,0 +1,18 @@
+require 'rspec-rails'
+module RSpec
+ module Rails
+ module Application
+ RSpec.configuration.add_setting :application, :default => ::Rails.application
+
+ # Raise an error when setting application in Rails < 3.1
+ unless RSpec::Rails.at_least_rails_3_1?
+ class << RSpec.configuration
+ def application=(*)
@dchelimsky

dchelimsky Jun 26, 2012

Owner

@danrasband @justinko Does this need to happen via RSpec config? Any reason not to just set Rails.application = xxx?

@justinko

justinko Jun 26, 2012

Contributor

@dchelimsky Side note: the implementation of this feature on master is drastically simpler.

@danrasband Can you please confirm @dchelimsky's question? Thanks.

@dchelimsky

dchelimsky Jun 26, 2012

Owner

I saw the implementation and agree it's clean - my issue is that this is not really an RSpec concern - it's a Rails concern.

@danrasband

danrasband Jun 27, 2012

Contributor

@dchelimsky I don't really know the answer to your question. I'll try to do some experimentation, and if doing Rails.application = xxx works and doesn't have unintended side effects, then I think that may be the way to go.

@dchelimsky

dchelimsky Jul 4, 2012

Owner

I'm planning to release 2.11 this weekend, but I don't want to include this as/is. @danrasband - will you have time to look at this before then?

@danrasband

danrasband Jul 4, 2012

Contributor

I should be able to look at this today or tomorrow.

@dchelimsky dchelimsky added a commit that referenced this pull request Jul 7, 2012

@dchelimsky dchelimsky revert all changes for #539
- not ready for release so backing out
- changes are stored in engine-support branch
603cf3d
Owner

dchelimsky commented Jul 7, 2012

I branched off master to the engine-support branch and then backed out these changes on master for the 2.11 release. We can revisit for the next release.

Contributor

jfirebaugh commented Dec 8, 2012

Should this be reopened? It seems like it's still an issue in 2.12.

Contributor

alindeman commented Dec 8, 2012

@jfirebaugh, you mean the code here didn't actually fix the issue?

Contributor

jfirebaugh commented Dec 8, 2012

From my reading of the comments, the changes got backed out in 2.11, to be revisited for 2.12. Did it actually get reinstated in 2.12?

Contributor

alindeman commented Dec 8, 2012

Oh. Right. Let me restore my context on this (might be Sunday or Monday) and get back to you.

Contributor

alindeman commented Dec 28, 2012

As far as I can tell, setting ::Rails.application = MyEngine::Engine (e.g., in spec_helper.rb) removes the need to use the use_route option (described in the Testing Rails Engines guide) in controller specs.

Does this work for everyone?

/cc: @jfirebaugh, @danrasband

Contributor

jfirebaugh commented Dec 28, 2012

No, that doesn't work. My specs fail with all sorts of errors, most prominently:

ActionView::Template::Error: undefined method `assets' for #<Rails::Engine::Configuration:0x007f7f9a27b5e8>

I'm pretty sure that for engine tests, a full dummy application is needed.

Contributor

jfirebaugh commented Dec 28, 2012

BTW, I'm coming here from rails/rails#4364, if that context helps.

Contributor

alindeman commented Dec 28, 2012

Good point. I hadn't tried a view spec.

Given that the Rails guides themselves recommend the use_route option, I am inclined (at this point) to leave routing as-is. What do you think?

Contributor

alindeman commented Dec 28, 2012

BTW, I'm coming here from rails/rails#4364, if that context helps.

Whoops, saw this after my previous comment.

This does help a bit. I probably need to spin up a Rails application that fully demonstrates the problem to poke around. I've added this to my list, but if you have time, feel free to do so and push it up to GitHub.

Exoth commented Jan 8, 2013

It would be nice to have it added in the next version as starting from version 2.12.1 this good old hack stopped working for routing specs:

before { @routes = CmsEngine::Engine.routes }

Now I need to do:

before do
  @routes = CmsEngine::Engine.routes
  assertion_instance.instance_variable_set(:@routes, CmsEngine::Engine.routes)
end

Which is quite clumsy.

jcoyne commented Jan 9, 2013

I also have the same use-case / complaint as @Exoth. Can we have a simple way to test our engine routes?

Contributor

alindeman commented Jan 12, 2013

It would be nice to have it added in the next version as starting from version 2.12.1 this good old hack stopped working for routing specs

2.12.2 will be released soon with the relevant code reverted. We should figure out a better way to expose the engine's routes, but I'm not sure what that will look like yet.

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