Convert ImageEffectsTest & ToolkitTest into Kernel tests

Mostly looks good. Definitely +1 to Kernel-ifying these if possible. :)

Disclaimers:
- This is my first time ever looking at any of these particular tests.
- I didn't closely verify that all the functions that got moved around are exact replicas, nothing got lost, etc. I assume it's cool, but a closer review would be great before RTBC.

Some questions/concerns/nits:

1. +++ b/core/modules/image/tests/modules/image_module_test/image_module_test.module
   @@ -16,12 +16,12 @@ function image_module_test_file_download($uri) {
   +  global $image_module_test_counter;
   +  // Increase the test counter, signaling that image effects were processed,
   +  // rather than being served from the cache.
   +  $image_module_test_counter++;
   

   Would love to see $image_module_test_counter declared explicitly, with a very clear phpDoc comment about it, why it's a raw global like this, @see to where it's used, etc.

2. +++ b/core/tests/Drupal/FunctionalTests/Image/ToolkitTestBase.php
   @@ -2,12 +2,19 @@
   +@trigger_error('The ' . __NAMESPACE__ . '\ToolkitTestBase is deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. There is no replacement provided as toolkit operations should be tested as kernel tests. See https://www.drupal.org/node/3035573.', E_USER_DEPRECATED);
   

   "The ...\ToolKitTestBase class is deprecated...".

   Also, should we mention ToolkitTestTrait here as a partial replacement?

3. +++ b/core/tests/Drupal/FunctionalTests/Image/ToolkitTestBase.php
   @@ -2,12 +2,19 @@
   + *   no replacement provided as toolkit operations should be tested as kernel
   + *   tests.
   

   Same here: mention the new Trait?

4. +++ b/core/tests/Drupal/Tests/Traits/Core/ToolkitTestTrait.php
   @@ -0,0 +1,87 @@
   + * Provides common methods for tests dealing with image toolkits.
   

   Seems like this should say it's got \Drupal::state() dependencies so no one tries to use it in a Unit test, right?

   In fact, seems like it depends on the image_test module being enabled, too, right? I'm only inferring that from reading this patch.

Nice idea, just needs more polishing

1. +++ b/core/modules/image/tests/modules/image_module_test/image_module_test.module
   @@ -16,12 +16,16 @@ function image_module_test_file_download($uri) {
   +  // Variable accessed from the ImageEffectsTest::testImageEffectsCaching() test
   +  // and used to signal if the image effect plugin definitions were computed or
   +  // were retrieved from the cache.
   +  // @see \Drupal\Tests\image\Kernel\ImageEffectsTest::testImageEffectsCaching()
   +  global $image_module_test_counter;
   +  // Increase the test counter, signaling that image effects were processed,
   +  // rather than being served from the cache.
   +  $image_module_test_counter++;
   
   +++ b/core/modules/image/tests/src/Kernel/ImageEffectsTest.php
   @@ -0,0 +1,201 @@
   +    // Variable incremented in image_module_test_image_effect_info_alter() every
   +    // time the image effect plugin definitions are recomputed.
   +    // @see image_module_test_image_effect_info_alter()
   +    global $image_module_test_counter;
   

   instead of new global makes sense to use state service as other tests doing

2. +++ b/core/modules/image/tests/src/Kernel/ImageEffectsTest.php
   @@ -0,0 +1,201 @@
   +    // @todo: Need to test upscaling.
   ...
   +    // @todo should test the keyword offsets.
   ...
   +    // @todo: Need to test with 'random' => TRUE
   

   todos needs follow-up issue link

3. +++ b/core/modules/image/tests/src/Kernel/ImageEffectsTest.php
   @@ -0,0 +1,201 @@
   +   * Test the 'image_crop' effect.
   ...
   +   * Test the 'image_scale_and_crop' effect with an anchor.
   ...
   +   * Test the 'image_desaturate' effect.
   ...
   +   * Test the image_rotate_effect() function.
   

   s/Test/Tests

4. +++ b/core/modules/image/tests/src/Kernel/ImageEffectsTest.php
   @@ -0,0 +1,201 @@
   +    $manager = $this->container->get('plugin.manager.image.effect');
   +    $effects = $manager->getDefinitions();
   ...
   +    $manager = $this->container->get('plugin.manager.image.effect');
   +    $effect = $manager->createInstance($effect_name, ['data' => $data]);
   

   I bet that this manager could be a protected variable instantiated in setUp()

#11.1 is a much better solution to #8.1, thanks! :)
+1 to the rest of #11, too.

Cheers,
-Derek

@claudiu.cristea looks you're mixed patch & interdiff from locale module

1. +++ b/core/modules/image/tests/src/Functional/ImageEffectsTest.php
   @@ -3,186 +3,19 @@
   +class ImageEffectsTest extends BrowserTestBase {
   

   Can this be a kerneltest too? Or maybe https://www.previousnext.com.au/blog/drupal-8-ftw-it-test-or-it-form-act...

2. +++ b/core/modules/image/tests/src/Kernel/ImageEffectsTest.php
   @@ -0,0 +1,219 @@
   +    $this->imageEffectPluginManager = \Drupal::service('plugin.manager.image.effect');
   

   We can get this from $this->container->get()

3. +++ b/core/modules/image/tests/src/Kernel/ImageEffectsTest.php
   @@ -0,0 +1,219 @@
   +    $state = \Drupal::state();
   

   We can get this from $this->container->get(). Possibly in setUp() ?

4. +++ b/core/tests/Drupal/KernelTests/Core/Image/ToolkitTest.php
   @@ -0,0 +1,119 @@
   +    /** @var \Drupal\Core\ImageToolkit\ImageToolkitInterface $toolkit */
   

   We can do assert($toolkit instanceof ImageToolkitInterface) instead.

Nice! LGTM

Unrelated fail

1. +++ b/core/tests/Drupal/FunctionalTests/Image/ToolkitTestBase.php
   @@ -2,12 +2,21 @@
   -use Drupal\Component\Render\FormattableMarkup;
   +@trigger_error('The ' . __NAMESPACE__ . '\ToolkitTestBase class is deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. There is no replacement provided as functional test base class because toolkit operations should be tested as kernel tests. \Drupal\KernelTests\Core\Image\ToolkitTestTrait trait has been added to provide a similar functionality for toolkit kernel tests. See https://www.drupal.org/node/3035573.', E_USER_DEPRECATED);
   

   We shouldn't be removing the use statement here - that'll break this test base class.

2. +++ b/core/tests/Drupal/KernelTests/Core/Image/ToolkitTest.php
   --- /dev/null
   +++ b/core/tests/Drupal/KernelTests/Core/Image/ToolkitTestTrait.php
   

   This is not the correct namespace for test traits. They can go in core/tests/Drupal/Tests/Traits/Core now - so let's add an Image directory in there and move this.

3. +++ b/core/tests/Drupal/KernelTests/Core/Image/ToolkitTestTrait.php
   @@ -0,0 +1,89 @@
   +    \Drupal::state()->delete('image_test.results');
   ...
   +    return $this->container->get('state')->get('image_test.results', []);
   ...
   +    $image_factory = $this->container->get('image.factory');
   

   As this trait can be used by both kernel and functional tests this is probably better done with \Drupal at this point. It makes less assumptions about the properties of the class it is being used from.

Looks good to me, just some minor stuff we should fix:

1. +++ /dev/null
   @@ -1,89 +0,0 @@
   -    $this->assertIdentical('foo_derived', $blur->getPluginId(), "'Blur' operation overwritten by derivative.");
   -    $this->assertIdentical('bar', $invert->getPluginId(), '"Invert" operation inherited from base plugin.');
   

   This is a bit sad that we lose the nice messages. We can still pass them in the new phpunit methods, right? Can we just keep them?

2. +++ b/core/tests/Drupal/FunctionalTests/Image/ToolkitTestBase.php
   @@ -2,12 +2,22 @@
   + * @deprecated in Drupal 8.8.0 and will be removed before Drupal 9.0.0. There is
   

   Format is "@deprecated in drupal:8.8.0 and is removed from drupal:9.0.0.", see #3024461: Adopt consistent deprecation format for core and contrib deprecation messages. Same for the trigger_error(), should use the new format.

I'm a bit concerned that we are throwing away all the messages with the converted assertions. Yes, sometimes they are a bit redundant, but in other cases they give very useful insight what the assertion does/expects. Any reason why you removed them?

I fixed the 2 minor stuff suggested at #30..

This was RTBCed before, reviewed by @alexpott on #26, after which the concerns were addressed. Regarding the messages, I think the response on #32 is valid and is what we've been doing on other issues. So I think it's safe to go back to RTBC.

+++ /dev/null
@@ -1,223 +0,0 @@
-    $calls = $this->imageTestGetAllCalls();
-    $this->assertEqual($calls['scale'][0][0], 10, 'Width was passed correctly');
-    $this->assertEqual($calls['scale'][0][1], 10, 'Height was based off aspect ratio and passed correctly');
-  }

I think we should do claudiu's suggestion and convert these messages to comments. The assertions are not very self documenting by themselves given it relies on array index.

I applied claudiu's suggestion, was my mistake by the way.

1. +++ /dev/null
   @@ -1,223 +0,0 @@
   -
   -    // Check the parameters.
   -    $calls = $this->imageTestGetAllCalls();
   -    $this->assertEqual($calls['scale_and_crop'][0][0], 7.5, 'X was computed and passed correctly');
   -    $this->assertEqual($calls['scale_and_crop'][0][1], 0, 'Y was computed and passed correctly');
   -    $this->assertEqual($calls['scale_and_crop'][0][2], 5, 'Width was computed and passed correctly');
   -    $this->assertEqual($calls['scale_and_crop'][0][3], 10, 'Height was computed and passed correctly');
   -  }
   

   I think we need to convert most/all of the messages to comments though for example the messages above:

2. +++ b/core/modules/image/tests/src/Kernel/ImageEffectsTest.php
   @@ -0,0 +1,245 @@
   +
   +    // Check the parameters.
   +    $calls = $this->imageTestGetAllCalls();
   +    $this->assertEquals(0, $calls['crop'][0][0]);
   +    $this->assertEquals(1, $calls['crop'][0][1]);
   +    $this->assertEquals(3, $calls['crop'][0][2]);
   +    $this->assertEquals(4, $calls['crop'][0][3]);
   +  }
   

   Are lost here. Really anything where we're asserting against array indexes should definitely be retained.

Appologies I was too hasty to set back to RTBC. Restoring all messages here as comments.

Re-rolling against 9.1.x-dev.

Fixing test case failure issue in #40, please review.

Drupal 9 was released, so we need to update the deprecation messages.

Why do we move the assertion messages to comments? We can leave them because assertEquals() has a message option as third parameter? I think having assertion messages is much better than comments because then you see them also in test output.

Here I have updated the depreciation messages.

Thanks!

Can we also use the assertion messages instead of comments as I suggested in #43?

Re: #43 / #46: See #3131946: [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages -- not fully decided yet, but core is moving away from custom assertion messages when they add nothing over the default assertions from PHPUnit itself.

Moving back to Needs Review.

It's not finalised yet, but at least two core committers are positive about #3154762: [policy, no patch] All new Drupal 10 code should use appropriate type hints wherever possible, so I would suggest to start that. Also, see #3138078: [D9.3 beta - w/c Nov 8, 2021] Add a 'void' return typehint to custom assert* methods.

1. +++ b/core/modules/image/tests/src/Kernel/ImageEffectsTest.php
   @@ -0,0 +1,265 @@
   +  protected function assertImageEffect($effect_name, array $data) {
   

   should have a : void return typehint, and $effect_name a string typehint.

2. +++ b/core/tests/Drupal/FunctionalTests/Image/ToolkitTestBase.php
   @@ -2,12 +2,22 @@
   +@trigger_error('The ' . __NAMESPACE__ . '\ToolkitTestBase class is deprecated in Drupal 9.0.0 and will be removed before Drupal 10.0.0. There is no replacement provided as functional test base class because toolkit operations should be tested as kernel tests. \Drupal\KernelTests\Core\Image\ToolkitTestTrait trait has been added to provide a similar functionality for toolkit kernel tests. See https://www.drupal.org/node/3035573.', E_USER_DEPRECATED);
   ...
   + * @deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. There is
   

   It's for deprecation in 9.1.0 now, and we miss a deprecation test.

3. +++ b/core/tests/Drupal/KernelTests/Core/Image/ToolkitTest.php
   @@ -0,0 +1,123 @@
   +    $this->assertSame(TRUE, $calls['apply'][0][1]['p2']);
   

   $this->assertTrue

4. +++ b/core/tests/Drupal/KernelTests/Core/Image/ToolkitTest.php
   @@ -0,0 +1,123 @@
   +    assert($toolkit instanceof ImageToolkitInterface);
   

   $this->assertInstanceOf

5. +++ b/core/tests/Drupal/Tests/Traits/Core/Image/ToolkitTestTrait.php
   @@ -0,0 +1,89 @@
   +  protected function imageTestReset() {
   

   should have a : void return typehint.

6. +++ b/core/tests/Drupal/Tests/Traits/Core/Image/ToolkitTestTrait.php
   @@ -0,0 +1,89 @@
   +  public function assertToolkitOperationsCalled(array $expected) {
   

   should have a : void return typehint.

7. +++ b/core/tests/Drupal/Tests/Traits/Core/Image/ToolkitTestTrait.php
   @@ -0,0 +1,89 @@
   +  protected function imageTestGetAllCalls() {
   

   should have an : array return typehint.

8. +++ b/core/tests/Drupal/Tests/Traits/Core/Image/ToolkitTestTrait.php
   @@ -0,0 +1,89 @@
   +  protected function getImage() {
   

   should have an : ImageInterface return typehint.

implementing suggestions from #49

1. +++ b/core/tests/Drupal/KernelTests/Core/Image/ToolkitTest.php
   @@ -0,0 +1,123 @@
   +    $this->assertTrue(TRUE, $calls['apply'][0][1]['p2']);
   

   should be $this->assertTrue($calls['apply'][0][1]['p2']);

2. +++ b/core/tests/Drupal/Tests/Traits/Core/Image/ToolkitTestTrait.php
   @@ -0,0 +1,89 @@
   +  protected function getImage(): \Drupal\Core\Image\ImageInterface {
   

   should be protected function getImage(): ImageInterface { and a use statement added at the top of the file with the fully qualified class name

Worked on #51

Thanks

A diff made using git diff --cached 9.1.x -M20% so to visually see what was changed from the functional version of the tests.

1. +++ b/core/modules/image/tests/src/Kernel/ImageEffectsTest.php
   @@ -0,0 +1,265 @@
   +    $this->assertImageEffect('image_rotate', [
   +      'degrees' => 90,
   +      'bgcolor' => '#fff',
   +    ]);
   +    $this->assertToolkitOperationsCalled(['rotate']);
   ...
   +  /**
   +   * Asserts the effect processing of an image effect plugin.
   +   *
   +   * @param string $effect_name
   +   *   The name of the image effect to test.
   +   * @param array $data
   +   *   The data to be passed to the image effect.
   +   */
   +  protected function assertImageEffect(string $effect_name, array $data): void {
   

   The assertImageEffect method is always followed by an assertToolkitOperationsCalled(). I think think the signature for assertImageEffect should be:

   protected function assertImageEffect(array $expected_operations, string $effect_name, array $data): void {
   

   and it should call assertToolkitOperationsCalled() because without that call we're not really asserting the effect of processing an image effect plugin.

   And also does this method belong on the trait? I think it might be useful for modules that provide an image effect.

2. +++ b/core/tests/Drupal/FunctionalTests/Image/ToolkitTestBase.php
   @@ -2,12 +2,22 @@
   +@trigger_error('The ' . __NAMESPACE__ . '\ToolkitTestBase class is deprecated in Drupal 9.0.0 and will be removed before Drupal 10.0.0. There is no replacement provided as functional test base class because toolkit operations should be tested as kernel tests. \Drupal\KernelTests\Core\Image\ToolkitTestTrait trait has been added to provide a similar functionality for toolkit kernel tests. See https://www.drupal.org/node/3035573.', E_USER_DEPRECATED);
   

   This is not the correct format and doesn't have the correct versions.

3. +++ b/core/tests/Drupal/KernelTests/Core/Image/ToolkitTest.php
   @@ -0,0 +1,123 @@
   +  /**
   +   * {@inheitdoc}
   +   */
   

   {inheritdoc}

4. +++ b/core/tests/Drupal/Tests/Traits/Core/Image/ToolkitTestTrait.php
   @@ -0,0 +1,90 @@
   +  /**
   +   * Sets up an image with the custom toolkit.
   +   *
   +   * @return \Drupal\Core\Image\ImageInterface
   +   *   The image object.
   +   */
   +  protected function getImage(): ImageInterface {
   

   <3 the return typehinting. Nice to see on new stuff.

Hi @alexpott
Made changes please review.

1. +++ b/core/modules/image/tests/src/Kernel/ImageEffectsTest.php
   @@ -0,0 +1,267 @@
   +  protected function assertImageEffect(array $expected_operations, string $effect_name, array $data): void {
   

   We need to change all the calls to this method to pass in the expected operations and we need to move the $this->assertToolkitOperationsCalled($expected_operations) inside this method.

   Also I suggested moving this to the trait as it looks helpful.

2. +++ b/core/tests/Drupal/FunctionalTests/Image/ToolkitTestBase.php
   @@ -2,12 +2,22 @@
   +@trigger_error('The ' . __NAMESPACE__ . '\ToolkitTestBase class is deprecated in Drupal 9.0.0 and will be removed before Drupal 10.0.0. There is no replacement provided as functional test base class because toolkit operations should be tested as kernel tests. \Drupal\KernelTests\Core\Image\ToolkitTestTrait trait has been added to provide a similar functionality for toolkit kernel tests. See https://www.drupal.org/node/3035573.', E_USER_DEPRECATION);
   

   E_USER_DEPRECATED was correct. The message should be:
   'The ' . __NAMESPACE__ . '\ToolkitTestBase class is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. . There is no replacement provided as functional test base class because toolkit operations should be tested as kernel tests. \Drupal\KernelTests\Core\Image\ToolkitTestTrait trait has been added to provide a similar functionality for toolkit kernel tests. See https://www.drupal.org/node/3035573.

3. +++ b/core/modules/image/tests/src/Kernel/ImageEffectsTest.php
   @@ -250,12 +250,14 @@
   +   *   Collection of effects.
   

   This is not that see the documentation for assertToolkitOperationsCalled()

4. +++ b/core/tests/Drupal/KernelTests/Core/Image/ToolkitTest.php
   @@ -17,7 +17,7 @@
   -   * {@inheitdoc}
   +   * {@inheritdoc}.
   

   No fullstop.

@Deepak Goyal also when making changes to a test it is good practice to run the test before uploading a patch - if you'd done that you'll see that #57 is incomplete... you'll get lots of

TypeError: Argument 1 passed to Drupal\Tests\image\Kernel\ImageEffectsTest::assertImageEffect() must be of the type array, string given, called in core/modules/image/tests/src/Kernel/ImageEffectsTest.php on line 189

Do we need a test for the deprecation of ToolkitTestBase?

Hi
Made changes as suggested in #55 and #58.
Please review.

#55 looks addressed - #57 was way out of track, so here attaching an interdiff #52 -> #61 and again a patch with -M20% to see changes between functional and kernel.

#61 is RTBC for me.

EDIT - re. #60 I looked back at the 8.9.x branch and I could not find explicit tests for entirely deprecated base test classes.

Committed bf94a9e and pushed to 9.1.x. Thanks!

@claudiu.cristea this is very unlikely to land in Drupal 8.9.x - it would have to be a completely different patch that didn't remove the old code and kept testing it.

I think you're probably better of patching 8.9.x for you own needs or doing some hocus-pocus.

Drupal 8 is very end-of-life, so restoring status against 9.1.x.

Publish CR