postmodern / chruby

Changes the current Ruby
MIT License
2.85k stars 190 forks source link

Make tests not depend on any hardcoded version #420

Closed eregon closed 4 years ago

eregon commented 4 years ago

This for instance makes it possible to run tests locally with a Ruby version newer than 2.2 since that might be difficult to install on recent systems.

I tried with ruby-install -r $PWD/test/opt/rubies ruby 2.6.5 and then modifying test_ruby_version in test/helper.sh is all that's needed.

eregon commented 4 years ago

@postmodern Could you merge this?

eregon commented 4 years ago

@postmodern @havenwood Could we merge this?

havenwood commented 4 years ago

This looks good to me.

postmodern commented 4 years ago

Great idea. I'm just worried that the presence of .ruby-version in the project dir might mess with some (or future) tests. Probably safer to use a global TEST_RUBY_VERSION env variable that gets passed in.

test_ruby_version="${TEST_RUBY_VERSION:-2.2.5}"

Thoughts?

eregon commented 4 years ago

@postmodern I removed /test/project/.ruby-version in this PR, and it's now generated automatically from $test_ruby_version. The point of that test is to test behavior chruby-auto with a .ruby-version file, so we need it.

We could remove the file automatically after the test though, I'll push a commit to do that.

postmodern commented 4 years ago

@eregon if you're wanting to add additional chruby_auto tests, then I'd recommend making a sub-directory within test/project/.

I should probably rename the directory to test/projects, since each sub-directory represents a different scenario.

eregon commented 4 years ago

I pushed a commit for removing the file, so it does not need to be excluded in .gitignore. I think the PR is good to merge now, it makes it easier to run tests without changing any test behavior.

eregon commented 4 years ago

@postmodern Hello, could you merge this?

postmodern commented 4 years ago

I'm working on a branch that allows you to override all the test_ruby_*/test_gem_* variables via TEST_RUBY_*/TEST_GEM_* variables, and then auto-generate the .ruby-version files in a oneTimeSetUp function (shunit2's global setup function). Also probably rename a few of those test/project/ directories to be more descriptive...

eregon commented 4 years ago

@postmodern Sounds good. Could you still merge this first? It's one step in the right direction.

postmodern commented 4 years ago

Would there be value in overriding test_ruby_root with something outside of test/opt/rubies/ entirely? If so, I'll have to add some short-circuit logic around test/setup.

eregon commented 4 years ago

I think the current value:

test_ruby_root="$PWD/test/opt/rubies/$test_ruby_engine-$test_ruby_version"

is good enough and I didn't need to change it to make tests pass locally.

postmodern commented 4 years ago

OK I finally finished my branch which implements the same functionality, but with improved control of file fixtures. https://github.com/postmodern/chruby/compare/master...test_ruby_version Does that look satisfactory, or am I missing anything?

eregon commented 4 years ago

Looks good to me.

From a contribution point of view, I would have found it nicer to merge this PR first and improve upon it after merge. I spent some time to do this and I think it was a worthwhile change on its own. Anyway I'll just close this then since there is no point to conflict with your commit. Please open a PR with your branch or push it to master then.

postmodern commented 4 years ago

Merged. Yeah, I hate to reject PRs, but I also wanted a more complete solution that could be expanded upon.

eregon commented 4 years ago

The tests still fail if I have test/opt/rubies/ruby-2.6.5 due to multiple places hardcoding the version to an ancient Ruby 2.2 :disappointed: I'll make a new PR, and I hope this time you merge it.

Apologies, I was on an older branch, master works better, although tests still fail for me locally.

eregon commented 4 years ago

@postmodern https://github.com/postmodern/chruby/pull/435 for the fixes needed to make the tests pass locally.