Simplify Formula#outdated_versions logic.

[vladshablinsky]

• 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 tests with your changes locally?

---

Description

Replace complicated logic from Formula#outdated_versions with simpler equivalent and add tests.

What outdated_versions used to do:

     1  all_versions = []
     2  older_or_same_tap_versions = []
     3  
     4  installed_kegs.each do |keg|
     5    version = keg.version
     6    all_versions << version
     7    older_version = pkg_version <= version
     8  
     9    tab_tap = Tab.for_keg(keg).tap
    10    if tab_tap.nil? || tab_tap == tap || older_version
    11      older_or_same_tap_versions << version
    12    end
    13  end
    14  
    15  if older_or_same_tap_versions.all? { |v| pkg_version > v }
    16    all_versions.sort!
    17  else
    18    []
    19  end

Suppose we have f -- an instance of Formula.

1. Go through all the _keg_s of installed under the f.name (line 4)
2. (line 10)

• If keg's tap was nil and if f.pkg_version was less or equal keg's version then at the end of outdated_versions [] used to be returned.
• If keg's was same as f.tap, then if pkg_version was less or equal keg's version then at the end of outdated_versions [] used to be returned.
• If keg's tap was not the same as f.tap we checked if f.pkg_version was less or equal to keg's version (line 7) and then after older_or_same_tap_versions was formed f.pkg_version was never greater than keg's version (line 15), so we returned [].
• (no conditions from line 10 satisfied) If keg's tap was not the same as f.tap we checked if f.pkg_version was greater then keg's version we didn't add keg's version to older_or_same_tap_versions.

So, [] was returned if and only if f.pkg_version less or equal for at least one of the kegs or f.rack was empty.

The PR suggests that we return [] as soon as we are sure the previously used older_or_same_tap_versions cannot satisfy older_or_same_tap_versions.all? { |v| pkg_version > v }.

Old Formula#outdated_versions implementation passes all the added outdated_versions tests.

[vladshablinsky]

@xu-cheng, what do you think about the outdated_versions tests?

Is it OK that all of them go in separate methods and there is some code duplication?
Splitting outdated_versions tests into several methods is handy when one of the tests fails, so we can explicitly know in which conditions it happens. If we put all of the tests logic into one method we won't be able to understand where exactly it fails that easily.

So, the question is do we need a separate test_outdated_versions.rb file with outdated_versions tests or something (maybe OutdatedVersionsTests < Homebrew::TestCase inside test_formula.rb)?

[xu-cheng]

what do you think about the outdated_versions tests?

👍

Is it OK that all of them go in separate methods and there is some code duplication?

Yes. I think it's a good idea to have multiple test methods to pin point the possible problem.

So, the question is do we need a separate test_outdated_versions.rb file with outdated_versions tests or something (maybe OutdatedVersionsTests < Homebrew::TestCase inside test_formula.rb)?

I think both are fine. But personally I prefer to putting then in test_formula.rb, so it's easy to locate the test in the future. If you choose OutdatedVersionsTests < Homebrew::TestCase, then you can avoid duplication using setup and teardown methods.

Have you run test case before simplifying the logic. Ideally, we should make the tests pass before and after you applying the logic change.

[vladshablinsky]

Have you run test case before simplifying the logic. Ideally, we should make the tests pass before and after you applying the logic change.

Yes, It worked.

Decided to move tests to OutdatedVersionsTests < Homebrew::TestCase.

[UniqMartin]

It looks like this pattern is repeated in every single test method (sometimes multiple times) which tells me that all of this should be consolidated in the helper function. And the returned Tab object isn't actually needed. How about renaming tab_for_prefix to setup_tab_with_tap and using it as follows?

setup_tab_with_tap(same_prefix, "homebrew/core")

This could be the implementation (untested):

def setup_tab_with_tap(prefix, tap_string = nil)
  prefix.mkpath
  tab = Tab.empty
  tab.tabfile = prefix/"INSTALL_RECEIPT.json"
  tab.source["tap"] = tap_string if tap_string
  tab.write
  tab # Not needed and not currently used anywhere (if I didn't overlook anything).
end

This method would also make rewrite_tab_with_tap obsolete (i.e. setup_tab_with_tap should create the tab file and the prefix if it doesn't already exist yet and overwrite it otherwise).

[UniqMartin]

👍 on the simplification in this PR, but I have added a suggestion to further reduce unnecessary code duplication in the test methods.

[vladshablinsky]

👍 I didn't remove tab at the end of the setup_tab_with_tap method, did I need to? Now it's not used, that is true, but I'm not sure if I need to remove it.

[UniqMartin]

The return value assigned to the local variable is never used, thus the assignment can be dropped.

[UniqMartin]

Thanks for making the change! To me the code now looks much cleaner.

I didn't remove tab at the end of the setup_tab_with_tap method, did I need to? Now it's not used, that is true, but I'm not sure if I need to remove it.

You don't have to. It's completely up to you and that's one additional line that doesn't really bother me (and maybe will even become useful later on).

[xu-cheng]

Thanks everyone.

[MikeMcQuaid]

Could this be tweaked in a future PR so you return false for each of those cases rather than chaining with &&? It makes it much easier to follow.

[vladshablinsky]

👍 #432

[MikeMcQuaid]

One comment that would be cool to address in a future PR. For refactoring like this it's sometimes nice to open the PR with just the commit with the added tests so we can see that all those tests pass on both the old and new versions.