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

Safer tests by not mutating current process #918

Merged
merged 2 commits into from
Sep 12, 2019
Merged

Conversation

schneems
Copy link
Contributor

@schneems schneems commented 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

@schneems schneems force-pushed the schneems/safer-tests branch from 2f8bc2d to 95968bd Compare September 11, 2019 18:04
@schneems schneems force-pushed the schneems/safer-tests branch 2 times, most recently from 31a02d6 to 19d760d Compare September 11, 2019 18:51
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
@schneems
Copy link
Contributor Author

Waiting on compliance before I rebase and re-run tests since I have a lot of commits waiting to be merged and every time I merge one I have to update the branch and re-run tests on all of them. This PR needs to be prioritized since it affects the stability of tests

@schneems schneems merged commit e9ded87 into master Sep 12, 2019
@schneems schneems deleted the schneems/safer-tests branch September 12, 2019 16:30
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