CommentFileSizeAuthor
#45 interdiff-42-45.txt1.67 KBvaplas
#45 2870448-45.patch42.77 KBvaplas
#42 interdiff-41-42.txt46.03 KBvaplas
#42 2870448-42.patch43.2 KBvaplas
#41 reroll_39-2870448-41.patch60.24 KBvaplas
#39 interdiff-33-39.txt1.38 KBvaplas
#39 2870448-39.patch52.56 KBvaplas
#39 interdiff-33-39-test-only.txt751 bytesvaplas
#39 2870448-39-test-only.patch52.24 KBvaplas
#33 interdiff-31-33.txt773 bytesvaplas
#33 2870448-33.patch52.09 KBvaplas
#31 2870448-31.patch51.64 KBvaplas
#31 interdiff-29-31.txt2.53 KBvaplas
#29 interdiff-27-29.txt1.16 KBvaplas
#29 2870448-29.patch52.75 KBvaplas
#27 interdiff-24-27.txt2.14 KBvaplas
#27 2870448-27.patch53.16 KBvaplas
#24 2870448-24.patch52.83 KBvaplas
#7 2870448-7.patch16.12 KBnaveenvalecha
#8 2870448-9.patch29.14 KBnaveenvalecha
#8 interdiff-2870448-7-9.txt15.93 KBnaveenvalecha
#9 2870448-11.patch28.39 KBnaveenvalecha
#9 interdiff-2870448-9-11.txt7.75 KBnaveenvalecha
#15 2870448-15.patch28.39 KBnaveenvalecha
#15 interdiff-2870448-9-15.txt7.75 KBnaveenvalecha
#18 2870448-18.patch45.08 KBvaplas
#18 interdiff-15-18.txt17.83 KBvaplas
#20 2870448-20.patch48.35 KBvaplas
#20 interdiff-18-20.txt6.46 KBvaplas
#22 2870448-22.patch48.38 KBvaplas
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

michielnugter created an issue. See original summary.

michielnugter’s picture

Status: Active » Postponed
michielnugter’s picture

dawehner’s picture

We could convert part of the tests (aka. the tests which don't depend on views) already.

michielnugter’s picture

It's true that we can do a partial convert but getting views in first would enable this issue to be converted in 1 go. To prevent a lot of spin-offs I postponed all issues that require base classes from other issues. A lot of them will be solved with getting views in though.

The way I see it the conversion can already be started but has to wait on the Views issue before it can be finished. Is that workable?

michielnugter’s picture

Issue summary: View changes
Status: Postponed » Active

Discussed on IRC and we decided to place all tests that have a non-converted base class out of scope. Updated the IS to reflect that.

naveenvalecha’s picture

Status: Active » Needs review
FileSize
16.12 KB

Here's the start. This patch should fail.
FileFieldTestBase class has functions(assertFileExists & assertFileNotExists) which PHPUnit_Framework_TestCase class also have static methods. We need to rename these methods from FileFieldTestBase class as we couldn't rely on the PHPUnit_Framework_TestCase assert methods directly. Because PHPUnit_Framework_TestCase assertFileExists expects $filename to be string. However in FileFieldTestBase its a file object.

//Naveen

naveenvalecha’s picture

Here's the new patch which renamed the FileFieldTestBase methods
assertFileExists to assertFileExistsOnDisk
assertFileNotExists to assertFileNotExistsOnDisk

Next item:
- FileManagedFileElementTest should be split into javascript and BTB test
- FileFieldWidgetTest should be split into Javascript and BTB test
Remove these tests from this patch.

//Naveen

naveenvalecha’s picture

Here we go with more fixes and removed the FileManagedFileElementTest & FileFieldWidgetTest from this converstion patch.

//Naveen

The last submitted patch, 7: 2870448-7.patch, failed testing. View results

naveenvalecha’s picture

Issue tags: +phpunit initiative

The last submitted patch, 8: 2870448-9.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 9: 2870448-11.patch, failed testing. View results

naveenvalecha’s picture

Component: phpunit » file.module
naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
28.39 KB
7.75 KB

Few more fixes.

Status: Needs review » Needs work

The last submitted patch, 15: 2870448-15.patch, failed testing. View results

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.

vaplas’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
45.08 KB
17.83 KB

Status: Needs review » Needs work

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

vaplas’s picture

Status: Needs work » Needs review
FileSize
48.35 KB
6.46 KB

Status: Needs review » Needs work

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

vaplas’s picture

Status: Needs work » Needs review
FileSize
48.38 KB
vaplas’s picture

Patch with #2917885: Add drupalPostFormWithInvalidOptions() to BrowserTestBase and #2919352: Move methods from FileFieldTestBase to Trait.


Revert #8, because we looks like we not need to do it here. FileFieldTestBase override assertFileExists, and can works with objects.
vaplas’s picture

Title: Convert web tests to browser tests for file module » [PP-2] Convert web tests to browser tests for file module

Added mark [PP-2], but not changed status, because maybe not everything is ready.

Status: Needs review » Needs work

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

vaplas’s picture

Status: Needs review » Needs work

The last submitted patch, 27: 2870448-27.patch, failed testing. View results

vaplas’s picture

Status: Needs work » Needs review
FileSize
52.75 KB
1.16 KB
dawehner’s picture

Nice progress!

  1. +++ b/core/modules/file/tests/src/Functional/DownloadTest.php
    @@ -70,17 +73,28 @@ protected function doPrivateFileTransferTest() {
    -    $this->assertEqual($contents, $this->content, 'Contents of the file are correct.');
    +    $this->assertSession()->responseContains($contents);
    

    Why do we switch from equals to contains?

  2. +++ b/core/modules/file/tests/src/Functional/DownloadTest.php
    @@ -172,4 +186,20 @@ private function checkUrl($scheme, $directory, $filename, $expected_url) {
    +  protected function settingsSet($name, $value) {
    

    This new method is used for \Drupal\file\Tests\DownloadTest::testFileCreateUrl which could be actually a kernel test, couldn't it? What about filing a follow up?

  3. +++ b/core/modules/file/tests/src/Functional/FilePrivateTest.php
    @@ -15,6 +15,7 @@
     class FilePrivateTest extends FileFieldTestBase {
     
    +
    

    🙃 Let's skip this additional new line :)

  4. +++ b/core/modules/file/tests/src/Functional/SaveUploadFormTest.php
    @@ -13,6 +15,12 @@
     class SaveUploadFormTest extends FileManagedTestBase {
    ...
    +  use DrupalPostFormWithInvalidOptionsTrait;
    

    I'm curious ... why is this $edit an invalid value here?

        $edit = [
          'file_subdir' => $test_directory,
          'files[file_test_upload]' => drupal_realpath($this->image->getFileUri())
        ];

    doesn't look too invalid for me, sorry for the dump question. This was done in #18 without much of an explanation.

vaplas’s picture

Great points as always!

30.1: done.
30.2: follow up or fix it here - both variants ok for me.
30.3: done.
30.4: done, I really overdid it with a replacement)

Status: Needs review » Needs work

The last submitted patch, 31: 2870448-31.patch, failed testing. View results

vaplas’s picture

Status: Needs work » Needs review
FileSize
52.09 KB
773 bytes

opps.

dawehner’s picture

30.2: follow up or fix it here - both variants ok for me.

Do you mind creating a follow up?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you!

vaplas’s picture

Title: [PP-2] Convert web tests to browser tests for file module » [PP-1] Convert web tests to browser tests for file module
Status: Reviewed & tested by the community » Postponed

@dawehner, thank you so much too!

Current patch contains two additional chunks from next issues:

  1. #2919352: Move methods from FileFieldTestBase to Trait
  2. #2917885: Add drupalPostFormWithInvalidOptions() to BrowserTestBase

I'm going to close first chunk as duplicate, because it's just a transfer of methods from the Class to the Trait. It's much better to do it right here to get a good test coverage.

But the second chunk is a trait with a new method, which most likely will need to be added separately. Therefore, I will add an additional test coverage there. And while to postpone this issue like [PP-1]. Correctly?

vaplas’s picture

I took the Drupal\file\Tests\FileFieldDisplayTest::testNodeDisplay() to extend the test coverage of drupalPostFormWithInvalidOptions(), and came across an interesting thing.

// Uncheck the display checkboxes and go to the preview.
$edit[$field_name . '[0][display]'] = FALSE;
$edit[$field_name . '[1][display]'] = FALSE;
$this->drupalPostForm("node/$nid/edit", $edit, t('Preview'));
$this->clickLink(t('Back to content editing'));
$this->assertRaw($field_name . '[0][display]', 'First file appears as expected.');
$this->assertRaw($field_name . '[1][display]', 'Second file appears as expected.');

Why we cann't use BTB::drupalPostForm() here?
Because $field_name . '[1][display]' not found.

But why?
Because we add new file in 'Preview' request directly.

Okay. But then we go back and check both:

$this->clickLink(t('Back to content editing'));
$this->assertRaw($field_name . '[0][display]', 'First file appears as expected.');
$this->assertRaw($field_name . '[1][display]', 'Second file appears as expected.');

What do we want to get with these asserts?
By message, that both files 'appears as expected'.

But only the first file does exist. No second file.
$this->assertRaw($field_name . '[1][display]', 'Second file appears as expected.');
This assert is passed only because the upload form already has a hidden display field with that name. It's enough to check exist of $field_name. '[1] [description]' or directly the second file itself, for confirm it.

And what suggestions?
For check the work without saving form, we can to do 'Upload' request, and only then to do 'Preview' request. Like last part of testAddValuesForFieldWithExistValues() in #2917885-24: Add drupalPostFormWithInvalidOptions() to BrowserTestBase.

vaplas’s picture

Here's what I proposed to do in #38. Now the test works correctly with drupalPostForm() too.

The last submitted patch, 39: 2870448-39-test-only.patch, failed testing. View results

vaplas’s picture

Updated with latest version DrupalPostFormWithInvalidOptionsTrait from #2917885: Add drupalPostFormWithInvalidOptions() to BrowserTestBase.

The changes are not very readable, so here's a quick review:

  1. Do not move methods form FileFieldTestBase to Trait (we can do it in #2809503: [PP-2] Convert AJAX part of \Drupal\file\Tests\FileFieldWidgetTest to JavascriptTestBase or reopen #2919352: Move methods from FileFieldTestBase to Trait after fix current issue).
  2. Bit update phpdoc and variable name in uploadNodeFiles
    +   * @param bool $is_invalid
    +   *   FALSE (by default) using drupalPostForm()
    +   *   TRUE using drupalPostFormWithInvalidOptions().
    ...
    -   public function uploadNodeFiles(array $files, $field_name, $nid_or_type, $new_revision = TRUE, array $extras = [], $is_hack = FALSE) 
    +  public function uploadNodeFiles(array $files, $field_name, $nid_or_type, $new_revision = TRUE, array $extras = [], $is_invalid = FALSE) {
    

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.

dawehner’s picture

+++ b/core/modules/file/tests/src/Functional/DownloadTest.php
@@ -70,17 +73,28 @@ protected function doPrivateFileTransferTest() {
+    try {
+      $http_client->head($url);
+      $this->fail('Not correctly denied access to a file when file_test sets the header to -1.');
+    }
+    catch (RequestException $e) {
+      $this->assertSame(403, $e->getCode());
+    }
...
+    try {
+      $http_client->head($url);
+      $this->fail('Not correctly returned 404 response for a non-existent file.');
+    }
+    catch (RequestException $e) {
+      $this->assertSame(404, $e->getCode());
+    }

What about setting ['http_errors ' => FALSE] in guzzle? Wouldn't this be way more readable overall?

vaplas’s picture

#44 it is nice idea! Thank you, @dawehner!

Status: Needs review » Needs work

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

vaplas’s picture

After epic #2346893: Duplicate AJAX wrapper around a file field we have one js-test FileFieldValidateTest::testAJAXValidationMessage().

How to do better?

  1. Leave the FileFieldValidateTest only with this one test, and include it in the scope of #2809503: [PP-2] Convert AJAX part of \Drupal\file\Tests\FileFieldWidgetTest to JavascriptTestBase
  2. Leave the full FileFieldValidateTest, and move it to separate issue or #2809503: [PP-2] Convert AJAX part of \Drupal\file\Tests\FileFieldWidgetTest to JavascriptTestBase.
  3. Convert FileFieldValidateTest::testAJAXValidationMessage() to JTB test here.