-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
Conversation
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 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! |
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. |
There was a problem hiding this 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!
There was a problem hiding this 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!
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 (
I hope you can relook at this taking my personal use case into consideration! |
There was a problem hiding this 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.
How long does that take?
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 |
Since it timed out, I don't have the exact answer how long
On aarch64 Linux (QEMU-emulated):
Also, running
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.
I'd say the default timeout of 5 minutes works in general. Running |
There was a problem hiding this 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 <[email protected]>
cafcfc5
to
d2cdd99
Compare
HOMEBREW_TEST_TIMEOUT_SECS
env configHOMEBREW_TEST_TIMEOUT_SECS
env var
Thanks for your inputs everyone! |
This, added in Homebrew/brew#18629, will hopefully allow the tests run on QEMU to pass.
brew style
with your changes locally?brew typecheck
with your changes locally?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.