Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

dev-cmd/tests: skip tests that require core if it's not tapped #17212

Merged
merged 1 commit into from
May 3, 2024
Merged

dev-cmd/tests: skip tests that require core if it's not tapped #17212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Library/Homebrew/test/cmd/--prefix_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should already work without homebrew/core? The command is definitely working without 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Failed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

expect { brew_sh "--prefix", "wget" }
.to output("#{ENV.fetch("HOMEBREW_PREFIX")}/opt/wget\n").to_stdout
.and not_to_output.to_stderr
Expand Down
5 changes: 5 additions & 0 deletions Library/Homebrew/test/spec_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,11 @@
skip "Requires network connection." unless ENV["HOMEBREW_TEST_ONLINE"]
end

config.before(:each, :needs_homebrew_core) do
core_tap_path = "#{ENV.fetch("HOMEBREW_LIBRARY")}/Taps/homebrew/homebrew-core"
skip "Requires homebrew/core to be tapped." unless Dir.exist?(core_tap_path)
end

config.before do |example|
next if example.metadata.key?(:needs_network)
next if example.metadata.key?(:needs_utils_curl)
Expand Down
2 changes: 1 addition & 1 deletion Library/Homebrew/test/support/helper/spec/shared_examples/formulae_exist.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like this could look in the JSON instead for this key.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically yes but I don't know a simple way to set that up in the test environment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Write a mock API JSON to the cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

formula_paths = core_tap.glob("Formula/**/#{f}.rb")
alias_path = core_tap/"Aliases/#{f}"
Expand Down