Problem/Motivation

drupalPostAjaxForm() is simulating the behaviour of ajax.js, so using it, doesn't really provide fundamental guarantees.
#2809161: Convert Javascript/AJAX testing to use JavascriptTestBase suggests to convert them to JavascriptTestBase

Proposed resolution

  1. Figure out which part of the test is testing PHP code and which part ajax behaviour
  2. Extract the ajax behaviour into a test that extends JavascriptTestBase

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

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

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

michielnugter’s picture

Component: phpunit » file system
Issue tags: +phpunit initiative

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.

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.

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

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

Lendude’s picture

Status: Active » Needs review
FileSize
11.76 KB

'Ere we go.

Biggest change is that the javascript version of the widget doesn't have an 'upload' button.

I went with assertWaitOnAjaxRequest() because that seemed to fit here, might want to change that to a waitForX.

Status: Needs review » Needs work

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

Lendude’s picture

Status: Needs work » Needs review
FileSize
1.41 KB
11.73 KB

That file logic works fine locally and in the BrowserTestBase version

Small tweak, lets see what the bot makes of that....

Status: Needs review » Needs work

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

Mixologic’s picture

I think the issue here is that you need to use attachFileToField to actually attach the file to the file upload in the form.

have a look at http://cgit.drupalcode.org/drupal/tree/core/modules/system/tests/src/Fun... for an example.

Lendude’s picture

Status: Needs work » Needs review
FileSize
1.58 KB
11.73 KB

@mixologic++

here we go

Status: Needs review » Needs work

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

Lendude’s picture

Status: Needs work » Needs review
FileSize
3.04 KB
11.1 KB

Looking at \Drupal\Tests\media\Functional\MediaRevisionTest it might be that we need to use 'temporary://' because that is shared between containers?

Works locally, lets see how the old bot feels about this one....

Status: Needs review » Needs work

The last submitted patch, 14: 2809505-14.patch, failed testing. View results

Lendude’s picture

Component: file system » file.module
Status: Needs work » Needs review
FileSize
933 bytes
11.1 KB

.... yeah ok, so fixed #11 in both spots now .... silly me

dawehner’s picture

  1. +++ b/core/modules/file/tests/src/FunctionalJavascript/FileManagedFileElementTest.php
    @@ -0,0 +1,98 @@
    +    $file_uri = 'temporary://foo.txt';
    +    file_put_contents($file_uri, $this->randomString(128));
    

    I would have expected to use tempnam() here

  2. +++ b/core/modules/file/tests/src/FunctionalJavascript/FileManagedFileElementTest.php
    @@ -0,0 +1,98 @@
    +          $this->assertSession()->assertWaitOnAjaxRequest();
    ...
    +          $this->assertSession()->assertWaitOnAjaxRequest();
    ...
    +          $this->assertSession()->assertWaitOnAjaxRequest();
    ...
    +          $this->assertSession()->assertWaitOnAjaxRequest();
    

    I wonder how hard it would be to actually wait for a specific element to appear instead

borisson_’s picture

Status: Needs review » Needs work

The change to tempnam() looks like it would be an easy change. The other one sounds like it would be a lot harder to figure out.

Moving this back to NW in any case.

borisson_’s picture

Lendude’s picture

#17.1 fixed
#17.2 wait for works when waiting for something to appear, but we still don't have anything to wait for something to disappear, so stuck with assertWaitOnAjaxRequest for the remove operations.

Lendude’s picture

Status: Needs work » Needs review
borisson_’s picture

Status: Needs review » Reviewed & tested by the community

Awesome, thank for fixing/explaining the review points. This looks great now!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/file/tests/src/FunctionalJavascript/FileManagedFileElementTest.php
    @@ -0,0 +1,100 @@
    +          $this->assertTrue($last_fid > $last_fid_prior, 'New file got uploaded.');
    

    $this->assertGreaterThan()?

  2. +++ b/core/modules/file/tests/src/FunctionalJavascript/FileManagedFileElementTest.php
    @@ -0,0 +1,100 @@
    +          $this->assertRaw(t('The file ids are %fids.', ['%fids' => '']), 'Submission after file removal was successful.');
    

    Is this assert deprecated?

  3. +++ b/core/modules/file/tests/src/FunctionalJavascript/FileManagedFileElementTest.php
    @@ -0,0 +1,100 @@
    +    return (int) db_query('SELECT MAX(fid) FROM {file_managed}')->fetchField();
    

    db_query is deprecated - let's not add a usage.

Lendude’s picture

Status: Needs work » Needs review
FileSize
2.23 KB
11.35 KB

@alexpott thanks for the review. Feedback addressed.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

#24 addressed #23 so back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed be64fa993e to 8.7.x and 0cac1a84d7 to 8.6.x. Thanks!

  • alexpott committed be64fa9 on 8.7.x
    Issue #2809505 by Lendude, dawehner, borisson_, Mixologic, alexpott:...

  • alexpott committed 0cac1a8 on 8.6.x
    Issue #2809505 by Lendude, dawehner, borisson_, Mixologic, alexpott:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.