Problem/Motivation

https://www.drupal.org/pift-ci-job/861269 shows a random failure.

Let's prevent this from happening!

Proposed resolution

Add a waitForText() method, and use it.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#12 2937073-field_block-12.patch2.54 KBsavkaviktor16@gmail.com
#7 2937073-field_block-7.patch2.57 KBtim.plunkett
#7 2937073-field_block-7-pass.patch3.18 KBtim.plunkett
#7 2937073-field_block-7-random-fail.patch623 bytestim.plunkett
#5 2937073-field_block-5-interdiff.txt2.38 KBtim.plunkett
#5 2937073-field_block-5.patch2.57 KBtim.plunkett
#2 2937073-field_block-2.patch2.26 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Active » Needs review
FileSize
2937073-field_block-2.patch2.26 KB
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented 
+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/FieldBlockTest.php
@@ -84,6 +84,7 @@ public function testFieldBlock() {
+    $assert_session->waitForText('The block configuration has been saved.');
     $assert_session->pageTextContains('The block configuration has been saved.');

You could do this in 1 line
$this->assertNotEmpty($assert_session->waitForText('The block configuration has been saved.'));
You wouldn't need to call pageTextContains() then.

Status: Needs review » Needs work

The last submitted patch, 2: 2937073-field_block-2.patch, failed testing. View results

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs work » Needs review
FileSize
2937073-field_block-5.patch2.57 KB
2937073-field_block-5-interdiff.txt2.38 KB

That CSS trick is nice, but not robust enough to rely on.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
CreditAttribution: tedbow at Acquia commented

This looks good. The new \Drupal\FunctionalJavascriptTests\JSWebAssert::waitForText() uses the same regex logic as existing \Behat\Mink\WebAssert::pageTextContains()

This should fix if the submit on the previous lines were slow down by testbot.

@tim.plunkett Could we get a patch that runs the updated test only x30(not sure if we can get more without timeout) and 1 that runs the current test only 30x? This could prove that 1 fails more often. I know they may likely both come back green but worth a shot.

See here for changes needed to run-tests.sh to run a single test mutiple times https://www.drupal.org/files/issues/2830485-142-100x.patch

Hmm that did 100x for some reason I thought it would time out. Could try that.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
FileSize
2937073-field_block-7-random-fail.patch623 bytes
2937073-field_block-7-pass.patch3.18 KB
2937073-field_block-7.patch2.57 KB

Reuploading the clean patch for commit, along with the hacked ones to try to prove the randomness is gone

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Component: layout.module » layout_builder.module
borisson_’s picture

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

There are no failures in #7, but the change does look good. Should we try this with more passes on the fragile test?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

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.

savkaviktor16@gmail.com’s picture

savkaviktor16@gmail.com CreditAttribution: savkaviktor16@gmail.com at Skilld commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2937073-field_block-12.patch2.54 KB

Re-rolled against 8.7.x

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented

Thanks for the reroll. As I wrote the patch, someone else should perform the final review for RTBC.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Status: Needs review » Reviewed & tested by the community

The code looks fine to me. I don't think there is any way to manually test this. Since tests are passing, marking RTBC.

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

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Priority: Normal » Major
Issue tags: +Random test failure

So according to the IS this caused a random test failure, but I haven't gotten an email about it failing since April, so looks like a low fail rate. Promoting to major for that reason (instead of critical).

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Fixing issue credit.

Normally for a random test failure I'd want to see an illustration that the random fail was fixed via bulk test runs (where the bulk test run of unpatched HEAD showed consistent fails by deliberately slowing the page down or such), but since the fail rate on this appears to be very low, it might be infeasible. Furthermore, according to @tim.plunkett:

I think it only failed randomly with PhantomJS
the switch to chromedriver really cut down on those sorts of random fails

  • xjm committed 06dbc81 on 8.7.x 
    Issue #2937073 by tim.plunkett, Saviktor, tedbow: Improve robustness of...

  • xjm committed 6c78d26 on 8.6.x 
    Issue #2937073 by tim.plunkett, Saviktor, tedbow: Improve robustness of...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Version: 8.7.x-dev » 8.6.x-dev
Status: Reviewed & tested by the community » Fixed

Committed to 8.7.x and cherry-picked to 8.6.x. Thanks!

Status: Fixed » Closed (fixed)

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