Problem/Motivation

We ship with Chromedriver 98 but Chromedriver 107 is out.

The JavaScript Chromedriver is only used by local Nightwatch tests if the environment variable DRUPAL_TEST_CHROMEDRIVER_AUTOSTART is set. This is not documented anywhere.

I am also fairly sure that Chromedriver needs to be close to your local version of Chrome for things to work properly. As we are shipping with Chromedriver 98 and Chrome 107 is currently out, I suspect nobody is using this feature. If someone does want to use this, it should be their responsibility to install a local Chromedriver that matches their version of Chrome.

Steps to reproduce

Proposed resolution

Remove the dependency and the supporting code.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Comments

longwave created an issue. See original summary.

longwave’s picture

Status: Active » Needs review
StatusFileSize
new5.76 KB

I just realised that I think the only time this dependency is used is if the environment variable DRUPAL_TEST_CHROMEDRIVER_AUTOSTART is set; I assume DrupalCI uses the separate chromedriver container instead.

longwave’s picture

I can't find where DRUPAL_TEST_CHROMEDRIVER_AUTOSTART is documented. Should we consider dropping this feature and dependency? It is only used in local Nightwatch tests if the environment var is set to true. It seems a bit pointless updating this dependency all the time as it should stay in sync with your local Chrome, afaik it doesn't work properly if it is too out of date. Nobody seems to be complaining about it, so I suspect nobody actually uses this feature.

catch’s picture

Makes sense to drop it if it's not being used. Since it's test-only, I think we could do it in 9.5/10.0 with a CR still.

andypost’s picture

Status: Needs review » Needs work
Issue tags: +Needs subsystem maintainer review

++ to remove as this dependency really huge (>100MB for no reason for every run) with binary elements which are not available for every hardware architecture

longwave’s picture

Title: Update to Chromedriver 107 » Remove Chromedriver as a JavaScript dependency
Issue summary: View changes

Updated IS with new direction.

longwave’s picture

Status: Needs work » Needs review
StatusFileSize
new19.01 KB

Not even sure we need the "local" Nightwatch test_settings any more?

nod_’s picture

Status: Needs review » Needs work
Issue tags: -Needs subsystem maintainer review

That's fine with me. I think we might need to point to an doc that explains how to start chromedriver in the docs as well : https://www.drupal.org/docs/automated-testing/phpunit-in-drupal/running-...

Need to remove DRUPAL_TEST_CHROMEDRIVER_AUTOSTART from .env.example too.

pooja saraah’s picture

Assigned: Unassigned » pooja saraah
pooja saraah’s picture

StatusFileSize
new19.3 KB
new22.01 KB

Thanks for your suggestions @nod_
Addressed the comment #8
Attached patch against Drupal 10.0.x
Attached reroll patch

pooja saraah’s picture

Assigned: pooja saraah » Unassigned
pooja saraah’s picture

Status: Needs work » Needs review
longwave’s picture

Status: Needs review » Needs work

#10 does not apply.

longwave’s picture

Status: Needs work » Needs review
StatusFileSize
new19.3 KB

After playing with this for a bit I have a better suggestion: we can keep the DRUPAL_TEST_CHROMEDRIVER_AUTOSTART variable, but Nightwatch can handle starting Chromedriver by itself; we don't need any custom code to do this. If Chromedriver is not present, then you get a nice error message:

$ yarn test:nightwatch --env local
...
┌────────────────────────────────────────────────────────────────────────────────────────────────────┐
│                                                                                                    │
│   ChromeDriver cannot be found in the current project.                                             │
│                                                                                                    │
│    You can either install chromedriver from NPM with:                                              │
│                                                                                                    │
│       npm install chromedriver --save-dev                                                          │
│                                                                                                    │
│    or download it from https://sites.google.com/chromium.org/driver/downloads,                     │
│   extract the archive and set "webdriver.server_path" config option to point to the binary file.   │
│                                                                                                    │
│                                                                                                    │
│                                                                                                    │
└────────────────────────────────────────────────────────────────────────────────────────────────────┘

This means we can keep the environment variable but drop the dependency; anyone who was using this setup can continue as before just by installing Chromedriver themselves using the instructions.

andypost’s picture

Nice idea! probably it needs CR to explain how to start/use it now

longwave’s picture

andypost’s picture

Assigned: Unassigned » nod_

fixed https://www.drupal.org/node/3031731/revisions/view/12800444/12849312 about how to start chromedriver the current version

+1 RTBC but #8 requires approval from @nod_

nod_’s picture

Assigned: nod_ » Unassigned
Status: Needs review » Reviewed & tested by the community

All good with me, nice find.

Added to the CR an easier way to run chromedriver: npx chromedriver

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.1.x and cherry-picked to 10.0.x, thanks!

  • catch committed 692d7fe on 10.0.x
    Issue #3317879 by longwave, pooja saraah, andypost, nod_: Remove...
  • catch committed d140747 on 10.1.x
    Issue #3317879 by longwave, pooja saraah, andypost, nod_: Remove...
longwave’s picture

Issue summary: View changes
Issue tags: +10.0.0 release notes
longwave’s picture

Version: 10.0.x-dev » 9.5.x-dev
Issue summary: View changes
Status: Fixed » Patch (to be ported)

Discussed with @nod_, we think this should also be removed from 9.5.x.

longwave’s picture

longwave’s picture

Status: Patch (to be ported) » Needs review
StatusFileSize
new18.6 KB
nod_’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Removing this in 9.5.x makes sense to me. If you were using it you'd not be testing on the currently supported versions of chrome.

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +9.5.0 release notes

Needs a re-roll against 9.5.x

nod_’s picture

Status: Needs work » Needs review
StatusFileSize
new18.44 KB
longwave’s picture

Status: Needs review » Reviewed & tested by the community

  • catch committed f6f1d85 on 9.5.x
    Issue #3317879 by longwave, pooja saraah, nod_, andypost: Remove...
catch’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -10.0.0 release notes

Committed/pushed to 9.5.x, thanks!

Status: Fixed » Closed (fixed)

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

2dareis2do’s picture

If I understand this correctly this docker image is used when testing this:

image: drupalci/chromedriver:production

That seems to run (Session info: headless chrome=106.0.5249.103)

As npm chromedriver is a dependency when running Drupal's PHPUnit Functional Javascript tests, I think it makes sense that this value is a requirement in package.json and should match that version.

e.g.
"chromedriver": "^106.0.1",

I think the confusion arises with running Nightwatch Tests that are run in the browser that require a version that matches the browser version installed.

andypost’s picture

drupalci/chromedriver:production

There's few issue issues https://www.drupal.org/project/issues/drupalci_environments?text=chromed...

But to update chromedriver docker image it needs policy because it's used in Drupal 9.5 still

2dareis2do’s picture

Patch attached if required but probably needs a new issue to test it. Patch based on Drupal 10.1.4

2dareis2do’s picture

Additional patch for installing patch in web directory, Also adding patch again for 10.1.x as I can see I can test against that.

2dareis2do’s picture

StatusFileSize
new368 bytes
2dareis2do’s picture

Ok test is not running, I guess because the issue is closed?

2dareis2do’s picture