Remove `ActiveRecord` based settings from generated `rails_helper` #1150

cupakromer · 2014-08-21T02:42:10Z

Improve support for Rails apps which do not use ActiveRecord. This does not add settings in the generated spec/rails_helper.rb file which are specific to ActiveRecord. This allows new apps to run without modification, instead of raising errors.

To Do

Check for ActiveRecord in the rails_helper.rb template
Spec out changes to the template
Do not attempt to run migrations if the example app does not use ActiveRecord
Implement a 2nd set of smoke tests which do not use ActiveRecord

myronmarston · 2014-08-22T22:57:48Z

spec/generators/rspec/install/install_generator_spec.rb

      run_generator
-      expect(File.read( file('spec/rails_helper.rb') )).not_to match(/ActiveRecord::Migration\.check_pending!/m)
+      expect(File.read( file('spec/rails_helper.rb') )).
+        to match(/^require 'rspec\/rails'$/m)


I generally dislike this style (where the line break is between expect and `to). I generally tend to use a local for the target:

contents = File.read( file('spec/rails_helper.rb') ) expect(contents).to match(/^require 'rspec\/rails'$/m)

...or put the line break at the opening paren on the matcher:

expect(File.read( file('spec/rails_helper.rb') )).to match( /^require 'rspec\/rails'$/m )

That's just subjective personal preference, though.

Interesting, I tend to fluctuate between breaking as I did and using a boxed matcher. It generally just depends on the matcher.

I think in this case using locals will provide a more readable spec.

What do you mean by a "boxed matcher"?

Your breaking at the opening paren. To me it looks like the matcher boxes-in the condition:

.to match( /^require 'rspec\/rails'$/m )

myronmarston · 2014-08-22T23:01:06Z

This is looking pretty good but I'm not familiar with these parts of rspec-rails (or with recent versions of rails), so someone who knows that stuff better (perhaps @xaviershay?) should probably review as well.

xaviershay · 2014-08-23T00:50:24Z

spec/generators/rspec/install/install_generator_spec.rb

+        not_to match(/config\.use_transactional_fixtures/m)
+    end
+
+    case ::Rails::VERSION::STRING.to_f


backwards compatibility sucks :(

xaviershay · 2014-08-23T00:54:46Z

Have we written down our official support matrix? And have we considered making it smaller? Like, supporting 4.0 going forward doesn't seem that useful. Rails team isn't even supporting it.

To keep semver, that's maybe a pretty good argument for decoupling rspec-rails version numbers from rspec-core. (Or maybe matching them to rails major/minor!?)

xaviershay · 2014-08-23T00:54:55Z

LGTM though

myronmarston · 2014-08-23T01:53:19Z

Have we written down our official support matrix?

Officially, it's Rails 3+, against whatever versions of ruby those versions of rails support. You can see the travis build matrix from a recent build:

https://travis-ci.org/rspec/rspec-rails/builds/33315313

Our official support is a bit larger than that simply because we support all 3.0.x releases of rails but only test against the latest patch release of each minor version to cut down on travis build times.

And have we considered making it smaller?

We talked a bit about it when planning what RSpec 3 would be. I said I thought it would be OK for us to drop support for Rails 3, but that I thought it would also be nice to keep supporting it if it is not a big maintenance burden. I left the choice up to Andy and at the time he thought the benefit of supporting it is greater than the cost of doing so.

Since we follow SemVer, we can't drop support for any rails versions until rspec-rails 4.x...unless we decide that SemVer only applies to the other RSpec libs and doesn't apply to rspec-rails.

Like, supporting 4.0 going forward doesn't seem that useful. Rails team isn't even supporting it.

Historically, upgrading Rails has not been easy, so even though the rails team isn't supporting it, there are many, many apps out there on Rails 3.0, 3.1, 3.2 and 4.0...and those apps may want to run the latest RSpec. I think that we do the ruby community a great service by keeping rspec-rails compatible with so many versions of rails (provided rails does a decent job of not breaking the extension points we use...), but of course such support takes a lot of effort and I don't to trivialize that.

If we want to consider dropping support for some rails versions I think we should try to do some kind of community survey so that we have some data about what versions of rails people are actually using with RSpec 3, and let that data guide our decisions. Or, instead of a survey, maybe we can ask some of the companies that have access to Gemfiles from many real apps (e.g. newrelic, github, travis) if they can provide us with this data from their datasets. (New Relic, for example, has published stats about this kind of stuff previously).

To keep semver, that's maybe a pretty good argument for decoupling rspec-rails version numbers from rspec-core.

Interesting idea. If we want to drop support for older versions of rails before we're ready for RSpec 4, we should probably do this.

(Or maybe matching them to rails major/minor!?)

I think this would probably be a big PITA, because it means rails release' schedule would dictate ours. We could only make breaking changes when rails releases a major version, and we can only release new features when rails releases a minor version.

JonRowe · 2014-08-23T13:39:01Z

Probably should have had the versions discussion before RSpec 3 :P

cupakromer · 2014-08-25T16:23:14Z

Since we follow SemVer, we can't drop support for any rails versions until rspec-rails 4.x...unless we decide that SemVer only applies to the other RSpec libs and doesn't apply to rspec-rails.

I would prefer we keep rspec-rails following SemVer. It can be a bit of a pain at times, but it makes it easy for people to upgrade / update knowing we do this.

if it is not a big maintenance burden

I'm on the fence here. For the most part no, it has not been. However, there are some issues, such as #1025, where this actually was an extreme pain.

many, many apps out there on Rails 3.0, 3.1, 3.2 and 4.0...and those apps may want to run the latest RSpec. I think that we do the ruby community a great service by keeping rspec-rails compatible with so many versions of rails

I agree with this for the most part. I think with there having been such a long life of RSpec 2.x that this was / is a great benefit. Moving forward, I'm going to have to think a lot about this for 4.x.

maybe we can ask some of the companies that have access to Gemfiles from many real apps (e.g. newrelic, github, travis) if they can provide us with this data from their datasets

I would be very interested in seeing some data.

To keep semver, that's maybe a pretty good argument for decoupling rspec-rails version numbers from rspec-core.

Interesting idea. If we want to drop support for older versions of rails before we're ready for RSpec 4, we should probably do this.

I'm not sure how I feel about this. rspec-rails is in a very interesting position being so tightly coupled with two major libs. I don't think changing things now that we've release 3.x is a good idea, but I'm open to ideas for 4.x. This is something else I'll have to think a bit about.

myronmarston · 2014-08-25T16:40:35Z

I would be very interested in seeing some data.

Let me know if you decide to pursue getting that kind of data and want some help from me in doing so.

JonRowe · 2014-08-25T21:04:33Z

I really don't want us to abandon semver, I'd rather we decouple rspec-rails from rspec than do that. Just my 2¢.

Since we are binstubbing things in general, it's a good idea to actually try them out when using the generators in the example apps.

More people are not using ActiveRecord these days. The generators should not include any configs related to `ActiveRecord` when it is not available. This allows rspec configurations to work properly out-of-the- box for these other apps.

Creates a 2nd set of generator and smoke tasks which target an example app which does not use ActiveRecord. In order to accomplish this, a custom in-memory model is used which supports enough of the AR protocol to allow default specs to pass. Use a custom "in-memory" model as the ORM replacement. This is a simple stand-in allowing us to continue to use all of the other Rails generators. It also prevents us from needing to pull in another ORM gem such as such as Mongoid, DataMapper, ROM, etc. The custom "model" is necessary as the non-ActiveRecord example app's specs fail with `NameError`s. This is due to the other Rails' generators expecting to have a "model" type class available, but Rails is unable to load it. Just adding the classes does not work as the generated controllers, form helpers, rendering process, etc. are so tightly coupled to the `ActiveModel` API interface that all sorts of things start failing.

Change the directory name from "templates" to "example_app_generator" to better describe what this directory holds. It is both custom Rails templates, as well as, generator files.

To help future proof changes in Rails, toggle on feature checks instead of the Rails version. Use this in the specs as well. Simplify the case where `ActiveRecord` is not available, by checking for all options at onice instead of trying to toggle on a feature or Rails version.

Remove `ActiveRecord` based settings from generated `rails_helper`

cupakromer force-pushed the remove-fixtures-on-no-ar branch 4 times, most recently from 42b00cd to 04eadc4 Compare August 22, 2014 17:37

myronmarston reviewed Aug 22, 2014
View reviewed changes

xaviershay reviewed Aug 23, 2014
View reviewed changes

cupakromer mentioned this pull request Aug 25, 2014

[Generators] ActiveJob #1155

Merged

cupakromer force-pushed the remove-fixtures-on-no-ar branch from 04eadc4 to cc71f61 Compare August 25, 2014 15:37

cupakromer added 6 commits August 28, 2014 18:42

Binstub the generators which shellout.

a6f7002

Since we are binstubbing things in general, it's a good idea to actually try them out when using the generators in the example apps.

Exclude ActiveRecord configs when not available.

6d4aedf

More people are not using ActiveRecord these days. The generators should not include any configs related to `ActiveRecord` when it is not available. This allows rspec configurations to work properly out-of-the- box for these other apps.

Move example app geneators.

c8a7ad7

Change the directory name from "templates" to "example_app_generator" to better describe what this directory holds. It is both custom Rails templates, as well as, generator files.

Note new behavior in Changelog.

9a49bc4

cupakromer force-pushed the remove-fixtures-on-no-ar branch from adda299 to da6a388 Compare August 29, 2014 00:58

cupakromer added 2 commits August 28, 2014 21:04

Include acceptance smoke task for job generator.

49c8a30

Fix constant reference.

cf2ef07

cupakromer force-pushed the remove-fixtures-on-no-ar branch from da6a388 to cf2ef07 Compare August 29, 2014 01:06

cupakromer added a commit that referenced this pull request Aug 29, 2014

Merge pull request #1150 from rspec/remove-fixtures-on-no-ar

953750d

Remove `ActiveRecord` based settings from generated `rails_helper`

cupakromer merged commit 953750d into master Aug 29, 2014

cupakromer deleted the remove-fixtures-on-no-ar branch August 29, 2014 04:15

cupakromer mentioned this pull request Sep 21, 2014

Add in an active_record_helper #1090

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `ActiveRecord` based settings from generated `rails_helper` #1150

Remove `ActiveRecord` based settings from generated `rails_helper` #1150

cupakromer commented Aug 21, 2014

myronmarston Aug 22, 2014

cupakromer Aug 22, 2014

myronmarston Aug 22, 2014

cupakromer Aug 22, 2014

myronmarston commented Aug 22, 2014

xaviershay Aug 23, 2014

xaviershay commented Aug 23, 2014

xaviershay commented Aug 23, 2014

myronmarston commented Aug 23, 2014

JonRowe commented Aug 23, 2014

cupakromer commented Aug 25, 2014

myronmarston commented Aug 25, 2014

JonRowe commented Aug 25, 2014

Remove ActiveRecord based settings from generated rails_helper #1150

Remove ActiveRecord based settings from generated rails_helper #1150

Conversation

cupakromer commented Aug 21, 2014

myronmarston Aug 22, 2014

Choose a reason for hiding this comment

cupakromer Aug 22, 2014

Choose a reason for hiding this comment

myronmarston Aug 22, 2014

Choose a reason for hiding this comment

cupakromer Aug 22, 2014

Choose a reason for hiding this comment

myronmarston commented Aug 22, 2014

xaviershay Aug 23, 2014

Choose a reason for hiding this comment

xaviershay commented Aug 23, 2014

xaviershay commented Aug 23, 2014

myronmarston commented Aug 23, 2014

JonRowe commented Aug 23, 2014

cupakromer commented Aug 25, 2014

myronmarston commented Aug 25, 2014

JonRowe commented Aug 25, 2014

Remove `ActiveRecord` based settings from generated `rails_helper` #1150

Remove `ActiveRecord` based settings from generated `rails_helper` #1150