cachable: make sure to clear caches between tests #16746

apainintheneck · 2024-02-26T01:55:01Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew typecheck with your changes locally?
Have you successfully run brew tests with your changes locally?

We've been seeing some difficult to debug test failures recently related to not clearing some caches between tests. Normally we use the Cachable mixin to facilitate caching in classes and modules. I thought it'd be nice to automatically clear those caches between tests without having to create a list manually of all of the classes and modules that use this mixin.

This adds a registry for all modules and classes that cachable is included in. The registry allows us to programmatically clear all caches in between tests so that we don't forget to do that when adding a new class or refactoring code. The goal here is to reduce the number of flaky tests in the future. We only fill this registry ~~when HOMEBREW_TESTS is set~~ in the testing environment.

Marking draft to wait for discussion in #16671 since this will depend on that.

This adds a registry for all modules and classes that cachable is included in. The registry allows us to programmatically clear all caches in between tests so that we don't forget to do that when adding a new class or refactoring code. The goal here is to reduce the number of flaky tests in the future.

reitermarkus · 2024-02-26T11:40:21Z

Library/Homebrew/api/formula.rb


+      class << self


Might as well use private_class_method and move everything to def self.* here.

Can be done in a follow-up PR though.

I didn't think that'd work but it seems fine. Good idea!

Sorry, I got confused. Yeah, I'll update that too.

Might not be worth killing the git blame though. I'll do it anyway and see if anyone cares.

Addressed in bea2dc6

reitermarkus · 2024-02-26T11:50:42Z

Library/Homebrew/extend/cachable.rb

+  # Collect all classes that mix in Cachable so that those caches can be cleared in-between tests.
+  if ENV["HOMEBREW_TESTS"]
+    def self.included(klass)
+      raise ArgumentError, "Don't use Cachable with singleton classes" if klass.singleton_class?


Why does this need to be special-cased/forbidden?

Can we not call klass.extend(self) in here, or is that a different scope?

I don't think there is a method to get the original class from the singleton class. Forcing classes to extend this module instead of including it in the singleton class avoids that problem and is functionally equivalent.

Library/Homebrew/extend/cachable.rb

reitermarkus · 2024-02-26T12:06:16Z

Library/Homebrew/extend/cachable.rb

@@ -11,4 +11,43 @@ def cache
  def clear_cache
    cache.clear
  end
+
+  # Collect all classes that mix in Cachable so that those caches can be cleared in-between tests.
+  if ENV["HOMEBREW_TESTS"]


I wonder, could we implement this as a test helper with allow(Cachable).to receive(:extended)?

I don't think so. It needs to be set up before other classes start mixing in this module. I thought of another way of doing this where we load a separate file in test/support/extend/ with these cachable methods before loading global.rb and then we can be sure that this only gets loaded in the test environment.

See 5cc1c85

apainintheneck · 2024-02-27T04:33:01Z

I'm marking this as ready for review since it no longer changes any of the same files as #16671 and should be useful whatever we decide to do there.

Library/Homebrew/extend/cachable.rb

Now we only include the cachable registry when running tests. We basically just load it first and add a bunch of methods to it before loading the rest of the formula files when we require global.rb. I added a check to make sure this require order is preserved. I also made a bunch of methods private, stop excluding classes that inherit from casks since it's unnecessary and add more docs.

These were changed to extend to make it easier to determine where the classes come to in the extended callback but that means that the file is somewhat inconsistent. On the one hand we're using class methods and on the other we're extend self. This cleans that up but now the diff is atrocious and the blame is even worse. Oh well...

MikeMcQuaid

Good idea, thanks again @apainintheneck!

reitermarkus · 2024-02-27T09:20:48Z

Library/Homebrew/test/spec_helper.rb

@@ -202,16 +204,7 @@
  config.around do |example|
    Homebrew.raise_deprecation_exceptions = true

-    Formulary.clear_cache
-    Tap.each(&:clear_cache)


@apainintheneck, this needs to stay.

Is it really necessary to clear taps at the instance level if we're already clearing Tap, CoreTap and CoreCaskTap at the class level? I guess .clear_cache doesn't clear the core tap instances, right?

That is the @instance instance variable.

The core taps exist outside of the normal cache busting cycle so they need to clear explicitly at the instance level. Just to be sure we should clear all of them each time. This essentially reverts part of the change in this PR. - #16746

reitermarkus reviewed Feb 26, 2024

View reviewed changes

Library/Homebrew/extend/cachable.rb Outdated Show resolved Hide resolved

reitermarkus reviewed Feb 26, 2024

View reviewed changes

apainintheneck marked this pull request as ready for review February 27, 2024 04:32

apainintheneck requested a review from reitermarkus February 27, 2024 04:32

apainintheneck commented Feb 27, 2024

View reviewed changes

Library/Homebrew/extend/cachable.rb Outdated Show resolved Hide resolved

apainintheneck added 2 commits February 26, 2024 20:41

apainintheneck force-pushed the simplify-cache-clearing-in-tests branch from 909828c to bea2dc6 Compare February 27, 2024 05:23

MikeMcQuaid approved these changes Feb 27, 2024

View reviewed changes

MikeMcQuaid merged commit ca9405d into Homebrew:master Feb 27, 2024
26 checks passed

reitermarkus reviewed Feb 27, 2024

View reviewed changes

apainintheneck mentioned this pull request Feb 28, 2024

test: clear tap instance caches between test runs again #16760

Merged

7 tasks

github-actions bot added the outdated PR was locked due to age label Mar 30, 2024

github-actions bot locked as resolved and limited conversation to collaborators Mar 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cachable: make sure to clear caches between tests #16746

cachable: make sure to clear caches between tests #16746

apainintheneck commented Feb 26, 2024 •

edited

Loading

reitermarkus Feb 26, 2024

reitermarkus Feb 26, 2024

apainintheneck Feb 27, 2024

apainintheneck Feb 27, 2024

apainintheneck Feb 27, 2024

apainintheneck Feb 27, 2024

reitermarkus Feb 26, 2024

reitermarkus Feb 26, 2024 •

edited

Loading

apainintheneck Feb 27, 2024

reitermarkus Feb 26, 2024 •

edited

Loading

apainintheneck Feb 27, 2024

apainintheneck Feb 27, 2024 •

edited

Loading

apainintheneck commented Feb 27, 2024

MikeMcQuaid left a comment

reitermarkus Feb 27, 2024

apainintheneck Feb 28, 2024

apainintheneck Feb 28, 2024

apainintheneck Feb 28, 2024

cachable: make sure to clear caches between tests #16746

cachable: make sure to clear caches between tests #16746

Conversation

apainintheneck commented Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reitermarkus Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reitermarkus Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apainintheneck Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

apainintheneck commented Feb 27, 2024

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apainintheneck commented Feb 26, 2024 •

edited

Loading

reitermarkus Feb 26, 2024 •

edited

Loading

reitermarkus Feb 26, 2024 •

edited

Loading

apainintheneck Feb 27, 2024 •

edited

Loading