Convert web tests to browser tests for image module

+++ b/core/modules/image/tests/src/Functional/ImageDimensionsTest.php
@@ -3,14 +3,20 @@
 namespace Drupal\image\Tests;

The namespace needs to be changed to:

namespace Drupal\Tests\image\Functional;

Added methods to replicate WTB's method drupalGetJSON().

At least fix the ::drupalBuildEntityView() problem.

Here's the straight reroll of the #13. Removed EntityTestTrait as drupalbuildEntityView is already added as part of #2795051: Move \Drupal\simpletest\WebTestBase::drupalBuildEntityView into a trait and make it available in BTB

//Naveen

- ImageAdminStylesTest should be split into JTB and BTB

//Naveen

Many of the test failures are

Behat\Mink\Exception\ElementNotFoundException: Button with id|name|label|value "Save and publish" not found.

which is easy to fix, because the patch was created before #2068063: Change "Save and keep un-/published" buttons to a "Published" checkbox and an included "Save" button. Is anyone working on this? If not I will re-roll, as this issue is holding up #2864035: Convert web tests to browser tests for rdf module

I'm working on it with @gido during the DrupalConEu at Vienna.

@wengerk
In this case please assign the issue to yourself :) Thank you.

assign to myself as suggested :D.

I re-roll the patch in #20 to works on Drupal 8.5.x and the publish/save buttons as mentioned in #24.

But now, I'm facing an issue with the next test, when running locally, which can't found some field to remove the default image.

1) Drupal\Tests\image\Functional\ImageFieldDisplayTest::testImageFieldDefaultImage
Behat\Mink\Exception\ElementNotFoundException: Form field with id|name|label|value "settings[default_image][uuid][fids]" not found.

When debugging it seems the user don't get the correct form, Field Image storage, but only the Log In one :/ very strange.

It looks like the CI make the tests pass, it was maybe an error on my local env.

@wengerk
Do you mind configuring your local git to detect renames, see https://www.drupal.org/node/1666604
This can dramatically reduce the size of the patch file.

@wengerk - as an alternative you can use -M80% --find-copies in your git diff command instead of doing the config change. The % number after M allows the copied file to vary by this percentage and git will still recognise it as a copy. For an example, see the start of https://www.drupal.org/files/issues/2870439-42.patch

I just re-read the previous patch in #20 & I figured out I completely messed it up with mine in #28.

I re-rerolled the patch, using the [diff] renames = copies for renaming Thanks @dawehner & @jonathan1055..

The test on my local env. didn't works with an issue:

1) Drupal\Tests\image\Functional\ImageFieldDisplayTest::testImageFieldDefaultImage
Behat\Mink\Exception\ElementNotFoundException: Form field with id|name|label|value "settings[default_image][uuid][fids]" not found.

I unfortunately no time to work on this now, maybe someone could ?

Run using:
SIMPLETEST_DB="sqlite://localhost//tmp/test.sqlite" SIMPLETEST_BASE_URL='http://d8.dev' ./vendor/bin/phpunit -c core --group image --printer="\Drupal\Tests\Listeners\HtmlOutputPrinter" --stop-on-error

I fixed the tests, it crash but intended by design of Mink.

The tests try to fill a hidden field but Mink is user+browser emulator. It emulates everything, that real user can do in real browser. And user surely can't fill hidden fields on the page - he just don't see them.

It now fail on another tests, I still upload my "wip" patch and I'm now working on the new failling one.

1) Drupal\Tests\image\Functional\ImageFieldDisplayTest::testImageFieldSettings
Failed asserting that false is true.

Hi wengerk,
Disregarding patch #28 which you told us to ignore, I have compared the previous patch #20 with yours in #34. I found that the only changes were:

• ImageFieldDefaultImagesTest.php - necessary for re-roll
• ImageFieldTestBase.php - button text "Save" changed for re-roll
• BrowserTestBase.php - reroll

There were no actual coding changes, which is good. So, starting from #34 (which is essentially at the same state of review as #20) could you provide an interdiff text file of what you have changed, so that reviewers can see what we should be looking for, and how the changes impact the result. It is confusing for reviewers that your patch in #37 passes, then with no further comment your patch in #39 (which you also named 37) fails.

Thanks for working on this. These are just suggestions to help make our reviews easier, and speed the progress of the issue.

Jonathan

@jonathan1055 thanks for your suggestion, here my explaination for the 2 same patch name :/ sorry.

In #37 I have made a first patch which was a broken one (I mean whitout the new files moved so the tests pass because they wasn't in the patch :/) I try to remove it from the issue but it seems the issue keep file in the history.
When I try to remove it I also re-upload a new one with the same name, I though it would be easier to have only 1 files with the name 37, I didn't know it was impossible to remove a previous posted patch).

I finally do some works and the patch has more tests which pass but still not all of them.

I'm now stuck cause some very old tests use legacy function setRawContent which has been deprecated & we have to rework the whole tests of /core/modules/image/tests/src/Functional/ImageThemeFunctionTest.php.

I have no time to work on it now, maybe some will continue.

The patch #42 fix 2 new tests in the core/modules/image/tests/src/Functional/QuickEditImageControllerTest.php.

Here fixes for the setRawContent of /core/modules/image/tests/src/Functional/ImageThemeFunctionTest.php.

Hi wengerk,
Thank you for continuing to work on this.

Just an observation - your interdiffs are actually comparisions of the patch files. This is not actually what an interdiff is meant to be, it should show the change in source code line. The changes can be inferred from looking at yout interdiff, but it is much more complicated than it should be. Have a look at https://www.drupal.org/documentation/git/interdiff

If you want me to, I will make an interdiff from the above patches, to show you what they should look like. Let me know if that would help.

Jonathan

Hi @jonathan1055,

Thanks for you help, I re-create the interdiff.

I don't understand why both tests testInvalidUpload & testValidImageUpload didn't works on the Drupal CI.

They run whitout probleme on my dev environment.
I run them using the --filter of phpunit such as:

testInvalidUpload
SIMPLETEST_DB="sqlite://localhost//tmp/test.sqlite" SIMPLETEST_BASE_URL='http://d8.dev' ./vendor/bin/phpunit -c core --group image --printer="\Drupal\Tests\Listeners\HtmlOutputPrinter" --stop-on-error --filter testInvalidUpload

testValidImageUpload
SIMPLETEST_DB="sqlite://localhost//tmp/test.sqlite" SIMPLETEST_BASE_URL='http://d8.dev' ./vendor/bin/phpunit -c core --group image --printer="\Drupal\Tests\Listeners\HtmlOutputPrinter" --stop-on-error --filter testValidImageUpload

The dispatcher says:
testInvalidUpload

Drupal\Tests\image\Functional\QuickEditImageControllerTest::testInvalidUpload
Failed asserting that '' contains ""main_error":"The image failed validation."".

testValidImageUpload

Drupal\Tests\image\Functional\QuickEditImageControllerTest::testValidImageUpload
Failed asserting that '' contains ""fid":"1"".

It seems the $response->getBody()->getContents() in the /core/modules/image/tests/src/Functional/QuickEditImageControllerTest.php return empty, but how is that possible ? If someone has a clue it would be great !

edit:
Is that possible the following code would cause issue in the Drupal CI ?

    $domain = parse_url($this->getUrl(), PHP_URL_HOST);

Should I have to do something such as :

    $absolute_url = $this->getAbsoluteUrl($this->getUrl());
    $domain = parse_url($absolute_url, PHP_URL_HOST);

@wengerk
For these kind of issues I access Drupal also from a folder like http://localhost/d8 or so.
This way you have an environment which works more like the testbot.

Just some other random comments:

1. +++ b/core/modules/image/tests/src/Functional/ImageStylesPathAndUrlTest.php
   @@ -199,7 +206,9 @@ public function doImageStyleUrlAndPathTests($scheme, $clean_url = TRUE, $extra_s
   +        $result = strpos($raw, chr(137) . chr(80) . chr(78) . chr(71) . chr(13) . chr(10) . chr(26) . chr(10));
   +        $this->assertEqual($result === FALSE, TRUE);
   

   Note: You can use assertNotContains here

2. +++ b/core/modules/image/tests/src/Functional/ImageThemeFunctionTest.php
   @@ -213,9 +220,28 @@ public function testImageAltFunctionality() {
   -    $this->setRawContent($renderer->renderRoot($image_with_alt_attribute_both));
   -    $elements = $this->xpath('//img[contains(@class, class) and contains(@alt, :alt)]', [":class" => "image-with-attribute-alt", ":alt" => "Attribute alt"]);
   ...
   +    $html = $renderer->renderRoot($image_with_alt_attribute_both);
   +    $elements = $this->getXPathResultCount('//img[contains(@class, "image-with-attribute-alt") and contains(@alt, "Attribute alt")]', $html);
   +    $this->assertEqual($elements, 1, 'Attribute alt overrides alt property if both set.');
   ...
   +  protected function getXPathResultCount($query, $html) {
   +    $document = new \DOMDocument();
   +    $document->loadHTML($html);
   +    $xpath = new \DOMXPath($document);
   +
   +    return $xpath->query($query)->length;
      }
   

   Is there a reason we cannot use the previous pattern aka. using xpath and counting the result?

@dawehner

Thanks for you help !

It's not possible to use assertNotContains in the case N°1 cause I want to assert some binary string and assertNotContains needs an UTF-8 string. So it crash when asserting chr().

For the second case, It didn't work anymore with the old syntax $this->xpath() is not anymore available.
Plus, the markup change & now when alt is NULL, the attribute is not anymore empty but missing so I fix it with a not(alt).

> -    $elements = $this->xpath('//img[@class="image-style-image-test" and @src=:url]', [':url' => $url]);
> -    $this->assertEqual(count($elements), 1, 'theme_image_style() renders an image correctly with a NULL value for the alt option.');
> +
> +    $html = $renderer->renderRoot($element);
> +    $elements = $this->getXPathResultCount('//img[@class="image-style-image-test" and @src="'.$url.'" and not(@alt)]', $html);

I use this class in contrib, and I'm getting the errors as well. It seems like that is not what's holding up this patch. What is the current issue? @wengerk @dawehner

Thanks for the great work on this so far!

Needed a reroll, so no interdiff

ImageThemeFunctionTest had no business being a Functional test so converted that to a Kernel test, that takes care of #48.2

Ran QuickEditImageControllerTest locally on a $base_path different then '/', but still green. So haven't figured out what's going on there.

@vaplas++ great as always

Last thing I could see here was the Views tests not getting converted. Converted to a Kernel test here.

Rerolled patch #60.

Rerolled #60, split ImageFieldValidateTest in a new functional test and 2 methods (testAJAXValidationMessage and testFriendlyAjaxValidation) that remain in the W2B test that will have to be converted to FunctionalJavascript. Fixed 3 PHPCS errors.

If this passes, I'd suggest to split ImageAdminStylesTest also in a B2B test and a part remaining in WTB that can be addressed as a separate FunctionalJavascript migration in a follow-up.

Splitting ImageAdminStylesTest in a B2B test and a part remaining in WTB. Fixed a couple oh PHPCS nits.

This should fix the 2 xpath related failures.

+++ b/core/modules/image/src/Tests/ImageAdminStylesTest.php
@@ -16,280 +11,6 @@
-  public function createSampleImage(ImageStyleInterface $style) {
...
-  public function getImageCount(ImageStyleInterface $style) {

Maybe it is just me, but I'd have kept about these helper functions. They might be used by a subclass.

Feedback has been addressed, followups have been created, this looks good to me. I think we've done everything possible to make this conversion as clean as possible.

Those they are removed at the beginning, and added later:

Oh I see. Thank you for clarifying that.

\Drupal\Tests\BrowserTestBase::drupalGetJSON() and \Drupal\Tests\BrowserTestBase::drupalGetWithFormat()

I'm not sure about adding - the only usage of drupalGetWithFormat in either WTB or BTB is the respective drupalGetJSON function so at the very least I don't think we should be adding that.

drupalGetJson is used in \Drupal\Tests\image\Functional\QuickEditImageControllerTest::testFieldInfo() and \Drupal\taxonomy\Tests\TermAutocompleteTest::testAutocompleteCountResults(). I'm not sure these wrappers are really adding too much. Let's just do:

$json = $this->drupalGet('quickedit/image/info/node/' . $node->id() . '/' . $this->fieldName . '/' . $node->language()->getId() . '/default', ['query' => ['_format' => 'json]]);
$info = Json::decode($json);

And not add the methods.

This change might break some contrib tests.

@jibran which change and why?

I was just trying to point out that having \Drupal\Tests\BrowserTestBase::drupalGetJSON() and \Drupal\Tests\BrowserTestBase::drupalGetWithFormat() will make the converstion of exsiting WTB tests in contrib a lot easier. OTOH not adding these methods to BTB and just creating a change notice will also suffice IMO.

@jibran so nothing is broken.

@vaplas / @jibran do you know how widely used WTB::drupalGetJSON() and WTB::drupalGetWithFormat() are in contrib? Also I have no idea why a change notice would be needed if we're not adding any methods to BrowserTestBase. Sugar is costly and needs to be maintained.

I was just trying to point out that having \Drupal\Tests\BrowserTestBase::drupalGetJSON() and \Drupal\Tests\BrowserTestBase::drupalGetWithFormat() will make the converstion of exsiting WTB tests in contrib a lot easier. OTOH not adding these methods to BTB and just creating a change notice will also suffice IMO.

That is for sure a good point. I think though that it is not "a lot". Sometimes explicit code is better than implicit code :)

Thanks for digging around and finding numbers on the usage of those methods, looks like not converting them makes sense.

All feedback has been addressed and we have a follow up for the remaining two tests in /core/modules/image/src/Tests

Committed and pushed b89bd0c978 to 8.6.x and a3d45a6e1e to 8.5.x. Thanks!

Reroll patch for work with 8.5.4

Brain Fart! Just ignore me!