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

Add HOMEBREW_TEST_TIMEOUT_SECS env var #18629

Merged
merged 1 commit into from
Oct 26, 2024
Merged

Conversation

ZhongRuoyu
Copy link
Member

@ZhongRuoyu ZhongRuoyu commented Oct 24, 2024

  • 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?

Intended mainly for third-party formulae, this allows formula tests to be run with a customised timeout.

This is a private, undocumented API and subject to change.

@p-linnane
Copy link
Member

Awesome, thanks! Can we gate this to non-official taps?

@ZhongRuoyu
Copy link
Member Author

ZhongRuoyu commented Oct 24, 2024

Awesome, thanks! Can we gate this to non-official taps?

Implementation-wise, yes, we can probably restrict this to non-official taps only. However, I think there isn't much harm to apply it unconditionally since 5 minutes is still the default. Changing the timeout for official taps would still require changes to at least one of brew, brew test-bot, or the GitHub Actions workflow (to be more specific, I don't think you can do it in the formula).

By keeping this, we could perhaps benefit from it even when testing official formulae locally, in some cases. We occasionally see tests that actually take more than 5 minutes, so we could perhaps use this to bump the timeout when debugging locally.

That's only my 2 cents though. Happy to change my mind if you'd still like this to be done that way!

@Bo98
Copy link
Member

Bo98 commented Oct 24, 2024

Changing the timeout for official taps would still require changes to at least one of brew, brew test-bot, or the GitHub Actions workflow (to be more specific, I don't think you can do it in the formula).

Yeah I agree. Ultimately, the restriction only matters in our CI and we'll never change this in our CI so I think it's ok to allow longer timeout for Homebrew/core for local development on e.g. slower computers or debug reasons. Third-party CI are free to work however they like.

It indeed wouldn't be easy to override this within the formula itself and if people tried it would be really obvious to any code reviewer.

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.

Hold on merging this for now, about to jump on a plane but want to discuss this more, thanks!

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.

I'd rather we didn't move forward with this for now. As mentioned in https://github.com/orgs/Homebrew/discussions/5692#discussioncomment-11031721 I don't think this is a great use of brew test and, moreso, I'd rather we didn't add new options like this we'll have to essentially maintain indefinitely unless we have at least multiple people asking. Sorry @ZhongRuoyu!

@ZhongRuoyu
Copy link
Member Author

I don't think this is a great use of brew test

I agree that formula tests should be brief so this shouldn't be needed in general. However, I do believe there is value in giving some degree of freedom like this to third-party taps, and to developers who test formulae (including core formulae) locally or on slow (even emulated) machines as discussed in earlier comments in this PR.

For third-party tap usage: it's similar to how we allow build options and platform-dependent binary URLs. We don't do these in official formulae for good reasons, yet third-party taps are still free to do this as they wish.

For development on local or slow machines, this is also something developers can benefit from. Some tests, though intended to be short, are just running long for whatever reasons -- expected or unexpected. When run on slow machines, it would allow those to pass without having to make sacrifices in the test; when debugging slow tests locally, it would be nice to have an option to specify a custom timeout to allow such tests to at least finish.

Disclosure: I know I'm going to talk about a fairly unsupported use case, but I'm personally hoping to see this because I maintain taps (some of which are private) that build formulae on QEMU-emulated aarch64 Linux which is super slow (brew config takes ~1 minute). It will hopefully unblock ZhongRuoyu/homebrew-portable-ruby-aarch64-linux#14 after being stalled for quite a while.

I'd rather we didn't add new options like this we'll have to essentially maintain indefinitely unless we have at least multiple people asking

I hope you can relook at this taking my personal use case into consideration!

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Seems worth it for development on slow machines TBH. If we don't want to officially support it we can just leave the environment variable undocumented.

@MikeMcQuaid
Copy link
Member

It will hopefully unblock ZhongRuoyu/homebrew-portable-ruby-aarch64-linux#14 after being stalled for quite a while.

How long does that take?

I hope you can relook at this taking my personal use case into consideration!

We're up to two people requesting the option from one 😉.

How were you working around this before?

Does it make sense to have our brew test timeout as-is or should it be globally raised/lowered/platform dependent?

@ZhongRuoyu
Copy link
Member Author

ZhongRuoyu commented Oct 25, 2024

How long does that take?

Since it timed out, I don't have the exact answer how long brew test $my_tap/portable-ruby took, but as an estimate, in one of the test steps:

On x86_64 Linux (native):

==> /tmp/portable-ruby-test-20240826-98136-qn3zvd/bin/gem install byebug
...
Done installing documentation for byebug after 5 seconds

On aarch64 Linux (QEMU-emulated):

==> /tmp/portable-ruby-test-20240826-304774-tvmmg5/bin/gem install byebug
...
Done installing documentation for byebug after 156 seconds

Also, running $my_tap/portable-openssl tests (during build-time so not restricted by timeout) took ~5 minutes when run natively, and ~57 minutes on QEMU.

How were you working around this before?

For some reason I couldn't figure out, running QEMU on GitHub Actions runners have been much slower than it used to be since mid to late May. So, it's not that I was able to work around this, it's just that it used to be possible to run the tests within the time limit, but not anymore. The build logs for the last known good run are not available anymore, but from memory, it was already close to the 5-minute timeout.

Does it make sense to have our brew test timeout as-is or should it be globally raised/lowered/platform dependent?

I'd say the default timeout of 5 minutes works in general. Running brew with QEMU emulation is probably a bit extreme 😅.

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.

Ok, thanks folks. I agree with @carlocab: let's add this environment variable but keep it undocumented for now and note somewhere that it's considered a private API that we may remove in future if needed/it causes issues.

Intended mainly for third-party formulae, this allows formula tests to
be run with a customised timeout.

This is a private, undocumented API and subject to change.

Co-authored-by: Carlo Cabrera <github@carlo.cab>
@ZhongRuoyu ZhongRuoyu force-pushed the env-config-test-timeout branch from cafcfc5 to d2cdd99 Compare October 26, 2024 09:33
@ZhongRuoyu ZhongRuoyu changed the title Add HOMEBREW_TEST_TIMEOUT_SECS env config Add HOMEBREW_TEST_TIMEOUT_SECS env var Oct 26, 2024
@ZhongRuoyu
Copy link
Member Author

Thanks for your inputs everyone!

@ZhongRuoyu ZhongRuoyu enabled auto-merge October 26, 2024 09:37
@ZhongRuoyu ZhongRuoyu merged commit 59d56f8 into master Oct 26, 2024
27 checks passed
@ZhongRuoyu ZhongRuoyu deleted the env-config-test-timeout branch October 26, 2024 09:42
ZhongRuoyu added a commit to ZhongRuoyu/homebrew-portable-ruby-aarch64-linux that referenced this pull request Oct 26, 2024
This, added in Homebrew/brew#18629, will hopefully allow the tests run
on QEMU to pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants