Change @dataprovider to static in CommentLinkBuilderTest

I started off trying to see if we could have a single patch for the conversion, but this is turning out to be quite detailed work, with interconnections. If we go that way this will soon become unreviewable.

So I'm rescoping here to only two test conversions, and I think we'll have to address the rest in a patch per test converted.

Hope this makes sense.

1. Checked code changes and they are (mostly) correct Mock -> Prophecy conversions.
2. Pattern matches the one agreed in 📌 [PHPUnit 10] Provide a static alternative to ConfigMapperManagerTest::providerTestHasTranslatable Fixed
3. Green TestBot

Checking 1. for only these two tests was already quite a strain on my mind, so I can agree with splitting this up in to multiple/many issues.

NW to check some stuff. Will explain later.

I found a problem in this approach - we are not checking call predictions for the prophecies in the dataproviders. See the inline comments.

Essentially, $contact_form->getRecipients()->shouldBeCalledTimes(500)->willReturn($recipients); does generate a double that can be used in the test and behaves as instructed, but shouldBeCalledTimes(500) does not have any effect, since we are not checking the prediction post-factum. That's because in order to have the prediction checked, the prophet object that generated the prophecy should be available after the prophecy has been used. But in the approach so far the prophet object gets out of scope after the dataprovider is executed. More - dataproviders are executed during test discovery, i.e. well before test execution.

Now, the question is: should we support call prediction checking for objects in dataproviders? Probably, a purist would say that dataproviders are there to... provide immutable data for the test execution on the SUT, and the idea itself to test the data clashes with the principle of separation of concerns. But I am not a purist, so I am open to hear differently. If we need to support it, though, the implementation may be very tricky.

For completeness, checked with HEAD, and

   protected function getMockContactForm($recipients, $auto_reply) {
     $contact_form = $this->createMock('\Drupal\contact\ContactFormInterface');
-    $contact_form->expects($this->once())
+    $contact_form->expects($this->exactly(500))
       ->method('getRecipients')
       ->willReturn($recipients);
-    $contact_form->expects($this->once())
+    $contact_form->expects($this->exactly(10000))
       ->method('getReply')
       ->willReturn($auto_reply);

yields PHPUnit errors like

1) Drupal\Tests\contact\Unit\MailHandlerTest::testSendMailMessages with data set #0 (Mock_MessageInterface_de4c667e Object (...), Mock_User_ac545cec Object (...), array(array('page_mail', array(Mock_MessageInterface_de4c667e Object (...), Mock_User_ac545cec Object (...), Mock_ContactFormInterface_c7317a4d Object (...)), 'contact', 'admin@drupal.org, user@drupal.org', 'en', 'anonymous@drupal.org')))
Expectation failed for method name is "getRecipients" when invoked 500 time(s).
Method was expected to be called 500 times, actually called 1 times.

However, with PHPUnit 10 and moving dataproviders to static I'd say it would be unlikely support for this will still be there.

Actually, I found a (relatively) simple way to keep track of the Prophets created for each class with @dataproviders methods, and checking predictions at the end of each class in tearDownAfterClass(). I like it, it looks like it can make much more robust doubles with this...

Love it!

RTBC for this one, core committers/other big brains will chime in when this is on their RTBC-radar.

However in the already committed 📌 [PHPUnit 10] Provide a static alternative to ConfigMapperManagerTest::providerTestHasTranslatable Fixed we have a

--- a/core/modules/config_translation/tests/src/Unit/ConfigMapperManagerTest.php
+++ b/core/modules/config_translation/tests/src/Unit/ConfigMapperManagerTest.php
@@ -169,20 +169,16 @@ protected function getElement(array $definition) {
    * @return \Drupal\Core\Config\Schema\Mapping
    *   A nested schema element, containing the passed-in elements.
    */
-  protected function getNestedElement(array $elements) {
+  protected static function getNestedElement(array $elements) {
     // ConfigMapperManager::findTranslatable() checks for
     // \Drupal\Core\TypedData\TraversableTypedDataInterface, but mocking that
     // directly does not work, because we need to implement \IteratorAggregate
     // in order for getIterator() to be called. Therefore we need to mock
     // \Drupal\Core\Config\Schema\ArrayElement, but that is abstract, so we
     // need to mock one of the subclasses of it.
-    $nested_element = $this->getMockBuilder('Drupal\Core\Config\Schema\Mapping')
-      ->disableOriginalConstructor()
-      ->getMock();
-    $nested_element->expects($this->once())
-      ->method('getIterator')
-      ->willReturn(new \ArrayIterator($elements));
-    return $nested_element;
+    $nested_element = (new Prophet())->prophesize(Mapping::class);
+    $nested_element->getIterator()->shouldBeCalledTimes(1)->willReturn(new \ArrayIterator($elements));
+    return $nested_element->reveal();
   }

I think/suppose/guess we have to revisit that after agreeing and commiting this, because of the $nested_element->getIterator()->shouldBeCalledTimes(1) and do the TearDown-Tango there as well?

#8 yes, and 📌 [PHPUnit 10] Provide a static viable alternative to $this->prophesize() in data providers Needs work too.

Ah, didn't even think of that one, adding it as related.

Also, I would like to claim "Data Provider Prophets of Doom" as the name for my next metal band...

😂

I'm not convinced we are doing the right thing here. I feel that by making data providers static, the PHPUnit authors are implying that data providers should be fast and not really be doing any work - and to me this includes creating mocks. If we need custom mocks per test combination, I think we should be providing the values required to create the mock into the test method, but not the mock itself?

the PHPUnit authors are implying that data providers should be fast and not really be doing any work - and to me this includes creating mocks

I do not think that's the intent. Otherwise they would not implement something like

TestCase::createStub(), TestCase::createStubForIntersectionOfInterfaces(), and TestCase::createConfiguredStub() are now static (and can be used from static data provider methods)

in 10.3.0, https://github.com/sebastianbergmann/phpunit/blob/main/ChangeLog-10.3.md

IMHO, the purpose of static dataproviders is to really separate the test object from the data used by the test. I wouldn't be surprised if in the future dataproviders will have to move to a separate class from their consumers.

In principle I agree though if we can move mocking from the dataprovider to the test it would be better, but I'm not sure this can be fits-for-all cases.

So would next steps be for this issue?

There was conversation between @catch, @mondrake, and @longwave on #contribute in slack. I was just a fly on the wall as this is above my head.

@mondrake proposed

I think the options here are:
1. we do this issue, which introduces a new API, allow checking call count expectations, etc - Symfony has done something similar.
2. we just drop all the calls to count expectation methods in dataproviders that use prophecy --> but in that case we need to correct other conversions and possibly find a way to prevent them from being called (PHPStan rule?)
3. we drop all the usage of Prophecy and createMock (that are not available statically) and convert all dataproviders to only use createStub - to be investigated if possible and require adjusting any conversion done already
4. ??

@catch

If upstream isn't helping we might want to do #1 and then open follow-ups to explore 2-4, since #1 would allow for conversions without massive refactoring of each individual test.

@longwave

a possible option 4 is check what other projects with large PHPUnit suites have done or are planning to do about this, if they also ran into it - unsure if there are any though

So tagging for follow ups but marking as this may be the best way forward (from my understanding), based on upstream phpunit.

If I paraphrased wrong sorry!

I'm triaging RTBC issues → . I read the IS and the comments.

@smustgrave, thanks for recording the conversation from Slack. I do think it is better for all if summaries are in the Issue Summary. Having a link to the Slack conversation would help as well, which is https://drupal.slack.com/archives/C1BMUQ9U6/p1694529624846449

I read that thread and I am not convinced that there is agreement to any of the 4 options.

I am setting this back to Needs Review for make sure we agree on the direction. I will also mention this issue in committer slack.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

rebased

What was the outcome of #15 - did we do research into how other projects are handling this.

I'm in the same camp as @longwave in #12 - I think we should be passing values in the data provider and create the mocks in the ::testSomething methods

Or failing that another option is to return a Closure that is bound to the class. In client projects we do that with Drupal Testing Traits because Drupal APIs aren't yet available in data providers.

public function someDataProvider(): array {
  return [
   'some node thing' => [fn(array $values) => $this->createSomeNode($values)],
  ],
}

/**
 * @dataProvider someDataProvider
*/
public function testSomething(callable $factory): void {
  $user = $this->createUser();
  $node = $factory(['uid' => $user]);
  // ...
}

I _think_ that would work here

Not sure an option was agreed upon. Moving to NW so the issue summary can be updated to include what the decided outcome should be.

Changing to Needs review because this needs an answer on the approach.

@larowlan support @longwave in passing values in the data provider and create the mocks in the ::testSomething methods. See #12.

Tried an alternative approach for CommentLinkBuilderTest in MR!6661.

MailHandlerTest is a bit more messy but I think we could clean it up to make the test more readable at the same time.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

#23 I think it's better split this issue in two then, one for the CommentLinkBuilderTest fix and one for MailHandlerTest.

mondrake → closed merge request !4230

I think I can RTBC this given it’s a different approach grim the previous MR. It’s in line with latest issues of this kind where instead of opening mocks in the dataprovider, we are passing data for the mocks to be created in the test itself.

Committed and pushed e021637080 to 11.x and 9ad8446eb0 to 10.3.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.