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.

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

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

Merged
merged 1 commit into from
May 3, 2024

Conversation

apainintheneck
Copy link
Contributor

  • 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 style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with 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

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.
@apainintheneck apainintheneck added the bug Reproducible Homebrew/brew bug label May 3, 2024
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

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
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.

@@ -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.

@MikeMcQuaid MikeMcQuaid merged commit e944830 into master May 3, 2024
25 checks passed
@MikeMcQuaid MikeMcQuaid deleted the skip-tests-that-require-core-if-not-tapped branch May 3, 2024 08:20
@github-actions github-actions bot added the outdated PR was locked due to age label Jun 8, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Reproducible Homebrew/brew bug outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

brew tests should pass with homebrew/core untapped
2 participants