This repository has been archived by the owner on Apr 14, 2021. It is now read-only.
Better test specific file searching #7166
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
deivid-rodriguez
force-pushed
the
better_test_specific_file_searching
branch
3 times, most recently
from
b2aa543
to
2bb86b5
Compare
|
This is now passing but I was getting a spec failure and 1f1162d fixed and I have no idea why. So I'd like to investigate that before asking for reviews. |
deivid-rodriguez
force-pushed
the
better_test_specific_file_searching
branch
2 times, most recently
from
6f299ca
to
040186d
Compare
|
I investigated the issues and finally decided to revert #7138, because I couldn't find a good solution that does not cause issues. |
|
Oh, this might fix #7169, by the way. |
I tried monkeypatching the gemfile finder methods in hax.rb, but that resulted in loading parts of bundler too early, and getting redefinition warnings when loading them again from exe/bundle, resulting in actual errors in some cases. The fix for that required eagerly loading the monkeypatched parts of bundler, but that involves changing lib just for the sake of making existing tests pass, and that's what the original changes intend to avoid. So, I'm just reverting to the previous approach of setting an environment variable just for testing and check that. Reverts 11597fe.
deivid-rodriguez
force-pushed
the
better_test_specific_file_searching
branch
from
040186d
to
d5729d2
Compare
|
This fixes several failures when testing locally on systems with a per-user bundler configuration, so I'm merging it. We can revisit if we find a solution that does not run into those issues. @bundlerbot r+ |
ghost
pushed a commit
that referenced
this pull request
May 16, 2019
7166: Better test specific file searching r=deivid-rodriguez a=deivid-rodriguez ### What was the end-user problem that led to this PR? The problem was that in #7138 I removed some test specific code from lib, but it's not quite working. ### What was your diagnosis of the problem? My diagnosis was that the mocking I used to fix the issue of bundler looking for files outside of bundler's clone doesn't work inside subprocesses, which many of our specs use. That means some specs were currently failing if you have Gemfile's on `.bundle` folders up in the directory hierarchy ascending from your bundler's clone ### What is your fix for the problem, implemented in this PR? My fix is revert to the previous approach of using a test specific environment variable to prevent search up higher than the test directory when running our specs. ### Why did you choose this fix out of the possible options? I initially tried to monkeypatch the gemfile finding helpers only inside specs, but that caused load order issues (I monkeypatched the helpers insde `support/hax.rb`, that caused the monkeypatched lib code to be loaded _before_ the rest of `bundler`, which caused issues in some specs). Overall, the old approach is the only one that works without issues, so I'm reverting to it, at least for now. Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
Build succeeded |
ghost
merged commit
This pull request was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
What was the end-user problem that led to this PR?
The problem was that in #7138 I removed some test specific code from lib, but it's not quite working.
What was your diagnosis of the problem?
My diagnosis was that the mocking I used to fix the issue of bundler looking for files outside of bundler's clone doesn't work inside subprocesses, which many of our specs use. That means some specs were currently failing if you have Gemfile's on
.bundlefolders up in the directory hierarchy ascending from your bundler's cloneWhat is your fix for the problem, implemented in this PR?
My fix is revert to the previous approach of using a test specific environment variable to prevent search up higher than the test directory when running our specs.
Why did you choose this fix out of the possible options?
I initially tried to monkeypatch the gemfile finding helpers only inside specs, but that caused load order issues (I monkeypatched the helpers insde
support/hax.rb, that caused the monkeypatched lib code to be loaded before the rest ofbundler, which caused issues in some specs).Overall, the old approach is the only one that works without issues, so I'm reverting to it, at least for now.