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

Fix Rails environment when running tests with Ruby #31355

Merged
merged 4 commits into from
Dec 8, 2017

Conversation

tenderlove
Copy link
Member

I frequently run tests with ruby, not with a runner like rake or
rails. When running the test with just ruby the RAILS_ENV
environment variable did not get set to "test", and this would cause the
tests to fail (and even mutate the development database!)

This commit adds integration tests for running tests with just ruby
and ensures the environment gets defaulted to "test". I also added a
test to ensure that passing an environment to -e actually works (and
fixed that case too).

An interesting / annoying thing is that Minitest picks up it's plugins
by asking RubyGems for a list of files:

https://github.com/seattlerb/minitest/blob/ca6a71ca901016db09a5ad466b4adea4b52a504a/lib/minitest.rb#L92-L100

This means that RubyGems needs to somehow know about the file before it
can return it to Minitest. Since we are not packaging Rails as a Gem
before running the integration tests on it (duh, why would you do
that?), RubyGems doesn't know about the file, so it can't tell Minitest,
so Minitest doesn't automatically require it. This means I had to
manually require and insert the plugin in our integration test. I've
left comments about that in the test as well.

Ugh.

time: result.time)
end
end

Copy link
Member

Choose a reason for hiding this comment

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

There are trailing whitespaces here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I saw the complaints, but Vim didn't show for some reason. 😀

Minitest.reporter.reporters.clear
Minitest.reporter.reporters << JSONReporter.new
end

Copy link
Member

Choose a reason for hiding this comment

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

There are trailing whitespaces here.

Copy link
Member

@rafaelfranca rafaelfranca left a comment

Choose a reason for hiding this comment

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

Oh. That explains it!

I frequently run tests with `ruby`, not with a runner like `rake` or
`rails`.  When running the test with just `ruby` the `RAILS_ENV`
environment variable did not get set to "test", and this would cause the
tests to fail (and even mutate the development database!)

This commit adds integration tests for running tests with just `ruby`
and ensures the environment gets defaulted to "test".  I also added a
test to ensure that passing an environment to `-e` actually works (and
fixed that case too).

An interesting / annoying thing is that Minitest picks up it's plugins
by asking RubyGems for a list of files:

  https://github.com/seattlerb/minitest/blob/ca6a71ca901016db09a5ad466b4adea4b52a504a/lib/minitest.rb#L92-L100

This means that RubyGems needs to somehow know about the file before it
can return it to Minitest.  Since we are not packaging Rails as a Gem
before running the integration tests on it (duh, why would you do
that?), RubyGems doesn't know about the file, so it can't tell Minitest,
so Minitest doesn't automatically require it.  This means I had to
manually require and insert the plugin in our integration test.  I've
left comments about that in the test as well.

Ugh.
Copy link
Member

@eileencodes eileencodes left a comment

Choose a reason for hiding this comment

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

I approved this and then actually tested it (sorry 😞 ) and it's didn't fix the issue on my repo. I tried creating a new one in case I messed something up and the ENV when running with Ruby is still development and not test.

@Edouard-chin
Copy link
Member

Ah doesn't work for me either :(. I think it's already too late to set the ENV side the runner when using ruby or even rake because we already loaded the gems Bundler.require(*Rails.groups) (Rails.groups, make a call to Rails.env which memoize 'development')

I think we can add ENV["RAILS_ENV"] || 'test' after this condition 🤷‍♂️

if defined?(Rake.application) && Rake.application.top_level_tasks.grep(/^(default$|test(:|$))/).any?
ENV["RAILS_ENV"] ||= Rake.application.options.show_tasks ? "development" : "test"
end

@tenderlove
Copy link
Member Author

Ah, so this patch is setting the ENV correctly, it's just too late. sigh

@Edouard-chin I think we used to have something like that. We really need a test for this case though because the behavior breaks too often for my liking 😉

When tests are run with just `ruby`, the RAILS_ENV is set to
`development` too early, and we connect to the development database
rather than the test database.
Copy link
Member

@eileencodes eileencodes left a comment

Choose a reason for hiding this comment

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

👍 looks good to me

Option parsing happens too late to have any impact on the Rails
environment.  Rails accesses the environment name and memoizes it too
early in the boot process for a commandline option to have any impact on
the database connection, so we'll change this test to set the
environment from an environment variable (and ensure it still works when
running tests with `ruby`)
@tenderlove tenderlove merged commit bc2e080 into master Dec 8, 2017
@tenderlove tenderlove deleted the fix-rails-env-with-ruby branch December 8, 2017 22:55
tenderlove added a commit that referenced this pull request Dec 8, 2017
Fix Rails environment when running tests with Ruby
Copy link
Contributor

@kaspth kaspth left a comment

Choose a reason for hiding this comment

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

❤️

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.

None yet

5 participants