Convert AJAX part of \Drupal\file\Tests\FileFieldWidgetTest to JavascriptTestBase

Taking this of postponed so we can take a fresh look at this. We've done some file conversions now, so maybe we can get this to work too now.

New stab at this, trying to make this more minimal.

I only converted part of the testMultiValuedWidget test to WebDriverTestBase since that seems like the most complex use case, all the others seem a little too much with the both js and nojs options.

testMultiValuedWidget tries to upload multiple files which Mink doesn't support (see https://github.com/minkphp/Mink/issues/358) because Selenium doesn't support it (see https://github.com/seleniumhq/selenium-google-code-issue-archive/issues/...)

#2917885: Add drupalPostFormWithInvalidOptions() to BrowserTestBase was aimed at working around this, but that seems like massive overkill just for this and that would mean we need to maintain that too. Bah!

So this strips out the overloading of the file widget, so we lose coverage for that. There are no other tests for this (kernel/unit). So that is not good. If anybody has an idea how to tackle this, feel free to chime in!

Ha!, so Mink doesn't support multiple file uploads, but Chromedriver does, so here is a Nightwatch test for it. Works on my machine, lets see if the bot can handle it!

Haven't fixed the other fail yet, just about nightwatch test for now.

So the bot handles the Nightwatch test differently then local testing. I've borrowed from \Drupal\Tests\file\Functional\MultipleFileUploadTest how to do multiple file uploads. Doesn't quite work in our scenario since the raised error is a warning not a blocking error, it gets redirected to the node view page and the error is lost.

But we can verify that only 3 files are uploaded.

I would suggest using this version and try to get the nightwatch test running in a follow up. So removed that for now.

Ok, now with the Nightwatch test really removed.

Patch seems ready to me.

Needs a reroll

It conflicts with #2402533: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation.

Thanks @jibran.

Reroll looks good, back to RTBC.

So this strips out the overloading of the file widget, so we lose coverage for that. There are no other tests for this (kernel/unit). So that is not good. If anybody has an idea how to tackle this, feel free to chime in!

I think we need the nightwatch issue created and probably committed before we commit this one as we'd be losing test coverage.

I only converted part of the testMultiValuedWidget test to WebDriverTestBase since that seems like the most complex use case, all the others seem a little too much with the both js and nojs options.

This assumes the multi value widget and single value widget share the same codepaths both in JS and non-JS which would surprise me. Are we sure that losing this coverage is ok?

@alexpott yeah that all makes sense to look onto. Created the nightwatch issue #3028957: Replicate multi file upload part of \Drupal\file\Tests\FileFieldWidgetTest to Nightwatch

Added a javascript test for the single file widget.
Also cleaned up some stale comments.

the Nightwatch test can take a while to land I'm afraid, bot vs local.

I did a bunch of research over in https://www.drupal.org/project/drupal/issues/3028957, and I've come to the conclusion that we should probably be able to do this in a FunctionalJavascriptTest. Mink doesnt support uploading multiple files, and neither does nightwatch.js, so we'll have to do it ourselves.

We can add a new method to core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php that adds an additional behavior similar to "attachFile" https://github.com/minkphp/MinkSelenium2Driver/blob/a65a77efd76cce4fd1fd...

But one that accepts an xpath, and an array of filenames.
"attachMultipleFiles". It would do what uploadFile in the original driver does, except it would upload all the files to the remote chromedriver , (by mimicing what 'uploadFile' does here:

https://github.com/minkphp/MinkSelenium2Driver/blob/a65a77efd76cce4fd1fd..., and finally it would concatenate the filenames together with the \n's and set that to the value of the filefield button.

Then, we can use that similar to how we used attachFile directly in the quickedit test:
http://cgit.drupalcode.org/drupal/tree/core/modules/image/tests/src/Func...

Except, we'll have to step around Mink's implementation of attachFile (https://github.com/minkphp/Mink/blob/master/src/Element/NodeElement.php#...) and use our's directly on the driver.

so pseudo something like :

    $input = $this->assertSession()->elementExists('css', '#edit-field-test-image-0-upload');
    //loop from 1 to 4 for the 4 files
    $array_of_local_filepaths[] = $this->container->get('file_system')->realpath('test_image_0.png'); \\ whatever it is we do to provide files to tests
    $this->getSession()->getDriver()->attachMultipleFiles($xpath, $array_of_local_filepaths); \\ use our implementation of attachMultipleFiles
    // Submit the form

This is all a mess of pseudo code and ideas, but I dont see how else we're going to mimic uploading multiple files at the same time and also have our chrome instance be non local/not inside the SUT at all.

How about we convert everything but the multiple upload test and then do that in a follow-up. So we only have one thing to focus on in that issue?

Hi, not sure if this helps, but the test in #2644468: Multiple image upload breaks image dimensions is managing multiple files upload in the ImageWidget.

@alexpott: re #29: sure that makes sense, I had put it here because It was originally part of this test. And I think that work may already be done for us.

@mondrake re: #30 Awesome. Yes, that issue is doing pretty much exactly what I was imagining we'd need to do. I think the blocker to getting this last test in is actually #2947517: Selenium driver: API to get remote file paths, which appears to be pretty much the follow up we want, and it's already somewhat solved for us.

#31 I'll bump #2947517: Selenium driver: API to get remote file paths to critical then, since that's a blocker for this and an helper for #2644468: Multiple image upload breaks image dimensions which is a crtical on its own.

Tried the multiple file upload scenario with the method from #2947517: Selenium driver: API to get remote file paths, upload 4 files with cardinality 3 and got the expected error, so when that lands/works this should be trivial to test and there should be no need to split this.

Thanks @all for all the research and digging done for this!

Here is a patch that should be green when #2947517: Selenium driver: API to get remote file paths lands.

+++ b/core/modules/file/tests/src/Functional/FileFieldWidgetTest.php
@@ -0,0 +1,536 @@
+    $node_storage->resetCache([$nid]);
+    $node = $node_storage->load($nid);
...
+    $node_storage->resetCache([$nid]);
+    $node = $node_storage->load($nid);
...
+    $node_storage->resetCache([$nid]);
+    $node = $node_storage->load($nid);
...
+    $node_storage->resetCache([$nid]);
+    $node = $node_storage->load($nid);
...
+    $node_storage->resetCache([$nid]);
+    $node = $node_storage->load($nid);
...
+    $node_storage->resetCache([$nid]);
+    $node = $node_storage->load($nid);
...
+    $node_storage->resetCache([$nid]);
+    $node = $node_storage->load($nid);

These can be replaced with loadUnchanged.

Re-rolled #34.

Incorporated suggestion from #36.

Had accidentally removed FileFieldTestBase.php when re-rolling patch. Resolved that here.

#2947517: Selenium driver: API to get remote file paths in
retest triggered for #39

Looks like #39 pass the tests. +1 for rtbc.

Needs some PHPCS love.

Checking changed files...
PHPCS: core/modules/file/tests/src/Functional/FileFieldTestBase.php passed
PHPCS: core/modules/file/tests/src/Functional/FileFieldWidgetTest.php passed

FILE: ...ules/file/tests/src/FunctionalJavascript/FileFieldWidgetTest.php
----------------------------------------------------------------------
FOUND 25 ERRORS AFFECTING 25 LINES
----------------------------------------------------------------------
 153 | ERROR | [x] Line indented incorrectly; expected 2 spaces,
     |       |     found 0
 156 | ERROR | [x] Line indented incorrectly; expected 2 spaces,
     |       |     found 0
 157 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 2
 158 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 2
 159 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 2
 160 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 2
 162 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 2
 163 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 2
 165 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 2
 166 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 2
 167 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 2
 168 | ERROR | [x] Line indented incorrectly; expected 6 spaces,
     |       |     found 4
 169 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 2
 170 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 2
 171 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 2
 172 | ERROR | [x] Line indented incorrectly; expected 6 spaces,
     |       |     found 4
 173 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 2
 175 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 2
 176 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 2
 177 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 2
 178 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 2
 179 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 2
 180 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 2
 186 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 2
 187 | ERROR | [x] Line indented incorrectly; expected 2 spaces,
     |       |     found 0
----------------------------------------------------------------------
PHPCBF CAN FIX THE 25 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 50ms; Memory: 8Mb

Re #45

Addressed #46

1. +++ b/core/modules/file/tests/src/Functional/FileFieldTestBase.php
   @@ -20,10 +20,10 @@
   -  * Modules to enable.
   -  *
   -  * @var array
   -  */
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   
   +++ b/core/modules/file/tests/src/Functional/FileFieldWidgetTest.php
   @@ -0,0 +1,528 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   +  public static $modules = ['comment', 'block'];
   

   should use inheritdoc

2. +++ b/core/modules/file/tests/src/Functional/FileFieldWidgetTest.php
   @@ -0,0 +1,528 @@
   +  protected function setUp() {
   +    parent::setUp();
   +    $this->drupalPlaceBlock('system_breadcrumb_block');
   +  }
   

   should go after variables as we have it all over core

public static $modules should be protected not public - whilst we're changing them

Fixed it

Fix #49 and comments for test classes

So excited this is the last one! Just a drive by review here:

+++ b/core/modules/file/tests/src/Functional/FileFieldWidgetTest.php
@@ -0,0 +1,525 @@
+    $this->assertResponse(200, 'Confirmed that the generated URL is correct by downloading the shipped file.');
...
+    $this->assertResponse(200, 'Confirmed that the generated URL is correct by downloading the shipped file.');
...
+    $this->assertResponse(200, 'Confirmed that the generated URL is correct by downloading the shipped file.');
...
+    $this->assertResponse(403, 'Confirmed that access is denied for the file without the needed permission.');
...
+    $this->assertResponse(200, 'Confirmed that the generated URL is correct by downloading the shipped file.');

assertResponse() is deprecated, can we use $this->assertSession()->statusCodeEquals() instead?

+++ b/core/modules/file/tests/src/FunctionalJavascript/FileFieldWidgetTest.php
@@ -0,0 +1,205 @@
+    $this->adminUser = $this->drupalCreateUser(['access content', 'access administration pages', 'administer site configuration', 'administer users', 'administer permissions', 'administer content types', 'administer node fields', 'administer node display', 'administer nodes', 'bypass node access']);

Can we split these permissions into multiple lines?

Please keep in mind we are still trying to do the minimal changes for this to pass, even though this is the last one and we all feel bad for it so we want to give it some extra love ;)

lol, fair enough @Lendude :D

Still not sure about changes, this 2 files are new so should not use deprecated methods, but there's more - this methods has no second argument (message)
The same applies to assertRaw()

This issue is about conversion. Regarding #55 we have #3027952: [Plan] Remove the usage of deprecated methods in tests.

+++ b/core/modules/file/tests/src/Functional/FileFieldTestBase.php
@@ -44,6 +45,7 @@ protected function setUp() {
+   *   The new unsaved file entity.

+++ b/core/modules/file/tests/src/FunctionalJavascript/FileFieldWidgetTest.php
@@ -0,0 +1,205 @@
+   *   The File entity object.

The only thing that requires to be consistent.

+ 1 for rtbc

Thanks, patch is looking good now so RTBC.

I've done a line-by-line comparision of the old test with the new tests and only one thing appears to be missing (and there might be a reason for that but it is not documented here). The JS part of ::testWidgetValidation() has not been converted to a test in \Drupal\Tests\file\FunctionalJavascript\FileFieldWidgetTest

@alexpott spot on, yeah that got lost somewhere, added it.

Nice catch @alexpott. Interdiff looks good. As #59 is addressed so back to RTBC.

Thanks @Lendude - I checked the new test and it does match the old functionality.

 ___________________________________________________________________________________________________________________
< Committed http://cgit.drupalcode.org/drupal/commit/?id=a726a43 and pushed to 8.7.x. Thanks! >
 -------------------------------------------------------------------------------------------------------------------
        \   ^__^
         \  (oo)\_______
            (__)\       )\/\
                ||----w |
                ||     ||

yay... no... YAYAYAYAYAYAYAYAYAYAYAYYAYAYAYAYAYAYAYAYAY!

WOOOOOOOO!