Problem/Motivation

We're currently pinned to selenium/standalone-chrome:127, yet the latest version of Chrome is 134. #3471113: Update to selenium/standalone-chrome:134+ seems to require many tests to be updated, so let's update to 133 first, then tackle 134+ afterwards.

Steps to reproduce

Proposed resolution

  • Bump the standalone-chrome version to 133
  • Fix the few tests that need fixing

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3515403

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mstrelan created an issue. See original summary.

mstrelan opened merge request !11623

mstrelan’s picture

mstrelan commented
Status: Active » Needs work

The fails here are all due to resizing the browser. It seems in 128+ the window.innerHeight is always 139px less than the height you resize to. I tried to debug this in non-headless mode, which was even weirder because resizing to 400px wide would set it to 500px width.

It seems that the 139px includes the browser chrome (toolbar, infobar, etc), but I'm not sure if the difference is in how the resizeWindow works, or in how the browser reports window.innerHeight. Either way, using window.outerHeight makes BrowserWithJavascriptTest::testJavascript pass.

There was an option to use --headless=old which would mean we don't need to refactor any tests, but this has been removed in Chrome 132.

I was able to apply the same logic to LayoutBuilderDisableInteractionsTest::testFormsLinksDisabled and increase the height by 139px, but it seems like a bit of a hack.

Tried the same approach for the Nightwatch tests but it seems like there must be something else going on there.

mstrelan’s picture

mstrelan commented

Looks like the random fails we saw in 134 are creeping back in, and we've got a green run on 134 with all the additional waits added, so maybe we should just stick with that.

catch’s picture

catch
he/him
Primary language English
commented

Nightwatch testing has not really been maintained since it was added (apart from in desperation), but there is #3467492: [policy, no patch] Replace Nightwatch with Playwright.

quietone made their first commit to this issue’s fork.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

longwave made their first commit to this issue’s fork.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Let's try to revive this before going to an even newer version of Chrome.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

The last three fails pass locally now.

longwave opened merge request !14731

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

133 is green, trying 144 (latest) in a separate MR.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Sorry, try to get to criticals early but this one fell through. Appears to need a rebase

mstrelan changed the visibility of the branch 3515403-chrome-144 to hidden.

mstrelan’s picture

mstrelan commented

Let's keep this for 133 only and focus on newer versions in #3471113: Update to selenium/standalone-chrome:134+. There are a couple open issues for 134+ at https://issues.chromium.org/issues/405607581 and https://issues.chromium.org/issues/402796660.

I've hidden the 144 branch and rebased 133. It's failing on two SettingsTray tests, that I think are already flakey in main.

quietone’s picture

quietone commented

The failing test is Drupal\Tests\settings_tray\FunctionalJavascript\SettingsTrayBlockFormTest::testValidationMessages().
The output of the test includes:
Warning: Undefined array key "#type" in Drupal\olivero\Hook\OliveroHooks->preprocessInput() (line 226 of core/themes/olivero/src/Hook/OliveroHooks.php).
which is fixed by adding 'search_node' to the list of installed modules. But the test still fails.

quietone’s picture

quietone commented

According to git bisect, #3568566: Olivero's secondary menus inoperable when authenticated & BigPipe present is the issue that broke the test. That was back in Feb of this year.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

Good find on #3568566: Olivero's secondary menus inoperable when authenticated & BigPipe present. Turns out this is a bug in that change. secondLevelNavMenus is undefined until init() populates it - but if the primary menu is empty then it is never populated. In turn this is only uncovered here because Chrome 133 fires resize events that older Chrome didn't, which causes the error. So the fix is trivial - just initialise the variable to an empty array instead of undefined.

Claude Code helped with this debugging session.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Related issues: +#3584314: Mobile menu doesn't close by ESC key with search block + empty main menu

Looks like this is also the fix for #3584314: Mobile menu doesn't close by ESC key with search block + empty main menu

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Let do it! On a work project we ran into major problems being on an old chrome version so glad to see we are keeping up.

  • catch committed 076e034a on main 
    task: #3515403 Update to selenium/standalone-chrome:133

By: mstrelan
By...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to main, thanks!

Will need a backport MR for 11.x - just gitlab-ci.yml conflicts though.

mstrelan opened merge request !16523

mstrelan’s picture

mstrelan commented
Status: Patch (to be ported) » Needs review

11.x backport

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Seems like a good backport.

As noted in slack though this update seemed to increase the number of randoms. Which is odd because my experience upgrading selenium at work it addressed failures that's what I was hoping for at least.

  • longwave committed 7a4b1cb6 on 11.x 
    task: #3515403 Update to selenium/standalone-chrome:133

By: mstrelan
By...
longwave’s picture

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

Committed and pushed 7a4b1cb6be0 to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

smustgrave’s picture

smustgrave commented

Think this one got stuck in RTBC queue so seeing if a new comment shakes it loose

Edit: It did!

Status: Fixed » Closed (fixed)

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