Add --default-time-limit #3224
Add --default-time-limit #3224
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3224 +/- ##
============================================
- Coverage 82.69% 82.61% -0.08%
- Complexity 3517 3528 +11
============================================
Files 143 143
Lines 9294 9315 +21
============================================
+ Hits 7686 7696 +10
- Misses 1608 1619 +11
Continue to review full report at Codecov.
|
src/TextUI/Command.php
Outdated
| @@ -1111,6 +1117,7 @@ protected function showHelp(): void | |||
| --disallow-test-output Be strict about output during tests | |||
| --disallow-resource-usage Be strict about resource usage during small tests | |||
| --enforce-time-limit Enforce time limit based on test size | |||
| --default-time-limit=<sec> Maximum running time in seconds for unsized tests | |||
There was a problem hiding this comment.
I am not perfectly happy with that help text: (a) I personally think something like "Timeout in seconds ..." is more common? (b) Is "unsized tests" also understandable for others? How short/compact must the help text be: is "... for tests without either @small, @medium or @large" too long?
(Sorry, if this is too picky)
There was a problem hiding this comment.
First of all: feel free to be as picky as you want, it was your idea after all! There is no deadline pressure from management, so we can take all the time we need to get it from 'good enough' to 'exactly what I needed'. :-)
Mentioning the three sizes of tests might not fit. I do agree that using timeout would be nice. The config flag is already called *-time-limit and adding timeout will make it extra clear what it does. I'll commit an improved help text, let me know what you think.
|
@sebastianbergmann Getting the diff coverage above the threshold would require |
|
@epdenouden @sebastianbergmann @reinholdfuereder any update here? This change makes a lot of sense for our legacy phpunit suites and a merge will be much appreciated. Even fallback of all non-annotated tests to @Small is something that will do a huge favor. |
|
@karmakoder No news from my side. I agree it would be nice to see it merged. |
|
@sebastianbergmann huge +1 here as well, working with our php code base for functional and unit tests, particularly for UI issues having this functionality would be huge. Running in remote CI/CD SaaS solutions we have the need to gracefully timeout more and more to achieve automation autonomy without chasing down red herrings in our tests or CI/CD tool. We're currently using phpunit 6.0.13 for our test suite, for reference, which works great except having the ability to skip tests that are idle to rerun them later with this functionality. |
|
@bc-garybottom This pull request is for version 7. If it gets merged I'd be happy to backport it to 6.x for you if @sebastianbergmann would accept a PR. |
|
Sorry, but no new features for PHPUnit 6. |
|
@epdenouden I tried using your changes for this PR on my local copy of phpunit version 6.0.13 to create the intended behavior but somehow I dont see the tests timing/erroring out after defaultTimeLimit value that I defined in my config file. Any idea what I may be missing here? |
|
NM. Figured what was wrong with my local setup. All good. |
|
@karmakoder No problem. Let me know if you run into bugs or edge cases I didn't think of yet. |
|
@karmakoder Ad "Any update here?": Not really
|
|
@reinholdfuereder Apologies for not engaging with your recent comment, missed that completely! Let me give that a think. |
|
@sebastianbergmann if you have time to look at the PR during the Code Sprint, that'd be lovely 🔬 |
src/Framework/TestResult.php
Outdated
| /** | ||
| * @var int | ||
| */ | ||
| protected $defaultTimeLimit = 0; |
There was a problem hiding this comment.
New attributes should be private.
| @@ -84,6 +84,7 @@ class Command | |||
| 'disallow-test-output' => null, | |||
| 'disallow-resource-usage' => null, | |||
| 'disallow-todo-tests' => null, | |||
| 'default-time-limit=' => null, | |||
There was a problem hiding this comment.
For consistency, I would rather not add an option like --default-time-limit to set the default time limit on the CLI. The timeouts for @small, @medium, and @large can also only be configured in the configuration file.
There was a problem hiding this comment.
Yes, that is true. I asked @reinholdfuereder what the exact use case would be and this time limit is meant to be for tests that are not marked with a @size: #2085 (comment)
I know a bunch of older test collections with legacy would benefit from this.
There was a problem hiding this comment.
Not sure about the trailing=, what do you think?
There was a problem hiding this comment.
The = is required for --key=value, right? I have not found a clean way of doing --key[=value].
There was a problem hiding this comment.
The = means that the option requires a value.
There was a problem hiding this comment.
Thank you for clarifying, @sebastianbergmann!
|
@sebastianbergmann Thanks for merging this! |
|
@epdenouden After doing several rounds of testing, am learning that this default-time-limit doesnt always take effect for our tests. I basically backported your changes in to phpunit-6.0.13 but the behavior of tests getting marked as failed doesnt seem to be working 100% of the time. Any pointers on what could be the reason? Unfortunately, we cannot upgrade to latest phpunit version as several of our dependencies don't yet support higher versions of php and phpunit. If it helps, we can connect offline to discuss this. Thx in advance. |
Implementation for #2085
This is a convenience feature for people managing large(r) and potentially slow legacy test collections. Classic example: tests for database integration that have huge
@dataprovidersets or when using PHPUnit to test webservices/APIs.Note: I tried running around with my laptop, however I did not manage to validate the timer at relativistic speeds.