InvocationMocker::withConsecutive() is deprecated in PHPUnit 9.6 and removed from PHPUnit 10

47 usages in core

Replaced mostly all places with willReturnOnConsecutiveCalls() but according to https://github.com/sebastianbergmann/phpunit/issues/4026 there's no direct replacement with reference to https://thephp.cc/articles/do-not-mock-what-you-do-not-own

It still needs work for remaining places but let's see what tests will tell

revert useless hunk

Another approach in https://stackoverflow.com/questions/75389000/replace-phpunit-method-with...

Still some instances but seems on the right track.

Addressed the files mentioned in #14

Patch in #15 is wrong, @Rishabh Vishwakarma did you read my comments that this places needs other approach?

Committed and pushed to 10.1.x and backported to 10.0.x and 9.5.x as a test-only fix to keep things in sync. Thanks!

Discussed this again with @mondrake in Slack. We now think that just replacing withConsecutive with willReturnOnConsecutiveCalls is not really the right thing to do - these two are not equivalent, and it just happens that here the return values probably don't matter so the tests happen to pass, but we have not made the correct change.

Therefore, reopening for revert once the current code freeze is over.

@mondrake reminded me that I had forgotten to revert these, this is done now.

As other users of PHPUnit are also calling for this functionality - or at least being able to test the same method with different parameters, even if the order is unimportant - then I think we should wait for this upstream pull request to be closed first: https://github.com/sebastianbergmann/phpunit/pull/5203

If it comes to it, the other option here is convert these mocks to use Prophecy instead, which already allows mocking the same method with different arguments. As an example MigrateSqlIdMapEnsureTablesTest would become

    $schema = $this->prophesize('Drupal\Core\Database\Schema');
    $schema->tableExists('migrate_map_sql_idmap_test')->willReturn(TRUE)->shouldBeCalledOnce();
    $schema->fieldExists('migrate_map_sql_idmap_test', 'rollback_action')->willReturn(FALSE)->shouldBeCalledOnce();
    $schema->fieldExists('migrate_map_sql_idmap_test', 'hash')->willReturn(FALSE)->shouldBeCalledOnce();
    $schema->fieldExists('migrate_map_sql_idmap_test', 'source_ids_hash')->willReturn(FALSE)->shouldBeCalledOnce();
    $schema->addField('migrate_map_sql_idmap_test', 'rollback_action', [
      'type' => 'int',
      'size' => 'tiny',
      'unsigned' => TRUE,
      'not null' => TRUE,
      'default' => 0,
      'description' => 'Flag indicating what to do for this item on rollback',
    ])->shouldBeCalledOnce();
    $schema->addField('migrate_map_sql_idmap_test', 'hash', [
      'type' => 'varchar',
      'length' => '64',
      'not null' => FALSE,
      'description' => 'Hash of source row data, for detecting changes',
    ])->shouldBeCalledOnce();
    $schema->addField('migrate_map_sql_idmap_test', 'source_ids_hash', [
      'type' => 'varchar',
      'length' => '64',
      'not null' => TRUE,
      'description' => 'Hash of source ids. Used as primary key',
    ])->shouldBeCalledOnce();
    $this->runEnsureTablesTest($schema->reveal());

No, Prophecy does not support call order: https://github.com/phpspec/prophecy/issues/130

I haven't reviewed our usage in detail but I suspect that in many/most cases the order is not actually important.

I reviewed this PR without reading the comments before (my bad!), as I was trying to write a test for #2873287: Dispatch events for changing content moderation states with an eye on this one.
I ended up with writing a constraint. Is not the most readable test code in the world, but I think it's good enough given we don't know where that will end up. The best past is that as far as I know that approach would be compatible for 9.5.x and 10.1.x and beyond.

Thoughts? https://www.drupal.org/project/drupal/issues/2873287#comment-15026320

No longer applies to 11.x

Posted in #testing but not sure the best way to test this one, if any pointers available.

From what I can tell it seems fine.

Testing passing I take as a positive sign.

Maybe the committers have another thought?

There is an unresolved thread with a question about the use of assertSame(). I am setting back to NR.

Rebased and replaced assertEquals with assertSame as adviced by @penyaskito.

Can't resolve thread, since I'm not the starter of this MR.

Looking at #44/45, this needs a green test result and Core Committers review, so back to RTBC, where this was before @quietone correctly pointed out the open thread on the MR.

Reversed replacing assertEquals with assertSame.

Like it, dropped a few minor nits and questions in the MR, mostly showing my ignorance :)
Back to NW for admin reasons, but I think we're either there or at the very least almost.

Thanks @mondrake, all clear now, RTBC.

@mondrake what do you think about the suggested solution using yield at https://github.com/sebastianbergmann/phpunit/issues/4026#issuecomment-14... ?

DX is slightly simpler and easier to convert for anyone that used it in contrib/custom tests, if we provide a trait similar to that and a simple CR - it's technically not our problem but it would be helpful.

I do think a lot of these uses don't strictly need to be consecutive, but the issue is really that you can't mock methods that take different arguments to return different results.

I prefer the simpler DX of the second one, unless there is a case where the reflection is required in order to discover additional arguments that aren't passed in the arrays? But perhaps we just shouldn't support that, as Sebastian Bergmann is trying to discourage use of this I think we should too - I feel this should only be for converting existing methods if at all possible, and we should try to find alternative approaches for new tests.

Going purely off the issue summary of replacing "withConsecutive" I applied the MR searched for withConsecutive and only one hit in a comment in MockTrait.php.

All phpstan lines have been removed too.

So from that standpoint believe it's good for 10.2

"But perhaps we just shouldn't support that, as Sebastian Bergmann is trying to discourage use of this I think we should too - I feel this should only be for converting existing methods if at all possible, and we should try to find alternative approaches for new tests."

I agree with this sentiment. This sort of approach is pretty brittle and I'd discourage it too. I expect there are various approaches like mocking less, using prophecy to tie the returns to specific arguments, or using data providers to deal with variations on duplicate code paths.

I also wouldn't hold this up on refactoring all of these tests though and all the individual reviews associated with that. Each one likely has their own unique problem and requires complicated understanding of the related system. This is a elegant stopgap.

So my only questions are:

1. Do we need to address this: "Do we need a CR in this case, if we are not suggesting to use the method?"
2. Should we create follow ups for removal/updates?

I think we should write a change record, but the change record should probably try to discourage use of this new method and instead suggest that people find alternative methods. This can still be useful for contrib or custom tests that are trying to solve their deprecations before we upgrade to PHPUnit 10, if we can't solve all cases immediately then I doubt they can either.

We should perhaps open followups to try and remove some of these calls, but as previously stated in this issue each case needs detailed inspection and there is no one size fits all solution, so perhaps we just start with a meta where we spin off issues for individual tests or groups of tests.

Created that followup meta to track replacing calls. #3406006: [META] Convert use of InvocationMocker::withConsecutive()

The CR mentions the need to rethink tests and does a good job explaining the stop gap. I think that's enough. Its going to be a lot easier to provide specific examples of how a developer might think about replacing this logic after core replaces its uses. So providing that with a later deprecation of MockTrait::consecutiveCalls makes more sense IMHO. We can provide some links between the CR's at that point to make them more findable as well.

All that to say, agree this is RTBC.

I'm triaging RTBC issues. I read the IS, the comments, the change record and the MR. I didn't find any unanswered questions. And we have a follow up to remove this 'stop gap'.

I updated the proposed resolution in the issue summary with the approach agreed to here. I also updated the CR to add emphasis not using this for new tests.

Leaving at RTBC.

I'm not convinced we should be adding code that we know we're going to deprecate - why don't we do #3406006: [META] Convert use of InvocationMocker::withConsecutive() and be done?

This was a step towards that for both us and contrib, to wean us off the method and continue unblocking PHPUnit 10. It doesn't appear trivial to refactor each case so this helper means we (and contrib) can use this as a stopgap.

On the other hand it doesn't look like the Symfony PHPUnit bridge upgrade is going anywhere fast and so we could postpone this and keep it on hold, in case we need it later, but try to migrate away in the mean time?

#3406006: [META] Convert use of InvocationMocker::withConsecutive() is very close, so we won't need this for core, but do we want to consider a shim for contrib? Moving back to review because things have changed since this was RTBCed and I'm not sure what the answer is.

So is this closed won't fix?

Yes I think so, although 'outdated' seems slightly more appropriate since we determined that core doesn't need the shim in the meantime.

There's change record draft for contrib
So maintainers can comment or reopen for common shim

++ good idea