Skip to content

test: replace some install_test_formula with setup_test_formula #21174

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
Merged
cho-m merged 1 commit into from
Dec 4, 2025
Merged

test: replace some install_test_formula with setup_test_formula #21174

cho-m merged 1 commit into from
Dec 4, 2025

Conversation

@cho-m
Copy link
Member

@cho-m cho-m commented Dec 4, 2025 •
edited
Loading

  • 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 lgtm (style, typechecking and tests) with your changes locally?

We can fake an install with just a tab file to avoid the overhead needed to run all of install_test_formula.

Copilot AI review requested due to automatic review settings December 4, 2025 02:08
Copilot AI reviewed Dec 4, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR optimizes integration test performance by replacing install_test_formula with setup_test_formula in select test files. The setup_test_formula helper creates the necessary formula and tab files without executing the full installation process (prelude, fetch, install, finish), reducing test execution time by 50% or more for tests that don't require actual formula installation.

Key changes:

  • Replaced install_test_formula with setup_test_formula in tests that only need formula metadata or tab file attributes
  • Streamlined tab attribute setup by using setup_test_formula's built-in tab_attributes parameter instead of manual tab manipulation

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
Library/Homebrew/test/formula_info_spec.rb Converted to use setup_test_formula in a let block since FormulaInfo.lookup only needs the formula path, not a full installation
Library/Homebrew/test/cmd/autoremove_spec.rb Replaced verbose install_test_formula + manual tab manipulation with concise setup_test_formula using tab_attributes parameter to create test fixtures

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cho-m cho-m force-pushed the test-setup_test_formula branch 4 times, most recently from d359c56 to 3ff5b9e Compare December 4, 2025 03:53
@cho-m cho-m marked this pull request as draft December 4, 2025 04:00
@cho-m
test: replace some install_test_formula with setup_test_formula
dfdef32 
We can fake an install with just a tab file to avoid the overhead needed
to run all of `install_test_formula`. This reduces time for modified
test cases by 50% or more.
@cho-m cho-m force-pushed the test-setup_test_formula branch from 3ff5b9e to dfdef32 Compare December 4, 2025 04:04
MikeMcQuaid
MikeMcQuaid approved these changes Dec 4, 2025
View reviewed changes
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Seems like a great idea. My guess is this is much faster? Any benchmarks yet? brew tests --profile, e.g. brew tests --ruby-prof or hyperfine may help here.

@cho-m
Copy link
Member Author

cho-m commented Dec 4, 2025 •
edited
Loading

My guess is this is much faster? Any benchmarks yet? brew tests --profile

For the specific tests, it is faster, e.g.

  • before 
    ❯ brew tests --profile 3 --seed 0 --only cmd/uninstall
Randomized with seed 0
1 process for 1 spec, ~ 1 spec per process
..

Top 2 slowest examples (4.87 seconds, 97.1% of total time):
  Homebrew::Cmd::UninstallCmd uninstalls a given Formula
    4.86 seconds ./test/cmd/uninstall_spec.rb:9
  Homebrew::Cmd::UninstallCmd behaves like parseable arguments can parse arguments
    0.01288 seconds ./test/cmd/shared_examples/args_parse.rb:8
  • after 
    ❯ brew tests --profile 3 --seed 0 --only cmd/uninstall
Randomized with seed 0
1 process for 1 spec, ~ 1 spec per process
..

Top 2 slowest examples (1.5 seconds, 88.8% of total time):
  Homebrew::Cmd::UninstallCmd uninstalls a given Formula
    1.5 seconds ./test/cmd/uninstall_spec.rb:9
  Homebrew::Cmd::UninstallCmd behaves like parseable arguments can parse arguments
    0.00732 seconds ./test/cmd/shared_examples/args_parse.rb:8

The total time is going to depend on interleaving of tests as some unmodified tests like cmd/install_spec.rb are still the slowest ones. In worst case, the slowest tests can run on the same thread. But the average time should be better.

I haven't found a good way to reduce some overhead on those. Was profiling them but not able to analyze the 2nd brew instance that gets run via Open3.capture3

@MikeMcQuaid
Copy link
Member

MikeMcQuaid commented Dec 4, 2025

@cho-m huge improvement, nice work!

@cho-m cho-m marked this pull request as ready for review December 4, 2025 18:38
@cho-m
Copy link
Member Author

cho-m commented Dec 4, 2025 •
edited
Loading

Going to merge this and continue looking into other slow parts of test in follow ups.

Hoping to remove unnecessary steps on test side to help identify places where brew may need further profiling and performance improvements.

EDIT: As note, I've verified all modified tests perform faster than original.

@cho-m cho-m added this pull request to the merge queue Dec 4, 2025
@cho-m cho-m removed this pull request from the merge queue due to a manual request Dec 4, 2025
@cho-m
Copy link
Member Author

cho-m commented Dec 4, 2025 •
edited
Loading

Actually removed from queue to trigger Copilot review as it may not have run during time I converted to draft.

EDIT: Review is complete with no new comments, so starting merge again.

@cho-m cho-m requested a review from Copilot December 4, 2025 19:20
Copilot AI reviewed Dec 4, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cho-m cho-m added this pull request to the merge queue Dec 4, 2025
Merged via the queue into main with commit 2d71360 Dec 4, 2025
43 checks passed
@cho-m cho-m deleted the test-setup_test_formula branch December 4, 2025 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@MikeMcQuaid MikeMcQuaid MikeMcQuaid approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cho-m @MikeMcQuaid