-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Make Tap::each
respect the API and clear all tap caches before each test.
#16710
Make Tap::each
respect the API and clear all tap caches before each test.
#16710
Conversation
Tap::each
respect the API and clear all tap caches before each test.
Library/Homebrew/test/spec_helper.rb
Outdated
@@ -260,6 +261,7 @@ | |||
@__stdin.close | |||
|
|||
Formulary.clear_cache | |||
Tap.each(&:clear_cache) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like without modifying Tap.each
you could do this instead or something similar. I have not tested this at all though.
Tap.each(&:clear_cache) | |
Tap.each(&:clear_cache) | |
CoreTap.clear_cache | |
CoreCaskTap.clear_cache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it will work, but it doesn't make sense for CoreTap
and CoreCaskTap
not to already be included in Tap.each
. In fact, they may actually be included, depending on whether the tap directory exists in tests, which is even weirder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should actually be done before the ENV
is restored above since this depends on HOMEBREW_NO_INSTALL_FROM_API
.
bb2f78c
to
a320b9f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be worth just making the simple change here (#16710 (comment)) and then discuss how we want Tap.each
to behave in a separate PR. It looks like this will change user facing behavior and it's a lot to review in one PR since it's used in a bunch of different places.
Needs rebased now but looks good so far! |
a320b9f
to
acacb47
Compare
|
acacb47
to
6b0a376
Compare
6b0a376
to
6a8ea0a
Compare
6a8ea0a
to
9cfc7ef
Compare
This helped solve one of the test problems in #16638. Thanks for catching this, @reitermarkus! |
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?Ensure that
Tap::each
includesCoreTap
andCoreCaskTap
when using the API. This ensures all caches can be cleared andTap::reverse_tap_migrations_renames
works correctly when these taps are not installed locally.More general solution than #16680, which added this before one specific test.