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

Don't load system rubygems at all when RGV specified for tests #3595

Merged
merged 11 commits into from
May 6, 2020

Conversation

deivid-rodriguez
Copy link
Member

Description:

In #3592 I noticed that "system rubygems" is being loaded by our tests. I think this is fine if we don't specify ENV["RGV"] but if we do, rubygems should always be picked up from there.

This PR tries to implement that.

Tasks:

  • Describe the problem / feature
  • Write tests
  • Write code to solve the problem
  • Get code review from coworkers / friends

I will abide by the code of conduct.

@bundlerbot bundlerbot added Bundler CI CI related issues labels May 4, 2020
@deivid-rodriguez deivid-rodriguez marked this pull request as draft May 4, 2020 13:20
@deivid-rodriguez deivid-rodriguez force-pushed the dont_load_system_rubygems_at_all branch 3 times, most recently from 54778ec to 1ca285d Compare May 5, 2020 12:52
Since it doesn't seem preserved by default.
This list of exceptions is no longer rescued since
1f03275.
I have no idea why, but when trying to change this spec so that the
environment is passded more `RUBYOPT`'s, it starts failing complaining
that no inline code was passed to `ruby -e`. I presume it was some
escaping issue, so I rewrote it in a simpler way which gives no issues.
This spec is very fragile to changes in `RUBYOPT`. I rewrote it to test
just was the spec is trying to verufy, namely, that `-rbundler/setup` is
not added twice to `RUBYOPT`.
This code was added in
rubygems/bundler@a1202ef
just for the sake of passing tests. However, it's not needed these days.
And it's also causing system rubygems to be unitentionally loaded on
ruby 2.3 for some reason.
@deivid-rodriguez deivid-rodriguez force-pushed the dont_load_system_rubygems_at_all branch from 1ca285d to ab2e43d Compare May 5, 2020 14:37
@deivid-rodriguez deivid-rodriguez marked this pull request as ready for review May 5, 2020 16:48
@deivid-rodriguez
Copy link
Member Author

Yay!

Bundler tested against a specific rubgems version (setting ENV["RGV"]) no longer load system rubygems whatsover. This has the following advantages:

  • Makes tests more isolated from the environment where they are run.
  • Hopefully gets Refactor binstub specs #3592 green, since the failures there seem to indicate leakages to the system bundler.
  • Checking for this uncovered at least two places that were leaking to either system bundler or system rubygems. This code is at the least unneeded, and maybe causes bugs, so I'm killing it in 4eb4ebe and 6e71e7b.

The test I added involve an at_exit hook, which is not super pretty but much better than the initial solution I tried: rm -rf <system_rubygems> 😆.

@deivid-rodriguez
Copy link
Member Author

Merging this. Happy to address comments/concerns on followup PRs!

@deivid-rodriguez deivid-rodriguez merged commit b307908 into master May 6, 2020
@deivid-rodriguez deivid-rodriguez deleted the dont_load_system_rubygems_at_all branch May 6, 2020 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bundler CI CI related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants