Safer tests by not mutating current process #65
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
schneems
force-pushed
the
schneems/in-directory-safe
branch
from
7e7c247
to
fa5fa64
Compare
If you don't know if code will mutate a process information, such as environment variables then run it in a fork to ensure it does not affect the parent process.
schneems
force-pushed
the
schneems/in-directory-safe
branch
from
fa5fa64
to
8185c21
Compare
schneems
added a commit
to heroku/heroku-buildpack-ruby
that referenced
this pull request
Sep 11, 2019
If you don't know if code will mutate a process information, such as environment variables then run it in a fork to ensure it does not affect the parent process.
Currently when you run any `LanguagePack::<Klass>.use?` such as `LanguagePack::Rails6.use?` it will mutate the environment by loading in bundler and modifying the LOAD_PATH.
This happens because the `use?` method relies on bundler:
```
def self.use?
instrument "rails6.use" do
rails_version = bundler.gem_version('railties')
return false unless rails_version
is_rails = rails_version >= Gem::Version.new('6.x') &&
rails_version < Gem::Version.new('7.0.0')
return is_rails
end
end
```
Which loads and initializes this wrapper:
```
def self.bundler
@@bundler ||= LanguagePack::Helpers::BundlerWrapper.new.install
end
```
And the wrapper install method mutates globals:
```
def install
ENV['BUNDLE_GEMFILE'] = @gemfile_path.to_s
fetch_bundler
$LOAD_PATH << @path
require "bundler"
self
end
```
This is an issue because other tests such as the rake_runner_spec tests expect a specific version of Rake to be available however if the load path is mutated and bundler is required then only one specific version of rake will be available on the system.
While I _believe_ that this fixes some flappy tests, I'm not totally sure why it only fails sometimes. You would think that it would either always pass or always fail for a given order. This test order is taken from a failing test run:
```
$ cat << EOL > log/test_order_fails_sometimes.log
./spec/helpers/rake_runner_spec.rb[1:2]
./spec/helpers/shell_spec.rb[1:2:1]
./spec/helpers/jvm_installer_spec.rb[1:4]
./spec/installers/yarn_installer_spec.rb[1:1:1:1]
./spec/installers/heroku_ruby_installer_spec.rb[1:1:1]
./spec/helpers/rake_runner_spec.rb[1:4]
./spec/hatchet/stack_spec.rb[1:1]
./spec/hatchet/rails6_spec.rb[1:1]
./spec/hatchet/jvm_spec.rb[1:1]
./spec/helpers/rake_runner_spec.rb[1:1]
./spec/helpers/jvm_installer_spec.rb[1:6]
./spec/helpers/jvm_installer_spec.rb[1:1]
./spec/hatchet/bugs_spec.rb[1:3:1]
./spec/hatchet/bundler_spec.rb[1:1]
./spec/installers/heroku_ruby_installer_spec.rb[1:1:2:1]
./spec/hatchet/ruby_spec.rb[1:4:2:1]
./spec/hatchet/rubies_spec.rb[1:3]
./spec/hatchet/node_spec.rb[1:2]
./spec/hatchet/getting_started_spec.rb[1:1]
EOL
$ while bundle exec rspec-queue --queue log/test_order_fails_sometimes.log --build 1 --worker 2; do :; done
```
Test output link https://dashboard.heroku.com/pipelines/ac057663-170b-4bdd-99d0-87560eb3a570/tests/519
This PR possible to the feature added in heroku/hatchet#65
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sometimes in a test you don't know of a bit of code will modify global state such as environment variables. To be extra safe we can run these actions inside of a fork.
Here's an example of some tests that were accidentally mutating the current env heroku/heroku-buildpack-ruby#918.