dev-cmd/tests: skip tests that require core if it's not tapped #17212
Conversation
There were a few tests which require core to be tapped and fail if it isn't. This is annoying if someone is trying to contribute to the project and they're using the JSON API instead of having the core repo tapped locally. I'm just skipping these because it's the simplest thing to do. The tests that failed are mostly rubocop tests so it's fine if they only run on CI.
There was a problem hiding this comment.
Thanks @apainintheneck! Let's merge this as-is to fix things for contributors and, if you have bandwidth, a follow-up for the comments would be great.
| @@ -13,7 +13,7 @@ | |||
| .and be_a_success | |||
| end | |||
|
|
|||
| it "prints the prefix for a Formula", :integration_test do | |||
| it "prints the prefix for a Formula", :integration_test, :needs_homebrew_core do | |||
There was a problem hiding this comment.
This should already work without homebrew/core? The command is definitely working without 🤔
There was a problem hiding this comment.
It was the only other test that failed for me locally after untapping homebrew/core. I believe it couldn't load the formula. Feel free to update it if you can get it working.
$ brew untap homebrew/core
Untapping homebrew/core...
Untapped 3 commands and 7053 formulae (7,402 files, 807.5MB).
$ brew tests --only=cmd/--prefix
Randomized with seed 37956
1 process for 1 spec, ~ 1 spec per process
...F.
Failures:
1) Homebrew::Cmd::Prefix prints the prefix for a Formula
Failure/Error:
expect { brew_sh "--prefix", "wget" }
.to output("#{ENV.fetch("HOMEBREW_PREFIX")}/opt/wget\n").to_stdout
.and not_to_output.to_stderr
.and be_a_success
expected block to output "/usr/local/opt/wget\n" to stdout, but output nothing
...and:
expected block to not output to stderr, but output "Error: No available formula with the name \"wget\".\n"
...and:
expected #<Proc:0x000000011089c900 /usr/local/Homebrew/Library/Homebrew/test/cmd/--prefix_spec.rb:17> to be a success
Diff for (output "/usr/local/opt/wget\n" to stdout):
@@ -1,2 +1 @@
-/usr/local/opt/wget
# ./test/cmd/--prefix_spec.rb:17:in `block (2 levels) in <top (required)>'
# ./test/support/helper/spec/shared_context/integration_test.rb:49:in `block (2 levels) in <top (required)>'
Took 11 seconds
Tests FailedThere was a problem hiding this comment.
This should be resolvable by adding a test_formula or similar which is written to the relevant location on disk or, as below, writing a mock API JSON to disk.
There was a problem hiding this comment.
Would that work for an integration test? I assumed that integration tests used the repo in it's normal state since they shell out to the brew executable so any test formula would need to be added to one of the non-test repos which seems less than ideal.
There was a problem hiding this comment.
Would that work for an integration test?
Yes, other integration tests use non-core formulae.
Also, in this case: I'm pretty sure writing to ${HOMEBREW_CACHE}/api/formula_names.txt is sufficient as formula_path.sh is the main logic here.
| @@ -2,7 +2,7 @@ | |||
|
|
|||
| RSpec.shared_examples "formulae exist" do |array| | |||
| array.each do |f| | |||
| it "#{f} formula exists" do | |||
| it "#{f} formula exists", :needs_homebrew_core do | |||
| core_tap = Pathname("#{HOMEBREW_LIBRARY_PATH}/../Taps/homebrew/homebrew-core") | |||
There was a problem hiding this comment.
Feels like this could look in the JSON instead for this key.
There was a problem hiding this comment.
Technically yes but I don't know a simple way to set that up in the test environment.
There was a problem hiding this comment.
Write a mock API JSON to the cache.
There was a problem hiding this comment.
I'm pretty sure that it would require writing a JSON file signed with a private key to the cache along with mocking the public key so that we can check that the JSON file is correct when loading it for the first time. The same thing would need to happen with the formula tap migrations file as well or that would need to be mocked. We'd want to decide before that where we should put the public and private keys in the repo and if we want to sign things on the fly or have fixtures for the signed JSON files. It's not as simple as just writing one file to the cache.
There was a problem hiding this comment.
That's only necessary if you want to avoid any Ruby mocking. You should be able to mock/populate the cache of higher-level methods instead e.g. Homebrew::API::Formula.all_formulae, tap_migrations.
CC @Bo98 for thoughts here. This has come up a few times and I don't think "it's not simple to write tests for the default API path so we shouldn't do it" is a good place for us to be in as a project.
There was a problem hiding this comment.
That's only necessary if you want to avoid any Ruby mocking. You should be able to mock/populate the cache of higher-level methods instead e.g.
Homebrew::API::Formula.all_formulae,tap_migrations.
Yeah, mocking those method calls is simple enough to do. We already do that in a few places in tests. We'd just have create a larger API JSON test fixture to read from.
I think I got confused by the Write a mock API JSON to the cache. comment which implied to me that we wanted to actually write a file to the cache directory which would require more of the stuff I mentioned before.
Either way, discussion about how to best add tests for the API path is better done here: #16895
I'm thinking that some of these problems could be solved by test helpers and convention.
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?There were a few tests which require core to be tapped and fail if it isn't. This is annoying if someone is trying to contribute to the project and they're using the JSON API instead of having the core repo tapped locally.
I'm just skipping these because it's the simplest thing to do. The tests that failed are mostly rubocop tests anyway so it's fine if they only run on CI.
Fixes #17147