RailsApps / rails3-devise-rspec-cucumber

An example Rails 3.2 app with Devise and RSpec and Cucumber.
http://railsapps.github.io/
445 stars 144 forks source link

Prevent occasional wrong database loss from tests when RAILS_ENV is preset (preventive measure) #44

Open MarkDBlackwell opened 11 years ago

MarkDBlackwell commented 11 years ago

Please see Rails issue 7175 for some discussion of this problem.

ENV["RAILS_ENV"] = 'test'

Why do I suggest changing this to '='?:

Because in some cases and with some software, the environment variable RAILS_ENV may be predefined with some value. Then, if it is '||=', running the tests will fail to change that value to 'test', because it already exists. This actually forces the initial database-dropping step to destroy the database of whatever environment RAILS_ENV was predefined to. This may be 'development' or 'production'. Testing normally makes a fresh, empty 'test' environment database during testing.

Why does Rspec have it as '||='?:

This could be motivated by continuous integration servers running tests in a special Rails environment. My source for the possibility is this comment. However, to implement continuous integration, pre-setting RAILS_ENV (externally) is unnecessary. Or if desired, the line above can be changed specifically to check for the value 'CI' (or some such), and not just allow any values to pass through, such as 'development'.

Dave Chemlinsky said here he didn't want "to force it to 'test' because that ties everybody's hands." However, if we just want to ensure that RailsApps users are in the 'test' environment whenever they run tests, no one else will be hurt by that.

When running specs (which require spec/spec_helper.rb), one never would want to use any Rails environment other than 'test', right? So, we might as well set it definitely to 'test'.

Without this, under certain circumstances, RailsApps users might suffer the slight frustration of losing their development database, or perhaps worse. I have experienced this loss, BTW.

For instance, the Foreman gem from Heroku's toolbelt includes general functionality to set environment variables, potentially including RAILS_ENV, by following user-defined directives from a file (.env). It includes a general run command (see Foreman's man page) which might possibly be used to run an app's test suite: foreman run rake. And, other software exists for managing Rails development. All have the possibility of this frustrating development database loss during testing, when we easily can afford this protection.

DanielKehoe commented 11 years ago

I applaud the thorough research into a potentially troublesome issue. This really is an issue for the RSpec community to resolve as they have more knowledge of the rationale for the current implementation and ramifications of making this change. Could you open an RSpec issue and initiate further discussion? I'll leave this issue open pending further discussion. I'm reluctant to make the change without seeing a consensus of opinion from RSpec users because I don't want to change the RSpec recommended default in case there are unforeseen consequences.

MarkDBlackwell commented 11 years ago

I see by this recent commit that Rails in its normal test infrastructure doesn't protect us from this problem either.

MarkDBlackwell commented 11 years ago

Beyond the RSpec community (gem, rspec-rails) community, this seems to involve other testing methods and communities.

Considering RailsApps' various kinds of users to include those new to Rails, and after rereading "Who This Is For" in the docs, supplementing new users with more protection than Rails and RSpec normally provide on this small frustration issue could perhaps be found appropriate after discussion.

Naturally, I haven't seen any negative consequences.

Could lacking this protection to new users be a consequence of an "I am an expert; I don't need it" way of thinking? Yet aren't Rails and RailsApps currently expanding to a broader base of programmers including more new users?

So, I revert this issue to the state you left it. And it's okay with me if you decide to close it.