Bring back `--repeat` CLI option

[nikophil]

fixes #5718

Here is an attempt to bring back --repeat CLI option.

The implementation follows what's proposed in https://github.com/sebastianbergmann/phpunit/issues/5718#issuecomment-2421955447 and https://github.com/sebastianbergmann/phpunit/issues/5718#issuecomment-2422253079

• "repeated" tests are run sequentially: if there are two tests A and B, they run in this order: AABB, whereas in PHPUnit 9 the order was ABAB
• the first failure of a repeated test skips all remaining "repetitions"
• if all repetitions of the test are successful, then the test as a whole is considered a success
• the first failure leads to:
  • no further attempt of executing the test
  • considering the test as a whole a failure
• a dependent test will run only if all the repetitions of its dependency are successful

I know that there had been some frictions around this feature, so I'd understand if this PR is refused. Nevertheless, the issue has been quiete for a long time, and many people have responded positively to the proposed solution.

I believe I’ve covered most of the cases, but please let me know if I’ve missed anything. Regarding the implementation: at first, I wanted to use an ExecutionOrderDependency to link repetitions, but that led to much more complex code. I think using a dedicated RepeatTestSuite object for these repetitions is cleaner.

[nikophil]

hey @sebastianbergmann

while re-reading your comment, I'm thinking I have misunderstood what you meant by saying:

the test as a whole is considered a success

I've actually duplicated the tests resulting in this output:

$ vendor/bin/phpunit --filter OnlyOneTest --repeat 10

..........                                                          10 / 10 (100%)

OK (10 tests, 10 assertions)

but I'm wondering if you were meaning that it would result in only one test execution, even if the test is run 10 times:

$ vendor/bin/phpunit --filter OnlyOneTest --repeat 10

.                                                                   1 / 1 (100%)

OK (1 tests, 10 assertions)

The only "visual" difference with a run without the --repeat option being the number of assertions:

$ vendor/bin/phpunit --filter OnlyOneTest

.                                                                   1 / 1 (100%)

OK (1 tests, 1 assertion)

If this is what you meant, I can fix the PR: I've already got something working with this other approach, just tell me

[sebastianbergmann]

Thank you for working on this, Nicolas! I won't be able to look at this for the next week or two, but I promise I'll look into it as soon as I can. In the meantime, did you know that I did some work on this about six months ago on the issue-5718/repeat branch? I didn't get very far and I've forgotten what problems I encountered. :(

[nikophil]

did you know that I did some work on this about six months ago on the issue-5718/repeat branch? I didn't get very far and I've forgotten what problems I encountered. :(

nope, I didn't! I'll have a check

[nikophil]

after checking your branch, I can see that regarding this comment, I did understood well your intent, and --repeat will actually change the number of tests in the whole testsuite :sweat_smile:

as notable differences, I can see that:

• the major difference is that I've introduced a RepeatTestSuite class, which helps a lot for keeping a link between repetitions and skipping further repetitions if a test fails.
• you disable the repetitions for a test which does not explicitly declare a : void return type. Not sure exactly why?
• you actually call $test = new $className($methodName); as many times as there are repetitions, while I'm using the same test instance, I'm not sure if it is a good idea

Regarding my implementation, I don't know how we should impact events:

• maybe for repetitions, we should change the name of the test, in order to mention the attempt number?
• I'm wondering if we should only call $emitter->testPassed() and PassedTests::instance()->testMethodPassed() only once all repetitions have passed?

Because of those questions about events, I did not create some functional tests which uses --debug, but I'll do once we decide what to do.

[sebastianbergmann]

you disable the repetitions for a test which does not explicitly declare a : void return type. Not sure exactly why?

If the test method is not declared to not return a value then it may return a value. Values returned by a test method are stored as other tests may depend on them using the #[Depends] attribute. What if a test is run multiple times and then return different values? This needs to be taken into account. My approach to tackling --repeat in the issue-5718/repeat branch was to explicitly disallow what is not (yet) supported and/or tested.

[sebastianbergmann]

you actually call $test = new $className($methodName); as many times as there are repetitions

Yes, to ensure we have a fresh instance for each test execution. I prioritise isolation over speed, so using clone instead of new seems like a risky performance optimisation.

[sebastianbergmann]

Regarding my implementation, I don't know how we should impact events

As I mentioned earlier, I don't really have the time to look into this right now. It's a complex topic, and it's been months since I last looked at it closely. However, I see that you are actively working on it and asking questions. I do not want to leave you hanging and will try my best to help you as quickly as I can. I'm afraid that answering quickly might not yield the best answers, so take what I write with a pinch of salt.

For now, I don't think we should implement --repeat without changing the existing test-related event objects and/or the value objects (such as PHPUnit\Event\Code\Test). The most minimal change I can think of would be to add an integer value to PHPUnit\Event\Code\Test to indicate the run. Of course, when --repeat is not used, this value would always be 1. However, I am not yet sure whether this change would be the best one.

[nikophil]

As I mentioned earlier, I don't really have the time to look into this right now. It's a complex topic, and it's been months since I last looked at it closely. However, I see that you are actively working on it and asking questions. I do not want to leave you hanging and will try my best to help you as quickly as I can

That's very nice, thank you, but I can wait 😊 Nevertheless, I may ask some questions as they come but I dont expect answers right away!

[nikophil]

If the test method is not declared to not return a value then it may return a value. Values returned by a test method are stored as other tests may depend on them using the #[Depends] attribute.

OK I wasn't aware of this behavior!

What if a test is run multiple times and then return different values? This needs to be taken into account.

I guess we have the choice between those solutions:

• always use the first returned value
• always use the last returned value
• each #N attempt in the dependent test gets the return value of the #N attempt of its dependency
• disallow to repeat a test which is dependent of another one which returns something (we can decide if "a test return something" if at least one of its repeat attempts returns something different from null. This would enable --repeat even for test which actually don't return anything, but do not have an explicit : void return type). In that case, trigger a warning

I think me preference goes to the last proposition. Because all the other solutions sound arbitrary. Moreover, we're facing a really niche problem

[staabm]

• disallow to repeat a test which is dependent of another one which returns something

IMO this sounds good. I would additionally emit a PHPUnit warning in such case.

[nikophil]

@staabm thanks for your input, that's exactly what I had in mind

do you have any thoughts about what to do with events?

[sebastianbergmann]

@nikophil I have not yet had time to look into this, so I cannot answer your question about the events yet. Please bear with me for a little while longer, hopefully only a little!

[nikophil]

Please bear with me for a little while longer, hopefully only a little!

yes no problem, I really didn't wanted to urge you, just wanted to have @staabm opinion

[sebastianbergmann]

@marcphilipp Does JUnit have a similar feature? If so, what do the Open Test Reporting events look like?

[marcphilipp]

JUnit Jupiter has a @RepeatedTest(value = 100, failureThreshold = 1) annotation that can be used instead of @Test. In that case, a intermediary container representing the test methods with a child node for each invocation is reported.

JUnit reports this as follows:

• Test class
  • Test method (container)
    • Repetition 1 of 100 (successful)
    • ...
    • Repetition 13 of 100 (failed)
    • Repetition 14 of 100 (skipped)
    • ...
    • Repetition 100 of 100 (skipped)

Repetition 14 to 100 wouldn't technically be required to be reported. We decided to do so for consistency.

If you want to handle this more transparently with an optimistic assumption that all tests pass, you could instead go for a flat structure like this:

• Test class
  • Test method [repetition 1 of up to 100] (successful)
  • Test method [repetition 2 of up to 100] (successful)
  • Test method [repetition 13 of 100] (failed)