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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MarkDBlackwell
Copy link

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
Copy link
Member

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
Copy link
Author

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

@MarkDBlackwell
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants