Configurable default for --enforce-time-limit

[ztec]

Currently by default all tests without large or medium annotation are considered as small for the timeout.

Currently by default, all tests without a tag among small/medium/large are not considered limited at all and do not fall into the time limitation set for those tags.

I would like to either configure a custom timelimit for them, or at least a configurable default behavoir so I can say all tests are large by default so i can only define some specific ones as small (Or the other way around to be more strict)

Thanks @Flyingmana for the original issue : #1799

[ztec]

related :
sebastianbergmann/phpunit-documentation#311

[sebastianbergmann]

I do not understand what you mean by "Currently by default all tests without large or medium annotation are considered as small for the timeout". For a test without a @small, @medium, or @large annotation no timeout is enforced.

[ztec]

I copied the original Bug report. You are right, you changed the behavior in the 4.7.

The idea stay the same : Be able to set a default value for untagged tests.

[sebastianbergmann]

A default for "untagged tests" makes no sense as no time limit is not (and should not) be enforced for them.

[reinholdfuereder]

@sebastianbergmann Sorry for revisiting this old issue and also #1799:

Frankly I also really like the idea behind which would allow enabling this test timeout checking for "legacy" projects containing loads of PHPUnit tests with just a single switch also for tests without @small annotation. The benefit and comfort and default maintenance effort would increase, as otherwise one would have to do (a) a big regex-search-replace initially, maybe combined with some manual work for "legacy" tests that e.g. do not use @test; and (b) for all newly written tests developer might forget to add the annotation again...

So it would allow for the defensiveness that I really like in the PHPUnit tool and its IMHO very sane and defensive defaults (e.g. being strict about useless tests)...

=> May I open a new (duplicate) issue of this one here (or could you re-open this one)? Or would that be insulting/stalking/... ;-)

[sebastianbergmann]

@reinholdfuereder I will not implement this myself but I might consider a pull request.

[reinholdfuereder]

Thanks for your fast and encouraging answer.

Frankly, I cannot exactly say when I will have time for this pull request and especially how long it will take me to be able to understand the existing code and prepare the contribution.

However, we could nonetheless try to discuss the requirements and API/usage:

1. Should it be rather (a) a switch meaning "treat tests with neither @small nor @medium nor @large annotation as @small"; or (b) a flag meaning "use this integer value as default timeout for all tests with neither @small nor @medium nor @large annotation in case the time limit is enforced"? (I think "abusing" the existing command line argument enforce-time-limit=<timeout> is not a good idea and will require yet another option in XML.)
2. What would be a good name for this new option? Maybe for (1a) treat-tests-with-unknown-size-as-small (maybe a bit longish), or for (1b) default-timeout-for-tests-with-unknown-size (again maybe a bit longish) or (I therefore think better) just default-timeout or default-test-timeout? (Existing related options are enforce-time-limit on CLI and beStrictAboutTestSize, as well as timeoutForLargeTests/timeoutForMediumTests/timeoutForSmallTests in XML configuration.)
3. Configuration via PHPUnit CLI argument as well as via XML configuration file must be supported.

Oh, and I was actually just wondering about:

• the performance penalty of using this (\SebastianBergmann\Invoker\Invoker)?
• the minimum timeout: as I have just read, it is 1s because of pcntl_alarm function => so one (unfortunately when considering plain/proper/real unit tests) cannot get too aggressive err ambitious ;-)

[epdenouden]

@reinholdfuereder just to confirm I read you correctly, you propose one of the following solutions:

1. an option to assume the size of unsized tests to be @small, after which the user can configure the timeout for @small tests. For example --default-test-size-is-small
2. an option to set the default timeout for tests of unspecified size, e.g. --default-time-limit=5. This timeout will only be enforced when the user also uses --enforce-time-limit

Personally I would prefer the second option as it more obviously puts a limit on legacy (=unmarked, unspecified) tests while leaving explicitly marked @someSize alone.

As I have recently added a bunch of CLI and XML-config parameters I would be happy to make a pull request for you.

Code reference:

phpunit/src/Framework/TestResult.php

Lines 623 to 645 in 00d26bb

	Timer::start();
	try {
	if (!$test instanceof WarningTestCase &&
	$test->getSize() != \PHPUnit\Util\Test::UNKNOWN &&
	$this->enforceTimeLimit &&
	\extension_loaded('pcntl') && \class_exists(Invoker::class)) {
	switch ($test->getSize()) {
	case \PHPUnit\Util\Test::SMALL:
	$_timeout = $this->timeoutForSmallTests;
	break;
	case \PHPUnit\Util\Test::MEDIUM:
	$_timeout = $this->timeoutForMediumTests;
	break;
	case \PHPUnit\Util\Test::LARGE:
	$_timeout = $this->timeoutForLargeTests;
	break;
	}

[reinholdfuereder]

@epdenouden I am wondering why you stumbled over this (closed) issue's recent comments so quickly -- big brother ;-) -- but this is actually really great and welcome!

Yes (2x): you understood correctly; and I also prefer this option (--default-time-limit=... when enforcing time limit)

@sebastianbergmann What is your opinion/suggestion?

[epdenouden]

@reinholdfuereder Well I sleep in the garbage bins around the back, so whenever people start stirring closed tickets I wake up ;-) But no the reason is much simpler. I have subscribed to all alerts for this repository so I can get a better understanding of how it works and what people do with it.

[reinholdfuereder]

@epdenouden Firstly, I love your humour!

Secondly, thanks a lot for your pull request preparation:

• At first glance it looks already finished, but you have not made a real pull request out of your branch https://github.com/epdenouden/phpunit/tree/issue-2085-default-time-limit yet -- why is that? (I assume you will not have to update the ChangeLog file by yourself, as this will done by the merger)
• Oh, and when just looking at your changes as a non-PHPUnit-insider the changes/commits look fine.
• What actually surprised me -- but that, I strongly guess, is not something new introduced by you -- is the need to specify the default value no less than 3 times!?

Of course after this has been merged, the PHPUnit documentation (https://github.com/sebastianbergmann/phpunit-documentation-english) will require some updates:

• src/risky-tests.rst and src/annotations.rst for the identical 'note' panel
• src/textui.rst for new CLI option
• configuration.rst for new XML option (and also not yet documented existing 'enforceTimeLimit' option)

If it helps, I could try to prepare those documentation updates, referencing your final pull request...

[epdenouden]

Hello @reinholdfuereder!
That is a quick review, thank you! I have indeed not made a PR yet because it didn't yet work. Something to do with the Invoker not being loaded or existing. I will figure that out later today.

And yes, having to specify the default value multiple times is not very elegant. Currently the PHPUnit community is discussing what to refactor for later versions of version 7.x and what needs a complete rewrite for version 8. In short, the whole pipeline from user-facing (command line, XML-config) to the internals could use a good powerwash.

[reinholdfuereder]

Ad "Invoker" problem:

• The documentation says:

A time limit can be enforced for the execution of a test if the PHP_Invoker package is installed and the pcntl extension is available.

• In my case that was fine, but it failed with "pcntl_signal() has been disabled for security reasons", because in one of the used "php.ini" files there was: disable_functions = pcntl_alarm,pcntl_fork,pcntl_waitpid,pcntl_wait,pcntl_wifexited,pcntl_wifstopped,pcntl_wifsignaled,pcntl_wexitstatus,pcntl_wtermsig,pcntl_wstopsig,pcntl_signal,pcntl_signal_dispatch,pcntl_get_last_error,pcntl_strerror,pcntl_sigprocmask,pcntl_sigwaitinfo,pcntl_sigtimedwait,pcntl_exec,pcntl_getpriority,pcntl_setpriority, => this had to be reduced (for CLI "php.ini") by pcntl_alarm and pcntl_signal

[epdenouden]

Yeah, I noticed the dependency on installing Invoker. I will add a warning to the CLI when using --enforce-time-limit when not having pcntl enabled and Invoker available.

[reinholdfuereder]

+1 (Two explicit specific/different warnings in case of missing dependencies are an excellent idea!)

[epdenouden]

@reinholdfuereder I will figure out what to do with the documentation, as the docs need updating for both 7.2 and newer versions.

[reinholdfuereder]

@epdenouden Thanks a lot for your work (the pull request). And also thanks in advance for taking care of the documentation too: frankly, I had not even thought about that being merged into PHPUnit 7.2 too, mostly because it is not a bug fix, but rather an addition/enhancement or new feature?

[epdenouden]

@reinholdfuereder Some work of mine has ended up in 7.2, some newer work in 7.3 and I need to add documentation for both still. This feature is not a bug fix, so it will probably be in the Changelog for a 7.3.x minor.

I need to do quite a bit of documenting as my PRs have added a few CLI, config and behaviour changes. And no worries, I'm happy to do it. :)

[epdenouden]

@sebastianbergmann I've finished pull request #3224 to provide functionality described by @reinholdfuereder

[reinholdfuereder]

@epdenouden Thanks a lot for your efforts and your instant/fast implementation!!!

There is one small item in the PR that grabbed my attention: The check for disable_functions is kind of hidden by the "PHP extension pcntl is required for enforcing time limits" warning. Frankly I am no PHP expert, so I was happy about the standard PHP warning (?) message "pcntl_signal() has been disabled for security reasons" which pointed me (more or less) directly to disable_functions; which I think, is no more the case?

[epdenouden]

@reinholdfuereder As a compromise, could you live with an expanded error message that mentions both the module pcntl being required AND it saying something like ...and pcntl* is not in disabled_functions`?

I want to avoid having to add a whole matrix of all variants of (un)availability of modules and their disabled_* states.

[reinholdfuereder]

@epdenouden Sorry if I might not understand you correctly: my maybe naive personal order of preferences is (1) not checking disable_functions at all (and let standard PHP do its duties like warning about "disabled for security reasons"), (2) keeping the check and extending the error message as you just suggested, and (3) the current code.

So for (1) instead of:
if (!\extension_loaded('pcntl') || \strpos(\ini_get('disable_functions'), 'pcntl') !== false) {
I "only" had in mind:
if (!\extension_loaded('pcntl')) {

If however this (1) is not defensive enough (for you), because it just warns about "disabled for security reasons" and then executes without error without enforcing timeouts, then I can perfectly live with (2), i.e. your compromise.

[reinholdfuereder]

@sebastianbergmann Do you mind re-opening this issue based on PR #3224 by @epdenouden? Considering the pending (a) little review and (b) approval by you maybe presumably targetting PHPUnit 7.4?

[epdenouden]

@reinholdfuereder The pull request has made into into master :)

[reinholdfuereder]

This is excellent news! And congrats! (Sorry for the delay of my response)