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 #5718 (comment) and #5718 (comment)

• "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 😅

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]

I'm wondering if we should trigger those events once per test, or once per test repeat attempt

currently Passed is triggered once per test, and Finished once per test repeat attempt but I don't think it is satisfying

[nikophil]

another solution would be to introduce new events: Repeat Attempt Started / Repeat Attempt Passed / Repeat Attempt Finished

I wont work further on the events until we discuss about them

[staabm]

another solution would be to introduce new events: Repeat Attempt Started / Repeat Attempt Passed / Repeat Attempt Finished

This sounds good to me. I don't have much experience with events though.

Lets wait for Sebastian on this one

[nikophil]

I tried to not change those names for the first attempt, because I don't know if the change in the name should be considered as a BC break

[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

[nikophil]

regarding to tests which depends on a test returning a value, I've opted for a very simple solution: if the last retry attempt of the depending test returns something different than null, then we skip the test and emit a PHPUnit warning.

This is a naive approach, but it prevents something much more complex for handling something which is a very niche problem.

Another simple solution would also to skip + trigger warning when $passedTests->hasReturnValue($dependencyTarget) returns true