Current \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest only test against the Bartik theme. It should test against Stark also like OffCanvasTest
Steps

  1. Create OutsideInJavascriptTestBase::getThemesToTest which will return list of theme names, bartik and stark for now.
  2. Update OffCanvasTest to call this instead of having its own list
  3. Remove $this->enableTheme('bartik'); form \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::setUp
  4. Update OutsideInBlockFormTest test functions to use OutsideInJavascriptTestBase::getThemesToTest
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tedbow created an issue. See original summary.

xjm’s picture

Issue tags: +Outside-in
tedbow’s picture

Component: javascript » outside_in.module
Issue tags: -Outside-in +JavaScriptTest

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.

tedbow’s picture

Status: Active » Needs review
FileSize
16.8 KB

Ok. Updated both test class and all test method to use new \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInJavascriptTestBase::getTestThemes()

Now testing bartik and stark everywhere.

Had to change a couple other things

  1. css selectors for blocks. Changes per theme. Bartik selector also changes because bartik is not installed during setup.
  2. selectors for node and fields because stark doesn't use the classes. Using QuickEdit attributes because they are available in both themes and only needed in QuickEdit Links test

Status: Needs review » Needs work

The last submitted patch, 5: 2784881-5-test_themes.patch, failed testing.

tedbow’s picture

Had to change some selectors to work with stark.
Also using a random string for the block id to not conflict when the theme changes.

tedbow’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 7: 2784881-7.patch, failed testing.

tedbow’s picture

Title: Update OutsideInBlockFormTest to test against Bartik and Stark theme » Update OutsideInBlockFormTest to test against Bartik, Stark, classy, stable themes
Status: Needs work » Needs review
FileSize
20.04 KB

Ok re-rolled

Also added stable and classy to themes that are tested.

Status: Needs review » Needs work

The last submitted patch, 10: 2784881-10.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
907 bytes
20.13 KB

Deleting the block added for each theme at the end of the loop fixes the problem locally.

tedbow’s picture

Title: Update OutsideInBlockFormTest to test against Bartik, Stark, classy, stable themes » Update Javascript based tests to test against all core themes
FileSize
565 bytes
20.34 KB

I forgot that Seven theme removes contextual links. @see seven_preprocess_block()

So Settings Tray Edit Mode won't work in Seven. But the Off-canvas dialog should still work. In fact Webform is already using the dialog on the admin side.

So added Seven theme to OffCanvasTest.

Status: Needs review » Needs work

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

tedbow’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Needs work » Needs review

Status: Needs review » Needs work

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

tedbow’s picture

Status: Needs work » Needs review

Was DrupalCI error. Tests passing now

bnjmnm’s picture

I'm going to re-roll the patch due to changes made by https://www.drupal.org/node/2862625 and provide a review. Will have in the next few days.

bnjmnm’s picture

Assigned: Unassigned » bnjmnm

Actually looks like re-roll needs to take into account multiple issue commits #2862625 #2786193, #2870146, #2786193 .
I'll have it ready shortly.

bnjmnm’s picture

Uploading a pretty elaborate reroll due to several commits worth of activity since the last patch.

To help with the review process: Based on gitk, these are the issues that altered the FunctionalJavascript test files since the last patch and this reroll.
https://www.drupal.org/node/2862625
https://www.drupal.org/node/2786193
https://www.drupal.org/node/2870146
https://www.drupal.org/node/2784567

bnjmnm’s picture

Assigned: bnjmnm » Unassigned
tedbow’s picture

Status: Needs review » Needs work

@bnjmnm thank for the re-roll!

Uploading a pretty elaborate reroll due to several commits worth of activity since the last patch.

Yep, it can get tricky with some many other issue affecting the same file.
Just a few things with the re-roll.

  1. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
    @@ -67,81 +61,83 @@ protected function setUp() {
    +      /*
    +       * 'element_selector' => 'span a',
    +       * 'label_selector' => 'h2',
    +       */
    

    I think this comment is new. I am not sure what it is here for.

    Is it documenting $element_selector, $label_selector? This would be different every time the test method is invoked.

  2. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
    @@ -249,85 +245,100 @@ public function testQuickEditLinks() {
    +        // Offcanvas should be closed when opening QuickEdit toolbar.
    

    Change "Offcanvas" to "Off-canvas dialog"

  3. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
    @@ -249,85 +245,100 @@ public function testQuickEditLinks() {
    +        // QuickEdit toolbar should be closed when opening Offcanvas.
    ...
    +      // Open QuickEdit toolbar via contextual link while in Edit mode.
    

    This is line if reverting the "Offcanvas" to "off-canvas" change in the other issue.

    Change "Offcanvas" to "off-canvas dialog"

  4. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
    @@ -67,81 +61,83 @@ protected function setUp() {
    -    $link = $page->find('css', "$block_selector .contextual-links li a");
    -    $this->assertEquals('Quick edit', $link->getText(), "'Quick edit' is the first contextual link for the block.");
    -    $this->assertContains("/admin/structure/block/manage/$block_id/off-canvas?destination=user/2", $link->getAttribute('href'));
    

    I think these lines were removed but not added back in.

    This test is necessary for #2784567: List "Quick edit" before "Configure" in contextual links while in Edit mode

  5. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
    @@ -67,81 +61,83 @@ protected function setUp() {
    -    $this->getSession()->executeScript('jQuery("body").trigger(jQuery.Event("keyup", { keyCode: 27 }));');
    ...
    +      $this->getSession()
    +        ->executeScript('jQuery("body").trigger(jQuery.Event("keyup", { keyCode: 27 }));');
    

    This change is not needed for the re-roll or this issue.

Just a note: I find that finding this non-related issue is easier with a complex re-roll like this when you view the git diff both in regular mode and ignore whitespace mode. git diff -w from command line, most GUI's also support this. Very help in cases like this where many lines are the same expect for the fact that they are now indented because they are inside a loop.

Also not sure if you know this, or maybe others who are reading this, but when you link to other issues you can use this format
"\[#2784567\]"
(without "\"s I couldn't figure who to show this with out filter changing it.
instead for the whole link then you will get the title and color by status
See
https://www.drupal.org/node/2784567
vs
#2784567: List "Quick edit" before "Configure" in contextual links while in Edit mode
Info on this is down on the bottom here: https://www.drupal.org/filter/tips

bnjmnm’s picture

FileSize
20.86 KB

Uploaded the revised reroll . The comment formatting info is much appreciated.

tedbow’s picture

Status: Needs work » Needs review

@bnjmnm thanks for the updated patch!

Just setting to "Needs Review" so the tests run

tedbow’s picture

@bnjmnm great. Think the reroll is done.

Just doing some small code cleanup things now.

  1. Add to shorten 1 comment
  2. Added comment \Drupal\Tests\outside_in\FunctionalJavascript\OffCanvasTest::getTestThemes() as to why it was adding 'seven' theme.
  3. Added comment to \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::getBlockSelector()
  4. Removed empty lines
rajeevk’s picture

Status: Needs review » Reviewed & tested by the community

Checked interdiff & then reviewed all code changes after applying patch in #25. All looks good. Marking it RTBC.

webchick’s picture

Title: Update Javascript based tests to test against all core themes » Update Outside-In Javascript based tests to test against all core themes
Status: Reviewed & tested by the community » Fixed

Very nice clean-up!

Committed and pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

  • webchick committed 21acfa6 on 8.4.x
    Issue #2784881 by tedbow, bnjmnm, RajeevK: Update Outside-In Javascript...

  • webchick committed 972041a on 8.3.x
    Issue #2784881 by tedbow, bnjmnm, RajeevK: Update Outside-In Javascript...
tedbow’s picture

@webchick thanks for committing!

Also @bnjmnm thanks for the work on this issue!

droplet’s picture

It will be useful if there's a simple way to add my custom theme to the test list also.

tedbow’s picture

@droplet, all someone would have to do is extend OutsideInBlockFormTest and override the getTestThemes().

Though I do think in general this type of functionality would be useful. It would probably be useful for any functional or javascript functional tests.

They all could be affected by markup or javascript changes implemented by a theme or really a module for that matter.

I am not sure if DrupalCI supports contrib themes having JavscriptFunctional tests but if it did they could add their own test that extends OutsideInBlockFormTest.

Status: Fixed » Closed (fixed)

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

tedbow’s picture

Component: outside_in.module » settings_tray.module

Changing to new settings_tray.module component. @drpal thanks for script help! :)