Test officially supported cmd taps. #390

MikeMcQuaid · 2016-06-22T10:28:14Z

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?

All of these taps use Homebrew internal APIs (or will shortly) and we autoinstall them all from brew $CMD. We should adjust our CI to ensure that we never accidentally break these taps when making changes to core code so that these taps can rely more on this core code rather than having to e.g. vendor equivalent code that never changes on our end.

CI may well by wrong here as I've not tested this on the actual bot and it won't run brew tests --official-cmd-taps (although I should fix that so we can test new brew test-bot changes here).

The approach may well be terrible; we could perhaps get away with a simpler integration tests or a subset of their tests. Alternatively, we may also want to consider running these tests in parallel with something like https://github.com/grosser/parallel_tests.

This (in some form) will be a requirement for @AnastasiaSulyagina's GSoC project to be able to remove duplicated/vendored core code from Caskroom/cask and eventually move it into Homebrew itself and it's something we should have done for Homebrew/bundle a while ago. This also perhaps overlaps with @eirinikos's integration testing work.

CC @Homebrew/maintainers for thoughts as this is a fairly big change in how we do CI for this repository but it's something we'll have to do in some form.

xu-cheng · 2016-06-22T10:37:13Z

Library/Homebrew/cmd/tests.rb

+
+    if ARGV.include? "--official-cmd-taps"
+      OFFICIAL_CMD_TAPS.each do |org, repo|
+        tap = "#{org}/#{repo}"


tap = Tap.fetch(user, repo)

MikeMcQuaid · 2016-06-22T17:09:16Z

@UniqMartin @xu-cheng Neither of you are opposed to the overall concept of this PR?

UniqMartin · 2016-06-22T17:11:12Z

Library/Homebrew/cmd/tests.rb

+            system "bundle", "install", "--path", "vendor/bundle"
+          end
+          system "bundle", "exec", "rake"
+          Homebrew.failed = !$?.success?


Homebrew.failed = true unless $?.success?

Otherwise a later success may mask a previous failure.

UniqMartin · 2016-06-22T17:23:07Z

@UniqMartin @xu-cheng Neither of you are opposed to the overall concept of this PR?

I'm not opposed. Makes a lot of sense to me, though I personally don't care that much about these external commands because I don't use them. How long does this additional step approximately take to complete compared to a regular brew tests run? Does running this on our Mavericks test bot (with Ruby 1.8.7) make sense? Do those external commands even support Ruby 1.8.7?

xu-cheng · 2016-06-23T05:01:56Z

I'm not opposed to this PR's concept as well.

MikeMcQuaid · 2016-06-30T21:47:09Z

Have addressed feedback.

How long does this additional step approximately take to complete compared to a regular brew tests run? Does running this on our Mavericks test bot (with Ruby 1.8.7) make sense? Do those external commands even support Ruby 1.8.7?

Will get you the numbers for this after this PR runs the CI job.

UniqMartin · 2016-07-01T17:33:49Z

Library/brew.rb

-    when "services"
-      Tap.fetch("Homebrew", "services")
-    end
+    possible_tap = OFFICIAL_CMD_TAPS.find { |_, repo| repo == cmd }


Are you intentionally dropping support for auto-tapping when brew bundle is invoked with the alternative commands brewdle, brewdler, and bundler? (I'm not opposing this change, just making sure.)

Why don't we use the Hash approach in the above? i.e.

OFFICIAL_CMD_TAPS = { "homebrew/bundle" => %w[bundle ...] ... }.freeze

I am however opposite to only check cmd with tap name, which it's a bad assumption for future commands or taps.

Are you intentionally dropping support for auto-tapping when brew bundle is invoked with the alternative commands brewdle, brewdler, and bundler? (I'm not opposing this change, just making sure.)

Yes. PR incoming for homebrew-bundle to do that.

I am however opposite to only check cmd with tap name, which it's a bad assumption for future commands or taps.

I don't think it's a bad assumption but we can correct it when it's no longer correct as it's an implementation detail.

If you take homebrew/alias into account, the assumption is already broken. 😉

This now either needs a require "official_taps" added before this line or directly in tap.rb (otherwise the constant OFFICIAL_CMD_TAPS is undefined at this point).

I also agree with @xu-cheng that it would be better to use his suggested hash approach from the start. Unlike the currently proposed solution it doesn't make any assumptions about the tap name vs. command name relationship and allows for multiple commands in a single tap, thus feels very future-proof to me. It doesn't feel like “doing it right” from the start creates a significant burden.

The redundancy and speculation annoys me a bit here but will defer to you to get this merged.

UniqMartin · 2016-07-02T00:32:56Z

Have you been able to run brew official-cmd-tap-tests successfully? If I try that locally on my machine (OS X 10.11) in a Homebrew installation without any pre-installed taps except homebew/core, the Homebrew Cask tests actually fail with:

$ brew official-cmd-tap-tests
==> Running caskroom/cask tests:
==> Tapping caskroom/cask
[…snip…]
8307 tests, 10105 assertions, 1 failures, 4 errors, 2395 skips
[…snip…]

The failure and those errors of course result in a nonzero exit code. On the other hand, the tests run for homebrew/bundle finish just fine.

Ignoring the failures for a moment, here are some numbers when running this locally:

$ /usr/bin/time brew tests --no-compat
[…snip…]
       74.16 real        57.36 user        16.19 sys
$ /usr/bin/time brew tests --no-compat
[…snip…]
       67.33 real        54.76 user        13.66 sys
$ /usr/bin/time brew official-cmd-tap-tests
[…snip…]
      353.80 real       142.69 user        39.52 sys
$ /usr/bin/time brew official-cmd-tap-tests
[…snip…]
      193.10 real        76.73 user        30.89 sys

Those extra tests are not exactly cheap, particularly on the first run when the taps need to be installed and the Gems required for testing need to be fetched and installed via bundle install. On our test bot we'll obviously always have the slower times because all the taps and Gems will need to be installed anew on every test bot run.

MikeMcQuaid · 2016-07-03T21:10:15Z

Cask's tests seem very oriented towards their Travis setup so I decided it was a bad idea to run the entire test suite (and probably overkill) so I decided to just write integration tests to smoke tests these three commands; in my experience if we e.g. break an API they are relying on we'll realise pretty quickly. I think this gives us the best of both worlds; fast integration tests (less than a second added in total for all three assuming they are already tapped) whilst making sure we don't obliterate the internal methods they rely on.

UniqMartin · 2016-07-04T06:50:39Z

Library/Homebrew/test/test_integration_cmds.rb

+
+  def test_services
+    skip_unless_test_cmd_taps
+    assert_equal "Warning: No services available to control with `brew services`", cmd("services", "list")


Can you wrap this overly long line, just like you did in the bundle check case?

UniqMartin · 2016-07-04T07:04:14Z

Commented on some code, that is probably causing the test bot run to fail, and on a few details.

Cask's tests seem very oriented towards their Travis setup so I decided it was a bad idea to run the entire test suite (and probably overkill)

I agree.

so I decided to just write integration tests to smoke tests these three commands; in my experience if we e.g. break an API they are relying on we'll realise pretty quickly.

Makes sense and works for me.

UniqMartin · 2016-07-04T13:09:42Z

Library/Homebrew/cmd/tests.rb

+    if ARGV.include? "--official-cmd-taps"
+      ENV["HOMEBREW_TEST_OFFICIAL_CMD_TAPS"] = "1"
+      OFFICIAL_CMD_TAPS.each do |tap, _|
+        tap = Tap.fetch tap


This probably needs a require "tap" at the top of the file. Here's what I currently get:

$ brew tests --no-compat --official-cmd-taps Error: uninitialized constant Homebrew::Tap Please report this bug: https://git.io/brew-troubleshooting /opt/brewery/dummy/Library/Homebrew/cmd/tests.rb:14:in `block in tests' /opt/brewery/dummy/Library/Homebrew/cmd/tests.rb:13:in `each' /opt/brewery/dummy/Library/Homebrew/cmd/tests.rb:13:in `tests' /opt/brewery/dummy/Library/brew.rb:87:in `<main>'

All of these taps use Homebrew internal APIs (or will shortly) and we autoinstall them all from `brew $CMD`. We should adjust our CI to ensure that we never accidentally break these taps when making changes to core code so that these taps can rely more on this core code rather than having to e.g. vendor equivalent code that never changes on our end.

This reverts commit 252c701. Taps installed prior to running the test suite are not visible to the test suite as most Homebrew paths are redefined as to not mess up the local installation.

UniqMartin · 2016-07-04T15:46:01Z

I'm sorry, but I had to revert this in dba1958. This needs more work and local testing via, e.g.,

brew tests --official-cmd-taps

before it can be merged (as the test bot won't run this code until it is merged, at which point it's too late). To quote from the revert commit:

Taps installed prior to running the test suite are not visible to the test suite as most Homebrew paths are redefined as to not mess up the local installation.

UniqMartin · 2016-07-04T15:59:38Z

I just realized that there is/was another issue with the current implementation. The entire test suite now runs thrice: once with coverage in no-compat mode, once regularly, and now once more with the three integration tests from this PR enabled. The latter two runs are basically identical except for those newly added integration tests, resulting in some unnecessary and wasteful redundancy.

This reverts commit dba1958.

MikeMcQuaid · 2016-07-04T20:36:57Z

Addressed this in a series of commits ending in a1b0ef1 as currently the bot isn't doing CI testing with the correct version so it was easier to just ensure it was working properly on all platforms by pushing to master. The green builds for Travis and Jenkins are: https://travis-ci.org/Homebrew/brew/builds/142266951 http://bot.brew.sh/job/Homebrew%20Testing/1029/

UniqMartin · 2016-07-05T14:19:10Z

👍

All of these taps use Homebrew internal APIs (or will shortly) and we autoinstall them all from `brew $CMD`. We should adjust our CI to ensure that we never accidentally break these taps when making changes to core code so that these taps can rely more on this core code rather than having to e.g. vendor equivalent code that never changes on our end.

This reverts commit 252c701. Taps installed prior to running the test suite are not visible to the test suite as most Homebrew paths are redefined as to not mess up the local installation.

This reverts commit dba1958.

BrewTestBot added the in progress Maintainers are working on this label Jun 22, 2016

xu-cheng reviewed Jun 22, 2016
View reviewed changes

UniqMartin reviewed Jun 22, 2016
View reviewed changes

UniqMartin reviewed Jul 1, 2016
View reviewed changes

UniqMartin reviewed Jul 4, 2016
View reviewed changes

MikeMcQuaid mentioned this pull request Jul 4, 2016

Delete exceptions from homebrew-fork Homebrew/homebrew-cask#22574

Merged

1 task

UniqMartin reviewed Jul 4, 2016
View reviewed changes

MikeMcQuaid merged commit 252c701 into Homebrew:master Jul 4, 2016

MikeMcQuaid deleted the test-cmd-taps branch July 4, 2016 15:08

BrewTestBot removed the in progress Maintainers are working on this label Jul 4, 2016

MikeMcQuaid added a commit that referenced this pull request Jul 4, 2016

Revert "Revert "Test officially supported cmd taps. (#390)""

bf42959

This reverts commit dba1958.

souvik1997 pushed a commit to souvik1997/brew that referenced this pull request Jul 25, 2016

Revert "Revert "Test officially supported cmd taps. (Homebrew#390)""

852f6db

This reverts commit dba1958.

Homebrew locked and limited conversation to collaborators May 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test officially supported cmd taps. #390

Test officially supported cmd taps. #390

MikeMcQuaid commented Jun 22, 2016

xu-cheng Jun 22, 2016

MikeMcQuaid commented Jun 22, 2016

UniqMartin Jun 22, 2016

UniqMartin commented Jun 22, 2016

xu-cheng commented Jun 23, 2016 •

edited

Loading

MikeMcQuaid commented Jun 30, 2016

UniqMartin Jul 1, 2016

xu-cheng Jul 1, 2016 •

edited

Loading

MikeMcQuaid Jul 1, 2016

xu-cheng Jul 2, 2016

UniqMartin Jul 4, 2016

UniqMartin Jul 4, 2016

MikeMcQuaid Jul 4, 2016

UniqMartin commented Jul 2, 2016

MikeMcQuaid commented Jul 3, 2016

UniqMartin Jul 4, 2016

UniqMartin commented Jul 4, 2016 •

edited

Loading

UniqMartin Jul 4, 2016

UniqMartin commented Jul 4, 2016

UniqMartin commented Jul 4, 2016

MikeMcQuaid commented Jul 4, 2016

UniqMartin commented Jul 5, 2016

Test officially supported cmd taps. #390

Test officially supported cmd taps. #390

Conversation

MikeMcQuaid commented Jun 22, 2016

Choose a reason for hiding this comment

MikeMcQuaid commented Jun 22, 2016

Choose a reason for hiding this comment

UniqMartin commented Jun 22, 2016

xu-cheng commented Jun 23, 2016 • edited Loading

MikeMcQuaid commented Jun 30, 2016

Choose a reason for hiding this comment

xu-cheng Jul 1, 2016 • 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

UniqMartin commented Jul 2, 2016

MikeMcQuaid commented Jul 3, 2016

Choose a reason for hiding this comment

UniqMartin commented Jul 4, 2016 • edited Loading

Choose a reason for hiding this comment

UniqMartin commented Jul 4, 2016

UniqMartin commented Jul 4, 2016

MikeMcQuaid commented Jul 4, 2016

UniqMartin commented Jul 5, 2016

xu-cheng commented Jun 23, 2016 •

edited

Loading

xu-cheng Jul 1, 2016 •

edited

Loading

UniqMartin commented Jul 4, 2016 •

edited

Loading