Image: Convert system functional tests to phpunit

Apparently ToolkitTestBase.php is also referenced in a couple of other files too.

To get to the bottom of the initial problems I click to see more details of the test failure (https://www.drupal.org/pift-ci-job/627973), then clicked "View results on dispatcher" (https://dispatcher.drupalci.org/job/drupal_patches/7129/) and "Console Output" (https://dispatcher.drupalci.org/job/drupal_patches/7129/console). Scrolling to the bottom I found the error:

23:14:09 Fatal error: Class 'Drupal\system\Tests\Image\ToolkitTestBase' not found in /var/www/html/core/modules/image/tests/src/Functional/ImageEffectsTest.php on line 13

Hope that helps.

1. +++ b/core/modules/system/tests/src/Functional/Image/ToolkitTest.php
   @@ -1,6 +1,6 @@
   diff --git a/core/modules/system/src/Tests/Image/ToolkitTestBase.php b/core/modules/system/tests/src/Functional/Image/ToolkitTestBase.php
   
   diff --git a/core/modules/system/src/Tests/Image/ToolkitTestBase.php b/core/modules/system/tests/src/Functional/Image/ToolkitTestBase.php
   similarity index 81%
   
   similarity index 81%
   rename from core/modules/system/src/Tests/Image/ToolkitTestBase.php
   
   rename from core/modules/system/src/Tests/Image/ToolkitTestBase.php
   rename to core/modules/system/tests/src/Functional/Image/ToolkitTestBase.php
   
   rename to core/modules/system/tests/src/Functional/Image/ToolkitTestBase.php
   index 0051e94..2e92858 100644
   
   index 0051e94..2e92858 100644
   --- a/core/modules/system/src/Tests/Image/ToolkitTestBase.php
   
   --- a/core/modules/system/src/Tests/Image/ToolkitTestBase.php
   +++ b/core/modules/system/tests/src/Functional/Image/ToolkitTestBase.php
   

   Note: We need to keep the existing base class around.

2. +++ b/core/modules/system/tests/src/Functional/Image/ToolkitTestBase.php
   @@ -1,14 +1,19 @@
     */
   -abstract class ToolkitTestBase extends WebTestBase {
   +abstract class ToolkitTestBase extends JavascriptTestBase {
   

   Any reason you extend from JavascriptTestBase and not BrowserTestBase ?

+++ b/core/modules/system/tests/src/Functional/Image/ToolkitTestBase.php
@@ -99,10 +104,10 @@ public function assertToolkitOperationsCalled(array $expected) {
-      $this->assertTrue(FALSE, SafeMarkup::format('Expected operations %expected to be called but %uncalled was not called.', ['%expected' => implode(', ', $expected), '%uncalled' => implode(', ', $uncalled)]));
...
-      $this->assertTrue(TRUE, SafeMarkup::format('All the expected operations were called: %expected', ['%expected' => implode(', ', $expected)]));
+      $this->assertTrue(TRUE, new FormattableMarkup('All the expected operations were called: %expected', ['%expected' => implode(', ', $expected)]));

@@ -110,7 +115,7 @@ public function assertToolkitOperationsCalled(array $expected) {
-      $this->assertTrue(FALSE, SafeMarkup::format('Unexpected operations were called: %unexpected.', ['%unexpected' => implode(', ', $unexpected)]));
+      $this->assertTrue(FALSE, new FormattableMarkup('Unexpected operations were called: %unexpected.', ['%unexpected' => implode(', ', $unexpected)]));

Please don't change that as part of the conversion. This makes the patch harder to review for both the committer and the other reviewers.

made changes as per #15. Haven't touched deprecated calls as in #16.

+++ b/core/modules/image/tests/src/Functional/ImageEffectsTest.php
@@ -3,7 +3,7 @@
-use Drupal\system\Tests\Image\ToolkitTestBase;
+use Drupal\Tests\system\Functional\Image\ToolkitTestBase;
 

Note: Given that \Drupal\Core\ImageToolkit\ImageToolkitBase is in core, I think the tests should be moved to core/tests/Drupal/FunctionalTests

+++ b/core/tests/Drupal/FunctionalTests/Image/ToolkitTestBase.php
@@ -103,10 +104,10 @@ public function assertToolkitOperationsCalled(array $expected) {
     if (count($uncalled)) {
-      $this->assertTrue(FALSE, SafeMarkup::format('Expected operations %expected to be called but %uncalled was not called.', ['%expected' => implode(', ', $expected), '%uncalled' => implode(', ', $uncalled)]));
+      $this->assertTrue(FALSE, new FormattableMarkup('Expected operations %expected to be called but %uncalled was not called.', ['%expected' => implode(', ', $expected), '%uncalled' => implode(', ', $uncalled)]));
     }
     else {
-      $this->assertTrue(TRUE, SafeMarkup::format('All the expected operations were called: %expected', ['%expected' => implode(', ', $expected)]));
+      $this->assertTrue(TRUE, new FormattableMarkup('All the expected operations were called: %expected', ['%expected' => implode(', ', $expected)]));

@@ -114,7 +115,7 @@ public function assertToolkitOperationsCalled(array $expected) {
     $unexpected = array_diff($actual, $expected);
     if (count($unexpected) && (!in_array('apply', $expected) || count(array_intersect($unexpected, $operations)) !== count($unexpected))) {
-      $this->assertTrue(FALSE, SafeMarkup::format('Unexpected operations were called: %unexpected.', ['%unexpected' => implode(', ', $unexpected)]));
+      $this->assertTrue(FALSE, new FormattableMarkup('Unexpected operations were called: %unexpected.', ['%unexpected' => implode(', ', $unexpected)]));

These changes are out of scope, aka. they increase the patch size more than needed. Do you mind reverting those changes?

this is not the first time in this issue I replaced the deprecated method calls.... martin107--

Hey, no worries.

Nice minimal changes, I think this is ready to go.

+++ b/core/tests/Drupal/FunctionalTests/Image/ToolkitTest.php
similarity index 94%
rename from core/modules/system/src/Tests/Image/ToolkitTestBase.php

rename from core/modules/system/src/Tests/Image/ToolkitTestBase.php
rename to core/tests/Drupal/FunctionalTests/Image/ToolkitTestBase.php

We need to leave base classes in place and deprecate them as this might be extended by contrib.

1. +++ b/core/tests/Drupal/FunctionalTests/Image/ToolkitTestBase.php
   @@ -0,0 +1,159 @@
   +  public function assertToolkitOperationsCalled(array $expected) {
   ...
   +  public function imageTestReset() {
   ...
   +  public function imageTestGetAllCalls() {
   

   Is there a reason these methods are marked as public?

2. +++ b/core/tests/Drupal/FunctionalTests/Image/ToolkitTestBase.php
   @@ -0,0 +1,159 @@
   +    if (count($uncalled)) {
   +      $this->assertTrue(FALSE, SafeMarkup::format('Expected operations %expected to be called but %uncalled was not called.', ['%expected' => implode(', ', $expected), '%uncalled' => implode(', ', $uncalled)]));
   +    }
   +    else {
   +      $this->assertTrue(TRUE, SafeMarkup::format('All the expected operations were called: %expected', ['%expected' => implode(', ', $expected)]));
   +    }
   ...
   +    if (count($unexpected) && (!in_array('apply', $expected) || count(array_intersect($unexpected, $operations)) !== count($unexpected))) {
   +      $this->assertTrue(FALSE, SafeMarkup::format('Unexpected operations were called: %unexpected.', ['%unexpected' => implode(', ', $unexpected)]));
   +    }
   +    else {
   +      $this->assertTrue(TRUE, 'No unexpected operations were called.');
   +    }
   

   You want to use assertEmpty() here and just skip the second assertion

public methods on base class are "API" --- That is a really good idea.

If I understood the talk correctly -- at one point there seems to be widespread support of a yet unconfirmed policy of
going through the list of undeclared internal APIs in this document and marking them as such.

While I totally agree this is an important discussion to have, can we please leave them out of these conversions? Any of those changes requires every other person involved to think about it.

Do you want to open up an issue to discuss best practices for test base classes (like adding @internal when it should be just used for the module itself). There are test base classes, which should be used by other implementors/entity types etc.

Thank you :)

Thank you @martin107!

Committed/pushed to 8.4.x and cherry-picked to 8.3.x, thanks!

Fixed this on commit.

FILE: .../8.x/core/tests/Drupal/FunctionalTests/Image/ToolkitTestBase.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 13 | ERROR | [x] Additional blank lines found at end of doc comment
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Published the change record.