Problem/Motivation

Testing for PHP8 compatibility has revealed that we fail with the dev-master version of behat/mink-selenium2-driver - see https://github.com/minkphp/MinkSelenium2Driver/issues/324

Steps to reproduce

composer require behat/mink-selenium2-driver:dev-master
./vendor/bin/phpunit -v core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderNestedFormUiTest.php --filter testAddingFormBlocksToDefaults

Proposed resolution

There are two changes that impact us:

https://github.com/minkphp/MinkSelenium2Driver/pull/323 breaks the JS in \Drupal\FunctionalJavascriptTests\JSWebAssert::assertWaitOnAjaxRequest()
https://github.com/minkphp/MinkSelenium2Driver/pull/322 breaks LayoutBuilderNestedFormUiTest::testAddingFormBlocksToDefaults() because we don't probably wait for a button to be visible.

322 is handled by #3174928: Improve the stability of core JS testing and prepare for update of MinkSelenium2Driver as it also affects HEAD and improvements there are critical. This issue will handle 323.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

CommentFileSizeAuthor
#10 3176361-10.patch7.16 KBnaresh_bavaskar
#5 3176361-reroll-5.patch7.16 KBabhijith s
#2 3176361-2.patch7.16 KBalexpott

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Active » Needs review
Issue tags: +PHP 8.0
StatusFileSize
new 3176361-2.patch7.16 KB
ranjith_kumar_k_u’s picture

ranjith_kumar_k_u commented

I have reviewed the above patch,the patch applied cleanly and it removes semi colon from javascript test conditions of following files
core/modules/big_pipe/tests/src/FunctionalJavascript/BigPipeRegressionTest.php
core/modules/ckeditor/tests/src/FunctionalJavascript/AjaxCssTest.php
core/modules/ckeditor/tests/src/Traits/CKEditorTestTrait.php
core/modules/image/tests/src/FunctionalJavascript/QuickEditImageEditorTestTrait.php
core/modules/media/tests/src/FunctionalJavascript/MediaUiJavascriptTest.php
core/modules/quickedit/tests/src/FunctionalJavascript/QuickEditJavascriptTestBase.php
core/tests/Drupal/FunctionalJavascriptTests/BrowserWithJavascriptTest.php
core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
RTBC

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

abhijith s’s picture

abhijith s commented
StatusFileSize
new 3176361-reroll-5.patch7.16 KB

Rerolled patch from #2 .It works on 9.2.x .please check

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@Abhijith S the patch in #2 still applies to 9.1.x. There is no need to reroll a patch that still applies to a branch.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Simple cleanup, makes sense to me.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a re-roll against 9.3.x

naresh_bavaskar’s picture

naresh_bavaskar
Location Pune, Maharashtra
commented
Status: Needs work » Needs review
StatusFileSize
new 3176361-10.patch7.16 KB

Re-rolled the patch for 9.3.x

spokje’s picture

spokje
Primary language Dutch
commented
Issue tags: -Needs reroll

Reroll done, removed Needs reroll tag.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Visually compared the two patches, the reroll is identical except for fuzz on a few unrelated lines.

  • catch committed 58fb7f8 on 9.3.x 
    Issue #3176361 by alexpott, naresh_bavaskar, longwave: Remove semi-colon...

  • catch committed e49605e on 9.2.x 
    Issue #3176361 by alexpott, naresh_bavaskar, longwave: Remove semi-colon...
catch’s picture

catch
he/him
Primary language English
commented
Version: 9.3.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 9.3.x and cherry-picked to 9.2.x, thanks!

Status: Fixed » Closed (fixed)

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