I am splitting off an issue - to prevent to parent issue from blowing up into a unreviewable mess.

The original plan was to make the minimal changes as part of the parent issue.

but I agree with this comment.

https://www.drupal.org/node/2862510#comment-12053691

So I think these assertions should be converted to both Unit tests and Javascript tests to cover it completely.

Summarising CommandsTest::testAjaxCommands()

There are 14 assertions all of a common form

AddCssCommand - needs only a straight conversion to units test as other functional test cover the more AJAX-ey functionality
Each command should be reviewed on a case by case basis.

CommentFileSizeAuthor
#19 2872603-19.patch17.79 KBLendude
#19 interdiff-2872603-14-19.txt637 bytesLendude
#14 2872603-14.patch17.94 KBLendude
#14 interdiff-2872603-10-14.txt3.16 KBLendude
#13 drupal-split-ajax-tests-2872603-13.patch36.61 KBm1r1k
#10 drupal-split-ajax-commands-test-2872603-10.patch18.24 KBm1r1k
#8 2872603-8.patch15.51 KBjofitz
#6 interdiff-4-6.txt8.83 KBAnonymous (not verified)
#6 2872603-6.patch15.52 KBAnonymous (not verified)
#4 interdiff-KTB-testAttachedSettings.txt9.8 KBAnonymous (not verified)
#4 interdiff-JTB-testAjaxCommands.txt12.9 KBAnonymous (not verified)
#4 2872603-4.patch18.77 KBAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

martin107 created an issue. See original summary.

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.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Active » Needs review
FileSize
2872603-4.patch18.77 KB
interdiff-JTB-testAjaxCommands.txt12.9 KB
interdiff-KTB-testAttachedSettings.txt9.8 KB

The patch breaks CommandsTest into:

  • testAjaxCommands -> JTB (all commands except 'alert', Selenium optionality)
  • testAttachedSettings -> KTB
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented

You added new base classes as well, is there a reason why those were added with just one usecase? Are they being used in the related issue? If they aren't, I don't think we should add the base classes.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
FileSize
2872603-6.patch15.52 KB
interdiff-4-6.txt8.83 KB

@borisson_, thanks for review!

I agree not to create a base classes. If they are needed in the future, we will create them in the future.

Also found easy way to test alert() command. So back it. Now a full replacement.

As @martin107 has already pointed out, we also have Unit test coverage via AjaxCommandsTest. So, this convertion seems quite caring.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Needs work
Issue tags: +Needs reroll
jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Assigned: martin107 » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2872603-8.patch15.51 KB

Re-rolled.

m1r1k’s picture

m1r1k CreditAttribution: m1r1k commented
Status: Needs review » Needs work

Should be updated because of this change record? [#2945059]

m1r1k’s picture

m1r1k CreditAttribution: m1r1k commented
Status: Needs work » Needs review
FileSize
drupal-split-ajax-commands-test-2872603-10.patch18.24 KB

Here is reroll that uses WebDriverTestBase instead of JavascriptTestBase

Status: Needs review » Needs work

The last submitted patch, 10: drupal-split-ajax-commands-test-2872603-10.patch, failed testing. View results

m1r1k’s picture

m1r1k CreditAttribution: m1r1k commented

Apparently CommandsTest should be rewritten because of [#2940704]

m1r1k’s picture

m1r1k CreditAttribution: m1r1k commented
Status: Needs work » Needs review
FileSize
drupal-split-ajax-tests-2872603-13.patch36.61 KB
Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Dx Experts commented
FileSize
interdiff-2872603-10-14.txt3.16 KB
2872603-14.patch17.94 KB

@m1r1k thanks for working on this. Not sure what happend in #13 but that seems to contain a lot of unrelated changes. I think the patch in #10 needed some work to account for #736066: ajax.js insert command sometimes wraps content in a div, potentially producing invalid HTML and other bugs landing (yay!!)

So took out some of the divs from the test and some other minor clean up.

Interdiff is versus #10

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.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Looks solid! Great work.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Status: Reviewed & tested by the community » Needs work
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented 
+++ b/core/tests/Drupal/KernelTests/Core/Ajax/CommandsTest.php
@@ -0,0 +1,111 @@
+    $this->installSchema('system', ['performance']);

This is not doing what we want it to do :)

The only reason this doesn't fail is because of some interesting code in ::installSchema

        // BC layer to avoid some contrib tests to fail.
        // @todo Remove the BC layer before 8.1.x release.
        // @see https://www.drupal.org/node/2670360
        // @see https://www.drupal.org/node/2670454
        if ($module == 'system') {
          continue;
        }

That should also fire a deprecation message so that we don't hit this issue again in core - can someone file a follow-up?

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at Dx Experts commented
Status: Needs work » Needs review
Related issues: +#2670454: Deprecate individual schema install of system module in KernelTestBase
FileSize
interdiff-2872603-14-19.txt637 bytes
2872603-19.patch17.79 KB

@alexpott thanks! Removed the installSchema call, it's indeed not needed at all.

I think the proper handling of that piece of BC code can be done in #2670454: Deprecate individual schema install of system module in KernelTestBase, so can we call that the follow up? (looking at the number of fails there it's something that is going wrong in a large number of tests)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented

@Lendude yep that can totally be the followup.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Setting to RTBC based on the approval of the @alexpott in #20.

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed ccece5260a to 8.7.x and 140b01f0e9 to 8.6.x. Thanks!

diff --git a/core/tests/Drupal/FunctionalJavascriptTests/Ajax/CommandsTest.php b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/CommandsTest.php
index d1b14211b8..0db5e5f74e 100644
--- a/core/tests/Drupal/FunctionalJavascriptTests/Ajax/CommandsTest.php
+++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/CommandsTest.php
@@ -2,7 +2,6 @@
 
 namespace Drupal\FunctionalJavascriptTests\Ajax;
 
-use Drupal\FunctionalJavascriptTests\DrupalSelenium2Driver;
 use Drupal\FunctionalJavascriptTests\WebDriverTestBase;
 
 /**

Fixed unused use on commit.

  • alexpott committed ccece52 on 8.7.x 
    Issue #2872603 by vaplas, Lendude, m1r1k, Jo Fitzgerald, borisson_,...

  • alexpott committed 140b01f on 8.6.x 
    Issue #2872603 by vaplas, Lendude, m1r1k, Jo Fitzgerald, borisson_,...

Status: Fixed » Closed (fixed)

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