tests: refactor formula file creation

[eirinikos]

• 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?

---

This change consolidates almost all of the instances of formula_file = CoreTap.new.formula_dir/"testball.rb" into the method include_formula_file, which makes use of @formula_file.

• For tests that involve at least two formula files, the method include_formula_files creates @foo_file and @bar_file.

  • Tests that involve three formula files create a @baz_file.

• The url for @formula_file is now solely "file://#{File.expand_path("..", __FILE__)}/tarballs/testball-0.1.tbz".

  • Before, it was either "file://#{File.expand_path..." or "https://example.com/testball-0.1.tar.gz".

• Some tests ensure only @formula_file.unlink, whereas others ensure @formula_file.unlink unless @formula_file.nil?

  • These were left as-is, but should all tests have the same ensure block?

• test_create was left as-is -- I wasn't sure how to improve upon it, given that url is not an accessible attribute on formula instances.

• test_options has also been (mostly) left as-is for now.

  • It seems like depends_on "bar" => :recommended should not be included in the default @formula_file, though maybe I could append this to the formula from within test_options?
  • Edit: hm, appending doesn't seem to be a solution (at least, not the way I've used it).
    • I've run test_options as shown below, and this error is raised within cmd_output :
      Error: undefined method 'depends_on' for Formulary::FormulaNamespace2d3ec45e3a12a54ca19cb33c3b0cddbf:Module

  def test_options
    include_formula_file
    @formula_file.append_lines("depends_on \"bar\" => :recommended")
  
    assert_equal "--with-foo\n\tBuild with foo\n--without-bar\n\tBuild without bar support",
      cmd_output("options", "testball")
  ensure
    @formula_file.unlink
  end
  

🎈

[MikeMcQuaid]

Where is balltest coming from? A typo?

[UniqMartin]

balltest is an alias for the testball formula that is being set up here and in the following two lines, if I'm not misreading the code.

[MikeMcQuaid]

A few comments but generally looking great 😍

[UniqMartin]

The de-duplication of this stuff is good, but may I suggest a somewhat different approach? Here is what I'd hope this refactoring would yield:

• A method named setup_test_formula(name) that:
  • Creates either of three possible formulae depending on the given name: testball, foo, or bar.
  • Adds the path (a Pathname object) of the just created formula to an array @formula_files.
  • Returns that same path for use within the calling test_* method.
• A modified teardown method that:
  • Iterates over the entries of @formula_files and unlinks all those files. No need to do this in every single test in the ensure block.
• A lot of modified test_* methods that:
  • Only use the path returned by the setup_test_formula method and refrain from touching the @formula_files instance variable.
  • Quite a few dropped or shortened ensure blocks.

I hope I was somewhat convincing. 😸 I strongly believe this will combine the benefits of less boilerplate (already achieved in the current code) with greater clarity (explicitly requesting certain formulae by name and getting their path in return while avoiding implicitly defined instance variables).

---

To accommodate the currently unhandled cases where a more customized formula is needed, the method could be extended with a second argument to accept something like:

baz_path = setup_test_formula "baz", <<-EOS.undent
  desc "The baz formula"
  url "http://example.com/baz-0.1.tar.gz"
  depends_on "foo"
  depends_on "bar" => :optional
EOS

This would allow to still benefit from the auto-cleanup logic suggested above and the method could further reduce the necessary boilerplate code by providing the surrounding class Baz (generated with Formulary.class_s from name) and end lines.

[eirinikos]

I added this line solely for test_options, but only now realized that it seems wrong to nest #{content} inside a declaration for content. Still, the test seems to pass without error...

[UniqMartin]

That's perfectly fine. You're not doing anything different from modifying an existing variable, e.g. content = "a prefix: #{content} and some extra test" is basically the same thing, its just not using a “here document”.

[eirinikos]

Thanks for the suggestions, @UniqMartin! This was a fun challenge.

I have:

• defined setup_test_formula.
• modified setup, teardown, and various test_* methods to account for the @formula_files array.

I have also:

• fixed most of the "offenses" detected by brew style test_integration_cmds.rb
• refactored test_desc and test_home according to prior discussion re: test_search

I hope it's OK that I made those extra changes even if they weren't strictly related to the task at hand; I figured I might as well.

[eirinikos]

Would it be preferable to use setup_test_formula "foo" && setup_test_formula "bar" instead of

setup_test_formula "foo"
setup_test_formula "bar"

?

[UniqMartin]

I'm definitely in favor of the latter. Intuitively, using && here has the same effect but doesn't make much sense, but maybe I'm just missing the point.

[UniqMartin]

Unsurprisingly, I really like the approach. 😂 But in all seriousness, I think this is a very nice improvement to the testing code and a nice consolidation of the test formula setup logic. 👍 The diff statistics are also quite encouraging (we got rid of 166 basically redundant lines):

$ git merge pr/370
Auto-merging Library/Homebrew/test/test_integration_cmds.rb
Merge made by the 'recursive' strategy.
 Library/Homebrew/test/test_integration_cmds.rb | 388 +++++++++++++++++++----------------------------------------------
 1 file changed, 111 insertions(+), 277 deletions(-)

I hope it's OK that I made those extra changes even if they weren't strictly related to the task at hand; I figured I might as well.

Ideally, these would have been separate commits, but I don't mind accepting this as-is (modulo the minor indentation fixes pointed out in the code comments).

[eirinikos]

I hope it's OK that I made those extra changes even if they weren't strictly related to the task at hand; I figured I might as well.

Ideally, these would have been separate commits, but I don't mind accepting this as-is (modulo the minor indentation fixes pointed out in the code comments).

👍 Thank you! I'll keep this in mind for future reference.

I've made the indentation fixes, but I'm having trouble with git rebase (again 😣 )... attempts to remove the previous commit leads me to a modified file where the work of my previous commit has been wiped out, and there are merge conflicts for test_deps and test_uses. Normally, just removing the previous commit during rebase seems to bring the desired effect...

This may be because I blindly ran git rebase -s recursive -X ours master at some point during this process. Sorry, any idea of how to resolve this problem?

[UniqMartin]

I've made the indentation fixes, but I'm having trouble with git rebase (again 😣 )... attempts to remove the previous commit leads me to a modified file where the work of my previous commit has been wiped out, and there are merge conflicts for test_deps and test_uses. Normally, just removing the previous commit during rebase seems to bring the desired effect...

I'm assuming you're talking about git rebase -i? You almost never want to remove individual commits in the sheet this brings up, as that indeed removes the changes made in the respective commits. This would also explain the merge conflicts as the follow-up adjustments depend on those changes. What you instead want to do in almost all cases is combine the changes from multiple commits, e.g. you leave the first commit at pick and change the next commit in the list from pick to squash. This will combine these two commits and ask you for the commit message of this combined commit in the process.

Or use git commit --amend, if all you want to do is make adjustments to the most recent commit.

This may be because I blindly ran git rebase -s recursive -X ours master at some point during this process. Sorry, any idea of how to resolve this problem?

Don't do that, unless you know exactly what it is doing or someone else (me) has advised to use it in very specific circumstances after having tested it does the right thing. 😛

You almost always want to use a regular git rebase or git rebase -i depending on whether you want to rebase your changes on top of an updated upstream branch or whether you want to squash/rearrange/… commits in your current branch (but usually without actually rebasing your branch, i.e. the base commit in the upstream branch from where your branch forks off remains the same).

[UniqMartin]

And if you're stuck with a borked local branch: The easiest fix in this case would be to throw away the local branch with git branch -D refactor-formula-file-creation and then recreate it from the remote branch in your forked repository via git checkout eirinikos/refactor-formula-file-creation (it is still in good condition). The indentation fixes are small enough to be manually reapplied on top of that.

[eirinikos]

This may be because I blindly ran git rebase -s recursive -X ours master at some point during this process. Sorry, any idea of how to resolve this problem?

Don't do that, unless you know exactly what it is doing or someone else (me) has advised to use it in very specific circumstances after having tested it does the right thing. 😛

Thanks @UniqMartin! Yup! I ought not to have done that; I guess I often throw caution to the wind when things seem desperate. 😬

And if you're stuck with a borked local branch: The easiest fix in this case would be to throw away the local branch with git branch -D refactor-formula-file-creation and then recreate it from the remote branch in your forked repository via git checkout eirinikos/refactor-formula-file-creation (it is still in good condition).

I went ahead with this route, did git add and git commit, checked out and switched to a new branch, and successfully squashed the commits with git rebase -i. Thanks for the explanations; I've had a lot of trouble with rebasing and I'm still getting familiar with its usage.

[UniqMartin]

I'm happy with this refactoring and think it's ready to be merged, but will keep it open a bit longer (maybe a day or two) so that @MikeMcQuaid and other maintainers have a chance to voice their opinions.

[MikeMcQuaid]

😍 @eirinikos!