See #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase).

Out of Scope:
- ImageAdminStylesTest should be split into JTB and BTB

CommentFileSizeAuthor
#61 interdiff-51-61.txt1.78 KBvaplas
#61 stream.patch27.92 KBvaplas
#60 2863626-60.patch28.4 KBLendude
#60 interdiff-2863626-59-60.txt1.89 KBLendude
#59 interdiff-51-59.txt3.7 KBvaplas
#59 2863626-59.patch26.15 KBvaplas
#57 interdiff-55-57-errors-true.txt999 bytesvaplas
#57 2863626-57-errors-false.patch28.35 KBvaplas
#57 2863626-57-errors-true.patch28.35 KBvaplas
#55 interdiff-53-55.txt1.88 KBvaplas
#55 2863626-55.patch28.17 KBvaplas
#53 interdiff-51-53.txt594 bytesvaplas
#53 2863626-53.patch27.46 KBvaplas
#51 2863626-51.patch27.41 KBLendude
#46 interdiff-2863626-42-44.txt7.63 KBwengerk
#44 interdiff-2863626-42-44.txt8.53 KBwengerk
#44 2863626-44.patch30.63 KBwengerk
#42 interdiff-2863626-41-42.txt5.69 KBwengerk
#42 2863626-42.patch23.4 KBwengerk
#41 interdiff-2863626-37-41.txt6.25 KBwengerk
#41 2863626-41.patch19.92 KBwengerk
#39 2863626-37.patch18 KBwengerk
#37 2863626-37.patch104.39 KBwengerk
#34 2863626-34.patch17.01 KBwengerk
#28 2863626-28.patch104.38 KBwengerk
#20 interdiff-2863626-17-20.txt2.62 KBnaveenvalecha
#20 2863626-20.patch16.03 KBnaveenvalecha
#17 2863626-17.patch17.02 KBnaveenvalecha
#13 interdiff-11-to-13.txt8.39 KBclaudiu.cristea
#13 2863626-13.patch26.32 KBclaudiu.cristea
#11 convert_web_tests_to-2863626-11.patch20.2 KBJo Fitzgerald
#11 interdiff-8-11.txt1.62 KBJo Fitzgerald
#8 convert_web_tests_to-2863626-8.patch18.58 KBnlisgo
#8 interdiff-2863626-4-8.txt485 bytesnlisgo
#4 convert_web_tests_to-2863626-4.patch18.6 KBscuba_fly
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

scuba_fly created an issue. See original summary.

scuba_fly’s picture

Issue tags: +drupaldevdays
Jo Fitzgerald’s picture

Title: Convert web tests to browser tests for imaget module » Convert web tests to browser tests for image module
scuba_fly’s picture

This patch is not finnished yet.

I'm running into some issues, which I will list once the patch is tested.

scuba_fly’s picture

Status: Active » Needs review

Starting the test.

Status: Needs review » Needs work

The last submitted patch, 4: convert_web_tests_to-2863626-4.patch, failed testing.

nlisgo’s picture

+++ 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;
nlisgo’s picture

nlisgo’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: convert_web_tests_to-2863626-8.patch, failed testing.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
1.62 KB
20.2 KB

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

Status: Needs review » Needs work

The last submitted patch, 11: convert_web_tests_to-2863626-11.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
Issue tags: -drupaldevdays
FileSize
26.32 KB
8.39 KB

At least fix the ::drupalBuildEntityView() problem.

Status: Needs review » Needs work

The last submitted patch, 13: 2863626-13.patch, failed testing.

michielnugter’s picture

Component: phpunit » image.module
Issue tags: +phpunit initiative
naveenvalecha’s picture

Issue tags: +Needs reroll
naveenvalecha’s picture

naveenvalecha’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 17: 2863626-17.patch, failed testing. View results

naveenvalecha’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
16.03 KB
2.62 KB

- ImageAdminStylesTest should be split into JTB and BTB

//Naveen

Status: Needs review » Needs work

The last submitted patch, 20: 2863626-20.patch, failed testing. View results

scuba_fly’s picture

Assigned: scuba_fly » Unassigned

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jonathan1055’s picture

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

wengerk’s picture

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

dawehner’s picture

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

wengerk’s picture

Assigned: Unassigned » wengerk

assign to myself as suggested :D.

wengerk’s picture

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.

wengerk’s picture

Assigned: wengerk » Unassigned
wengerk’s picture

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

wengerk’s picture

Status: Needs work » Needs review
dawehner’s picture

@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.

jonathan1055’s picture

@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

wengerk’s picture

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

Status: Needs review » Needs work

The last submitted patch, 34: 2863626-34.patch, failed testing. View results

wengerk’s picture

Assigned: Unassigned » wengerk
wengerk’s picture

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.

wengerk’s picture

wengerk’s picture

jonathan1055’s picture

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

wengerk’s picture

@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.

wengerk’s picture

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

wengerk’s picture

Assigned: wengerk » Unassigned
wengerk’s picture

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

jonathan1055’s picture

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

wengerk’s picture

FileSize
7.63 KB

Hi @jonathan1055,

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

wengerk’s picture

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);
dawehner’s picture

@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?

wengerk’s picture

@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);
wizonesolutions’s picture

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

Lendude’s picture

Status: Needs work » Needs review
FileSize
27.41 KB

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.

Status: Needs review » Needs work

The last submitted patch, 51: 2863626-51.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

vaplas’s picture

Status: Needs work » Needs review
FileSize
27.46 KB
594 bytes

Maybe this?

Status: Needs review » Needs work

The last submitted patch, 53: 2863626-53.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

vaplas’s picture

Status: Needs work » Needs review
FileSize
28.17 KB
1.88 KB

Status: Needs review » Needs work

The last submitted patch, 55: 2863626-55.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

vaplas’s picture

Status: Needs work » Needs review
FileSize
28.35 KB
28.35 KB
999 bytes

The last attempt, and I go to ignore issue :)

Status: Needs review » Needs work

The last submitted patch, 57: 2863626-57-errors-false.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

vaplas’s picture

Status: Needs work » Needs review
FileSize
26.15 KB
3.7 KB

I started working on this in #2651860-19: Ignore, testing issue. Mink performs these requests correctly. But while I could not understand the reasons for this.

Lendude’s picture

@vaplas++ great as always

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

vaplas’s picture

@Lendude, thank you very much!


And found! The reason is end of stream!
+++ b/core/modules/image/tests/src/Functional/QuickEditImageControllerTest.php
@@ -132,7 +132,7 @@ public function testValidImageUpload() {
-    $this->assertContains('"fid":"1"', $response->getBody()->getContents(), t('Valid upload completed successfully.'));
+    $this->assertContains('"fid":"1"', (string) $response->getBody(), t('Valid upload completed successfully.'));

Mr. Bot somewhere reads the stream, as result $response->getBody()->getContents() starts from end.
String casting or seek(0) read the stream from startfirst and this solves the problem! (I steal it from Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase(), so @Wim Leers++)

Patch with demostration. I not sure what varaints better Mink or pure Guzzle request here.

+++ b/core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php
@@ -410,10 +420,11 @@ public function testImageFieldDefaultImage() {
-    $edit = [
-      'settings[default_image][uuid][fids]' => 0,
-    ];
-    $this->drupalPostForm("admin/structure/types/manage/article/fields/node.article.$field_name/storage", $edit, t('Save field settings'));
+    // Can't use fillField cause Mink can't fill hidden fields.
+    $this->drupalGet("admin/structure/types/manage/article/fields/node.article.$field_name/storage");
+    $this->getSession()->getPage()->find('css', 'input[name="settings[default_image][uuid][fids]"]')->setValue(0);
+    $this->getSession()->getPage()->pressButton(t('Save field settings'));

Clever! We also have #2917885: Add drupalPostFormWithInvalidOptions() to BrowserTestBase, that can be useful for this.

And #2911915: Add getHttpClient() to BrowserTestBase, if we choose variant with guzzle client.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.