Problem/Motivation

@dawehner spotted this whilst we were waiting for a test run to finish.

19:20:10 Drupal\Tests\settings_tray\FunctionalJavascript\QuickEditInt   2 passes                                      
19:25:22 Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTray  22 passes

That's way longer than any other JS test.

Proposed resolution

Don;t use a data provider. They are really for a unit tests. Using them in functional tests means we have to do a full re-install each time.

Remaining tasks

User interface changes

none

API changes

none

Data model changes

none

CommentFileSizeAuthor
#18 2971472-10.patch2.49 KBAnonymous (not verified)
#17 2971472-14_SLOW_DRUPAL.patch3.36 KBAnonymous (not verified)
#17 no_changes_SLOW_DRUPAL.patch889 bytesAnonymous (not verified)
#14 interdiff-2971472-10-14.txt877 bytestedbow
#14 2971472-14_SLOW_DRUPAL.patch3.35 KBtedbow
#14 no_changes_SLOW_DRUPAL.patch876 bytestedbow
#10 2971472-10.patch2.49 KBalexpott
#10 9-10-interdiff.txt1.34 KBalexpott
#9 2971472-9.patch2.11 KBalexpott
#9 3-9-interdiff.txt1.02 KBalexpott
#3 2971472-3.patch2.11 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Title: QuickEditIntegrationTest is taking 5 minutes to run » SettingsTrayBlockFormTest is taking 5 minutes to run

Wrong test name - doh.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Active » Needs review
FileSize
2971472-3.patch2.11 KB

Let's see how much time this saves. Note that JS tests are not run concurrently.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented 
22:17:07 Drupal\Tests\settings_tray\FunctionalJavascript\QuickEditInt   2 passes                                      
22:19:55 Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTray   3 passes

So it's still long but it is 2 mins quicker.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Thank you alex!

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Reviewed & tested by the community » Needs review

Really, very elegant! Better than my attempt at #2942900-61: Convert JavascriptTestBase Tests to use DrupalSelenium2Driver. But it would be nice to add to doTestBlocks():

// Some asserts can be based on this value, so it should not be the same
// for different blocks, because it can be saved in the site config.
$new_page_text .= $this->randomMachineName(10);

To eliminate the random fail described in the #2902191-11: Determine cause and fix random fail in \Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testBlocks.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented 
+++ b/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
@@ -131,12 +138,18 @@ public function testBlocks($theme, $block_plugin, $new_page_text, $element_selec
-  public function providerTestBlocks() {
+  public function getBlockTests() {

This can be protected now, instead of a public method.

I don't really understand what @vaplas is saying in #7, but hopefully @alexpott or @dawehner do understand that :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
3-9-interdiff.txt1.02 KB
2971472-9.patch2.11 KB

Thanks for the reviews @vaplas and @borisson_

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
9-10-interdiff.txt1.34 KB
2971472-10.patch2.49 KB

Whoops ignore the patches from #9 - it's the same as #3. Correct patch and interdiff.

googletorp’s picture

googletorp CreditAttribution: googletorp as a volunteer and at Reveal IT commented
Status: Needs review » Reviewed & tested by the community

I think this is good to go.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

#8: 😀

RTBC++

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 2971472-10.patch, failed testing. View results

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented
Status: Needs work » Needs review
FileSize
no_changes_SLOW_DRUPAL.patch876 bytes
2971472-14_SLOW_DRUPAL.patch3.35 KB
interdiff-2971472-10-14.txt877 bytes

I think the last test failure was 

+++ b/core/modules/settings_tray/tests/src/FunctionalJavascript/SettingsTrayBlockFormTest.php
@@ -50,11 +50,15 @@ public function testBlocks() {
+    if ($new_page_text) {
+      // Some asserts can be based on this value, so it should not be the same
+      // for different blocks, because it can be saved in the site config.
+      $new_page_text = $new_page_text . ' ' . $theme . ' ' . $block_plugin;
+    }

I just checking on this issue to make sure it wasn't going to cause a random failure like in #2902191: Determine cause and fix random fail in \Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testBlocks.

This is great solution!

This patch looks good.

Going to upload "slow drupal" patch we were able to help debug some of the random failures by running the test multiple times with usleep() call in index.php.

Just figure we should try just in case in the past we only found the random failures when DrupalCi was running slow.

So attached 2 "SLOW_DRUPAL" patches once with the test changes and once without. If they both pass then I think we can RTBC with #10 again.

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

Status: Needs review » Needs work

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

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs work » Needs review
FileSize
no_changes_SLOW_DRUPAL.patch889 bytes
2971472-14_SLOW_DRUPAL.patch3.36 KB

In the #13 fail, all tests were successful, but information about them was lost. This problem has recently appeared more often.

+++ b/core/scripts/run-tests.sh
@@ -145,6 +145,11 @@
+if (in_array('Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest', $test_list)) {
+  $test_list = array_fill(0, 10, 'Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest');
+}

outside_in 😉 Reroll.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Reviewed & tested by the community
FileSize
2971472-10.patch2.49 KB

All good. Reupload #10.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed 1ef022e and pushed to 8.6.x. Thanks!

  • catch committed 1ef022e on 8.6.x 
    Issue #2971472 by alexpott, vaplas, tedbow, borisson_:...

Status: Fixed » Closed (fixed)

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