Update to new driver when D11 "current" uses Nightwatch 3

Note - no work needs to be started on this issue until MR242 has been merged and 📌 Update templates so 11.0 is the default/current branch RTBC is complete

Adding related issue

Now that 📌 Update templates so 11.0 is the default/current branch RTBC is complete we can unpostpone this and start work. It will not be ready for merging yet, but we can at least investigate the timeline for when we can do it.

jonathan1055 → changed the visibility of the branch main to hidden.

Pushed the inital removal and changes, but any testing on this will fail for 'current' because 11.0.7 still needs the legacy chrome browser.

Postponed on 🐛 Service not known with OPT_IN_TEST_NEXT_MINOR Active because MR291 will change the way services are defined.

Probably also postponed until core 11.1 is released and we update CORE_STABLE to '11.1.0'

Is this one still needed? We might have solved it already, so if that's the case let's close it.

Maybe, I've forgotten the details so will check back, and find out the Nightwatch versions now used in the variants.

I will rebase MR257 anyway, and if there's nothing to be done, then the MR can be used to remove those @todo, etc.

OK I had to go back and fully refresh what we are doing here.

.nightwatch-base has all the variables

DRUPAL_TEST_WEBDRIVER_HOSTNAME_LEGACY: localhost
DRUPAL_TEST_WEBDRIVER_HOSTNAME: selenium
DRUPAL_TEST_WEBDRIVER_PORT_LEGACY: '9515'
DRUPAL_TEST_WEBDRIVER_PORT: '4444'
DRUPAL_TEST_WEBDRIVER_CHROME_ARGS: '--disable-dev-shm-usage --disable-gpu --headless'
DRUPAL_TEST_WEBDRIVER_W3C: 'true'

There is no plan to change this

'current' nightwatch has

# Use the legacy webdriver non-W3C-compliant settings
# @todo In MR257 remove these three webdriver variables from here, and add
# them into 'nightwatch (previous major)'
DRUPAL_TEST_WEBDRIVER_HOSTNAME: $DRUPAL_TEST_WEBDRIVER_HOSTNAME_LEGACY
DRUPAL_TEST_WEBDRIVER_PORT: $DRUPAL_TEST_WEBDRIVER_PORT_LEGACY
DRUPAL_TEST_WEBDRIVER_W3C: 'false'

because when 'current' was 11.0.n we needed to use the old values

The nightwatch variant jobs extend nightwatch, not .nightwatch-base. The 'max PHP', 'previous minor' and 'previous major' do not modify anything from 'current' so they all have the legacy variables.

next minor and next major both have

# Set the driver variables to the W3C-compliant versions.
# @todo In MR257 remove these three webdriver variables.
DRUPAL_TEST_WEBDRIVER_HOSTNAME: !reference [.nightwatch-base, variables, DRUPAL_TEST_WEBDRIVER_HOSTNAME]
DRUPAL_TEST_WEBDRIVER_PORT: !reference [.nightwatch-base, variables, DRUPAL_TEST_WEBDRIVER_PORT]
DRUPAL_TEST_WEBDRIVER_W3C: !reference [.nightwatch-base, variables, DRUPAL_TEST_WEBDRIVER_W3C]

This is because the two 'next' variants need the later versions of the variables, not the legacy values.

I checked in this test to see what Nightwatch versions are loaded for the available varaints (ignore the failures, the project has no nightwatch tests, just a dummy file to trigger the jobs)

• current core - core 11.1.1, Nightwatch 3.9.0
• next minor - core 11.2-dev, Nightwatch 3.9.0
• previous minor - core 11.0.8, Nightwatch 2.4.2
• previous major - core 10.4.1, Nightwatch 2.4.2

The changes done in MR257 is to have 'current', 'max php' and 'previous minor' all use the newer variables, and just 'previous major' use the legacy values. This is done by removing the overrides in 'current' and just adding them to 'previous major'. This also means that the two 'next' varaints can simply inherit from 'current' and no longer need their own overrides.

But this is not actually correct, because 'previous minor' still needs the legacy variables as it uses version 2.4.2. That's a small change which I will do after I've run an actual test with this MR - I can use Decoupled Pages 📌 Testing gitlab_templates merge requests Active for that.

Also the MR has some changes to the MINK_DRIVER_ARGS_WEBDRIVER variable in PHPUnit jobs, which will need to be checked.

As predicted, the Nightwatch 'previous minor' fails with this MR, but all other variants are OK
https://git.drupalcode.org/issue/decoupled_pages-3422594/-/pipelines/402046

Re-run with commit for 'previous minor' to use the legacy values, and they all pass
https://git.drupalcode.org/issue/decoupled_pages-3422594/-/pipelines/402063

The changes made for the Nightwatch variants are all still valid, nothing needs to be done. I re-ran the Decoupled Pages MR8
current 11.1.4, max php, next minor (11.2x-dev) all run Nightwatch 3.9.0
previous minor (11.0.12) and previoius major (10.4.4) both use Nightwatch 2.4.2
See this pipeline for MR8 for the details

PHPUnit

The changes in the PHPUnit jobs involve the variable MINK_DRIVER_ARGS_WEBDRIVER
.test-variables has the two definitions MINK_DRIVER_ARGS_WEBDRIVER and MINK_DRIVER_ARGS_WEBDRIVER_LEGACY

As it stands in the main branch, the phpunit 'current' job has the override to use the legacy variable
MINK_DRIVER_ARGS_WEBDRIVER: $MINK_DRIVER_ARGS_WEBDRIVER_LEGACY
which means that all the variants would use this value. However, the two 'next' variants contain
MINK_DRIVER_ARGS_WEBDRIVER: !reference [.test-variables, variables, MINK_DRIVER_ARGS_WEBDRIVER]
meaning that they use the new value not the legacy one.

The existing work done in MR257 removes the legacy override in the 'current' phpunit job, but adds it back only for 'previous major'. Which means that 'previous minor 11.0 is using the new values. We need to check if this is right, because for Nightwatch the 'previous minor' still needed the legacy value.

I think MR257 looks good, but yeah, I totally agree that we if we react to the nightwatch version to set the variable one way or another that'd be amazing.

We should probably try that here in this issue. I was thinking that we could use rules:variables (https://docs.gitlab.com/ci/yaml/#rulesvariables), but I'm not sure what the if conditions will be, so I'm happy for it to be calculated in the .composer-base job where we do all the other variables calculations.

I've pushed some changes, the detection of the Nightwatch version is working, but now I see why the variables are empty in build.env - thi si s because they are only defined in .nightwatch-base so are empty at the time the composer job is running. We have several ways forward -

1. move the definition of the variables from .nightwatch-base into .composer-base, or
2. move the definition of the variables from .nightwatch-base into hidden-variables.yml
3. do the derivation of the variables in .nightwatch-base no .composer-base

I think option 3 is better, as then the work is only done if the Nightwatch job is actually being run. It will sometimes be wasted effort if doing it always in Composer job.

Option 4: move the variables to .test-variables where we already have variables related to testing?

Scratch that, only used "too late".

I think we can do option 3, but we need to keep BC in case somebody has given values there for their jobs. So we can "null" the values and then derive them in the job, in a way that it matches what we have now, but also do the derivation if the variable is empty, to keep any possible project-defined override.

I have made the changes to implement option 3. Keeping BC by defaulting the actual variables to blank, and only setting if non-blank. The legacy variables remain, as they may be referred to directly in customisations.

The PHPUnit changes are better done in a separate issue, so I have removed them here. The work is similar, but testing separately is easier.

Code-wise it looks good and clean. I've triggered the decoupled pages pipeline here https://git.drupalcode.org/issue/gitlab_templates-3475581/-/pipelines/45...

I see temporary code and I guess you might want to run additional tests, so marking NW based on that, but the code looks good and I am happy with the approach and the way we are keeping BC. Thanks for creating the separate PHPUnit issue, that'll make it easier here, and then there, to review and test.

I have also triggered the downstream pipelines for d10-plus and d9-basic, both passed and the log shows what we expect. In particular, in d9-basic, the "current" variant automatically uses the legacy values, because it is running Drupal 10 not 11. The "previous major" is running Drupal 9.5 and an even earlier Nightwatch version 1.7, and it uses the same legacy values.
https://git.drupalcode.org/project/gitlab_templates_downstream/-/pipelin...

Here is a test of overriding the variables in Max PHP and here is the job showing that it worked. The legacy values were used, but the log message is confusing, so I'm going to improve that. The message is shown, then we do the checks for if the variable is empty, and may not make any changes. This needs to be better.

Pushed a change so the message is given only when the value was blank and is being set. In the normal job we get

Nightwatch version is 3.9.0
DRUPAL_TEST_WEBDRIVER_HOSTNAME set to 'selenium'
DRUPAL_TEST_WEBDRIVER_PORT set to '4444'
DRUPAL_TEST_WEBDRIVER_W3C set to 'true'

But when overrides are used we get get the version line but not anything else - shown in this job

Here is the updated documentation page

I have removed the temporary code which skips the other downstream jobs, and this is ready for review.

I'd suggest changing the output slightly and always show it in the job, whether is derived or because it's set by the user.

We could do something like:

Nightwatch version is 3.9.0
- Webdriver: selenium:4444 (w3c: true)

Other than that small change I think it looks good to go. Setting to NW for that small change.

That info is actually always shown later in the Nightwatch job - see https://git.drupalcode.org/project/gitlab_templates_downstream/-/jobs/47... But I like your concise output at the top so I will change that.

Done. I left in the echos but changed to say "Using default ..." and therefore the absence of these indicates that the values were preset in a custom job. d9-basic and d10-plus all pass. Here's an example

Here is a test with override values

Ready for review. Could you trigger the Decoupled Pages pipeline please?

Two really small nitpicks on the MR.

Thank you. Both are done.

Did you notice that the older versions of Nightwatch (2.4 and 1.7) have output in colour, but version 3.9 is plain white. I don't know if there was a default of color 'on' which is now 'off'. I have just checked nightwatch --help and there does not appear to be any option to control this. So I guess we just go with it.

Thanks for the changes! Marking RTBC.

Good catch on the colors. Maybe an issue with the nighwatch code where it detects color support? (for example:https://github.com/nightwatchjs/nightwatch/issues/3601)

This can be checked on another issue in any case.

Merged. Thanks!

Do you want an issue opened for the color support? I am not sure how to progress it, and I've got enough other open issues on the go at the moment. But if someone else, who knows more about Nightwatch than I do, would like to pick it up, that's fine with me.

We can defo open the issue so at least we know about it. It doesn't need to be top of the list and it can wait until other issues have been dealt with... and of course, anybody could pick it up in between.

I've opened ✨ Nightwatch tests have lost their colors Active

Fix typo in title