Add W3C compliant JS testing

Project Browser reported that their Nightwatch tests are failing when run against "next major" which uses 11.x and Nightwatch 3.7, which is probably related to this issue.

https://git.drupalcode.org/issue/project_browser-3466307/-/jobs/2508579#L40

⚠ Failed to connect to Selenium Server on localhost with port 9515.
...
  Error
    An error occurred while creating a new Selenium Server session: [SessionNotCreatedError] session not created: Missing or invalid capabilities
  (Driver info: chromedriver=123.0.6312.58 (6b4b19e9dfbb93aa414dc045bd445287977d8d7a-refs/branch-heads/6312_46@{#3}),platform=Linux 5.10.220-209.869.amzn2.x86_64 x86_64)

This is possibly because Nightwatch 3 is trying to connect to the older drupalci/chromedriver:production container, I think the templates will need the newer selenium/standalone-chrome container to be added from 📌 Use selenium/standalone-chrome instead of our chromedriver image Needs work ?

https://git.drupalcode.org/project/drupal/-/merge_requests/8735/diffs is the changes that were made in core and so similar changes likely need to be made here.

Thanks for the link to the MR, that really helps.

I see it's mostly this block: https://git.drupalcode.org/project/drupal/-/blob/11.x/.gitlab-ci/pipelin...
And these two variables: https://git.drupalcode.org/project/drupal/-/blob/11.x/.gitlab-ci/pipelin...

I'm trying a straight-out replacement of the driver.
Downstream pipelines: https://git.drupalcode.org/project/gitlab_templates/-/pipelines/261318

It seems that D10 projects will need the "old" (current) version and D11 projects will need the new version.

@fjgarlin ideally everything can use the same version. I think the templates should support both w3c testing and nonw3c testing. I think the nightwatch update has forced the hand for nightwatch testing on 11.x but nightwatch tests in contrib and custom are rare.

The new driver worked directly for phpunit functional tests, but failed for nightwatch tests. I guess it's due to the nightwatch library required by core.

So we'll need to do some if/else or variable replacements in a few places to allow for both.

This is ready to review now.

See downstream pipeline running successfully in all cases: https://git.drupalcode.org/project/gitlab_templates/-/pipelines/273652
* The legacy driver and variables are used in versions prior to 11.0.x
* The new driver and variables are used in version 11.x

If the changes are backported to previous Drupal versions ( 📌 Use selenium/standalone-chrome instead of our chromedriver image Needs work ), like 11.0.x or 10.4.x, then we can just remove the overrides for those jobs.

Once all supported versions use the new driver only, we can remove all the "legacy" related code.

I have tested this with Scheduler, which has two javascript tests. They passed at "next major" and "previous major" but not at "current". Here is an example of the error:

WebDriver\Exception\CurlExec: Curl error thrown for http POST to
    http://selenium:4444/session/c16bf73626b1c5496b7d14d823beb0b3/element/f.F1D21469553F74C96E60D94AD65296CE.d.7BB145E685E27718F35E32982D32F53E.e.15/click
    
    The requested URL returned error: 400 Bad Request

Here is the failing test. It's interesting that the test passes at Core 9.5.11 and 11.0-dev but not 10.3.4-dev

I changed the approach slightly based on some of the feedback.
Downstream pipelines: https://git.drupalcode.org/project/gitlab_templates/-/pipelines/275511

I'm not sure about the why for that 10.3.4 vs others. Let's run another test and see if we can find out why.

I had noticed that there were no changes for the phpunit tests, and was going to suggest exactly the additional lines that you have added. The tests pass at all three variants now.

https://git.drupalcode.org/project/scheduler/-/pipelines/275571

I think it is an improvement to remove the code snippet and echo the ${VARIABLE_NAME} instead. It is cleaner and also easier to understand.

The phpunit changes are not actually using the new W3C configurations, due to how the 'next major' extends phpunit. This can be seen in the next major test job which still has the legacy values in the log. I will push a change to fix this.

I have pushed this change to move the

services:
    - <<: *with-database
    - <<: *with-chrome-legacy

into .phpunit-base, so that it can be changed to *with-chrome for Next Major. I know there was a reason why we extended all the PHPunit jobs from phpunit not .phpunit-base but for this scenario it would have been nicer to extend from the base. That is what we do in the Nightwatch jobs.

The above worked for the services. But to get the desired value for MINK_DRIVER_ARGS_WEBDRIVER we need to use a reference back to the orginal value from .test-variables as this variable has already been over-ridden in the phpunit (current) job. This can be done using MINK_DRIVER_ARGS_WEBDRIVER: !reference [.test-variables, variables, MINK_DRIVER_ARGS_WEBDRIVER]
See this commit

Interestingly, the javascript tests I am using obviously do not need the MINK_DRIVER_ARGS_WEBDRIVER value, as I have set this to 'this-is-a-dummy-value' and the test still worked. Maybe the Nightwatch tests need it.
https://git.drupalcode.org/project/scheduler/-/jobs/2679114#L395

Here's a proper test showing the new values used in the phpunit next major job.

The same problem happens with Nightwatch (Next Major), the intended W3C services and updated variables are not being set in this job because it extends from nightwatch (the current core test) not .nightwatch-base

Here is an example. I have just created a dummy nightwatch test file, to get the jobs to appear (because Scheduler does not have any Nightwatch tests). The nighwatch (current) and nightwatch (next major) jobs both have the same values, from the $DRUPAL_TEST_WEBDRIVER_*_LEGACY variables.

This commit fixes the problem
https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/248...

There are also some redundant lines in the other variants, which can be removed
https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/248...

Re-run Scheduler tests, and Nightwatch next major has the correct new values.

I've tested this MR on 💬 Nightwatch tests failing due to lack of "capabilities" - need Selenium driver Needs review , the issue which discovered this problem. That nightwatch test runs better now, which is good to see.

The code looks much cleaner this way. Thanks for the refactoring. I asked a small question in the MR.
Once that question is replied or addressed and the @todo in the code is restored back to what it should be, I think it might be ready.

Setting to needs work based on that.

Good spot. I've pushed a commit to move that variable.

Regarding the @todo, where you referring to this commenting out in 'check versions'?

    # Check on merge requests.
    # Temporarily removed on MRs, as this will always fail until D11 is current
    # and a failure here can obscure the fact that 'Check Code' may have also failed.
    # @todo uncomment in https://www.drupal.org/project/gitlab_templates/issues/3463894
    # - if: $CI_MERGE_REQUEST_IID
    # - if: $CI_PIPELINE_SOURCE == 'merge_request_event'

That was intended to stay in this MR, so that we can more easily test MRs and not have them failing for 'check versions' as that can hide a fault in 'check code', when the pipeline always ends red you start to ignore it. The code check will still run on scheduled pipelines. The commenting out can be removed in the MR for Step 2 of the switch-over.

Gotcha. Makes sense.

I ran the downstream pipelines and it seems good: https://git.drupalcode.org/issue/gitlab_templates-3462681/-/pipelines/27...

I am happy with the code and the results so far, so I'll mark it RTBC but it'd be great to have extra eyes from @alexpott or @longwave on the code.

Once merged, we'll have a conflict in the D11 issue MR, but that's fine and expected as we'll need to tweak what gets "legacy" and what doesn't.

Thanks. The test I ran on 💬 Nightwatch tests failing due to lack of "capabilities" - need Selenium driver Needs review was a good outcome.

As per @alexpott via slack

I think we could have instructions for how to change it back because the legacy mode is still supported on 11.x - it will get deprecated at some point

So there are some cases where even 11.x can support the legacy driver (I guess that's what we saw in PHPUnit jobs).

In any case, let's add some documentation for this, probably in the https://project.pages.drupalcode.org/gitlab_templates/jobs/nightwatch/ or/and PHPUnit page.

If it is something that many projects may need, or it might be needed to switch to test and switch back, etc, we could make it easy and introduce a _w3c_compliant_js_test flag variable, or something like that.

Looking at BC concerns:

How will this work for projects that needed additional services ?

Ref: https://git.drupalcode.org/project/s3fs/-/blob/c5c85de5c352161810db1dadb...

Re #25 - the challenge is that we can’t have if/else in services.

—

Re #26 - we are keeping the existing name for the new driver and we should definitely document the process for the new driver, so if modules have issues we have the workaround ready. The new driver is supposed to work as well in most cases, specially phpunit.

The BC change comes from core, we’re just reacting to it. Ideally, in a few versions time we might even not need the “legacy” tweaks anymore (or only for D10).

Re #25 - the challenge is that we can’t have if/else in services.

Yes OK, let's keep it simple and just document how to add back the legacy services.

For the legacy variables, it might be easier if we had a resubale reference for the three, called something like legacy-driver-variables which would contain all the !reference [ ] bits. Then that snippet could be added in the jobs in this MR, using <<: *legacy-driver-variables.
The instructions to add it for D11 jobs that need to revert would be to add that one line into the job.

I pushed this change to create the re-usable snippet.

I had a first draft at updating the Nightwatch doc page with a section on the webdriver version. You can read the updated page here and I don't know if I have got the exact terminology right, so let me know, or make suggestions in the MR.

The earlier work done on this MR added a new variable DRUPAL_TEST_WEBDRIVER_W3C which is set to "true" in the .nightwatch-base, so this needs to be set to "false" in the legacy section. I added that here

Tested on Scheduler with Current and Next Major. The Nightwatch tests are dummy, but they demonstrate the variables values. The logs also display the variables and we can see how they alter.
https://git.drupalcode.org/project/scheduler/-/jobs/2715438

Retested on 💬 Nightwatch tests failing due to lack of "capabilities" - need Selenium driver Needs review and it has the new W3C driver and variables as required
https://git.drupalcode.org/issue/project_browser-3469490/-/jobs/2715626

Thanks so much for the additional changes.
Downstream pipelines: https://git.drupalcode.org/issue/gitlab_templates-3462681/-/pipelines/27...

I'm reviewing the code now.

I only made a minor request in the MR, to change the name of a variable. They called it like that in core, but we need way more logic for the different variants and I think the suggested name change will be consistent with the rest of the naming we've been using all over (aka "legacy").

The rest looks good and the pipelines seem happy. I also think that the addition in the documentation is good and it's easy enough. I wonder if we should also add it to the PHPUnit page as those jobs could also be affected and would require the same workaround.

Back to "needs work" based on the two above things, but things are looking good and solid.

Thanks for the review, suggestions applied.

Scheduler test
https://git.drupalcode.org/project/scheduler/-/jobs/2716917

Project Browser test
https://git.drupalcode.org/issue/project_browser-3469490/-/jobs/2716861

Downstream pipelines: https://git.drupalcode.org/issue/gitlab_templates-3462681/-/pipelines/28...

Some of the Project Browser tests were failing, I just retriggered a run in case it was a one-off.

The code looks good to me.

Some of the Project Browser tests were failing, I just retriggered a run in case it was a one-off.

I think they fail anyway. Unfortunately there are no scheduled pipelines on Project Browser, so I could not easily check the status of the default branch. Thoses tests might not pass when using a W3C-compliant browser (which is one of the main purposes of this issue)

The nightwatch error has changed as you point out in the PB issue, but the "phpunit (next major)" error happens with this MR but not on the 2.0.x branch: https://git.drupalcode.org/project/project_browser/-/pipelines/274761

I just triggered a brand-new pipeline against 2.0.x: https://git.drupalcode.org/project/project_browser/-/pipelines/280185

Good idea to try that screen-size option as per your comment on #3469490-10: Nightwatch tests failing due to lack of "capabilities" → .
https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/248...
But I am not sure that the variable MINK_DRIVER_ARGS_WEBDRIVER will actually help here, as that is only used in the Nightwatch tests. The PHPUnit calls have the arguments hard-coded. This is probably something else we need to look at, because it is very easy to change one variable and think that the fix will be respected.

oh, that's totally right! let me change it as well for PHPUnit first and retry and then we see about the variable (great suggestion btw).

Pinning version as per 🐛 Nightwatch and Functional JavaScript fails since selenium/standalone-chrome:128 Fixed

I've tried pinning the version to a specific one, setting the window-size, and both things together. None of them make the "phpunit (next major)" any happier.

I wonder if this is a "project_browser" specific issue due to AJAX loading elements. We don't get the issue with the legacy driver, but Drupal core is shifting to the new one anyway, so I guess it's better that they face this issue sooner rather than later.

Not sure what to do about the code in this issue. All the other projects we tried in seem to work well.

In a separate problem, I tried to make Project Browser revert to using the legacy driver for PHPUnit Next Major, using

services:
    - <<: *with-database
    - <<: *with-chrome-legacy

but this gives the error `.gitlab-ci.yml`: Unknown alias: with-database
See https://git.drupalcode.org/issue/project_browser-3469490/-/pipelines/280982

Fair enough, * is for use within the same .gitlab-ci.yml so it works in .main, but we need to use !reference in other projects. So I tried

services:
    - <<: !reference [ .with-database ]
    - <<: !reference [ .with-chrome-legacy ]

and this produces

Unable to create pipeline
Undefined error (01J7JXQXD6PECQZRW8ZGGHV4AA)

See https://git.drupalcode.org/issue/project_browser-3469490/-/pipelines/281048

I have not been able to find the syntax for using <<: with a !reference [ ]

Maybe - !reference [ .with-database ]

I had tried that syntax with variables: and it failed, but yes it does work with services: because of the - . Here's a test with

phpunit (next major):
  services:
    - !reference [ .with-database ]
    - !reference [ .with-chrome-legacy ]

https://git.drupalcode.org/project/scheduler/-/jobs/2729622
The variables are not reverted, but the services should be. Is there a way that I can confirm we are using .with-chrome-legacy not .with-chrome ? I have downloaded and compared the logs before are after, and cannot see anything significant.

Re-ran with CI_DEBUG_SERVICES: 'true' and there is more to see.

W3C driver eithout reverting
https://git.drupalcode.org/project/scheduler/-/jobs/2729950
Log shows service:selenium/standalone-chrome-selenium

Reverting to legacy driver with the code in #43
https://git.drupalcode.org/project/scheduler/-/jobs/2729826
Log shows service:drupalci/webdriver-chromedriver-chrome

So that appears to work OK. Now for the variables.

I have pushed some more changes to allow reverting to the non-W3C driver for D11 jobs that do not want it. It was a bit tedious, but I learned a few things about the pipeline syntax and how it is built, which I will share here for future reference

1. In a contrib project file, within the extends: key, it is possible to use a reference to load from an included file, but oddly the syntax is not the expected
   

     extends:
       - nightwatch
       - !reference [ .legacy-driver-variables ]
   

   which produces unknown keys in `extends` (#). The actual syntax is

     extends:
       - nightwatch
       - .legacy-driver-variables
   

   without the !reference keyword, even thought the alias is not locally defined, but is from the gitlab templates file

2. Using extends: in a contrib file to load config to set variables does work, but these variables are then overwritten/superceded with values defined in the original job definition's variables: key (in the gitlab templates file). This is actually quite logical when you work through it, but at first attempt I was thinking that anything added to a project's file should take precedence over what is in the included file. So this was not going to be a way to override the W3C variables back to the legacy values.
3. The syntax <<: *named-alias to load and merge a multiple set of configuration values works fine when the anchor alias is defined in the same file. But you cannot use this syntax in a contrib module to load config from another included file (such as .main.yml). The usual way to do this would be <<: !reference [ .named-alias ] , but this produces Undefined error (01J7P1NG7GTVF8Q1Q1HBZR1047). I tried many other varieties too but it appears that this syntax has not been catered for.

So in the end, the variables are named out explicitly in the contrib job that needs to revert them. I had hoped to have a single alias/snippet to do all the reverting, to make the instructions simple etc, but I failed to make that work. This MR now contains both the updated Nightwatch page and the PHPUnit page, with the full code needed to do the revert.

Here's a test on Project Browser, reverting to the non-W3C driver for PHPUnit Next Major, and it passed as expected.
https://git.drupalcode.org/issue/project_browser-3469490/-/jobs/2762568

MR248 is ready for review.

I also noticed that the variable DRUPAL_TEST_WEBDRIVER_CHROME_ARGS: "--disable-dev-shm-usage --disable-gpu --headless" is defined in .test-variables: but is not used in any job, except for writing to >> ./core/.env in .nightwatch-base.. These values are also repeated insisde the definitions of MINK_DRIVER_ARGS_WEBDRIVER_LEGACY (main) and MINK_DRIVER_ARGS_WEBDRIVER (d7.main). This variable probably came from the core implementation of Nightwatch tests, as it is included there. But is it actually needed in the gitlab templates ./core/.env file for contrib testing? If so, maybe the variable should be used inside the definition of the MINK_DRIVER_ARGS_WEBDRIVER variables, providing the same values are required. Or if the variable is not required at all it should be deleted from .test-variables as having it there gives the impression that changing it would have some effect.

Fully agree that the last bit could/should be done in a follow-up.

Thanks so much for all the back and forth until finding the right syntax for each job and for all the testing. I think this is important in case we start having people mentioning that their tests aren't quite working with the new plugin.

For cases like the one in Project Browser, they'll need to figure out why it fails with the new plugin. We are setting the same one as core and providing a fallback, which I think it's great.

I've triggered all downstream pipelines here: https://git.drupalcode.org/issue/gitlab_templates-3462681/-/pipelines/28...
That, in addition to your testing, and the fact that the code looks good, grants giving this a RTBC.

Big thanks again!

Merged! Thanks for the @todo as well.

We will have a rebasing/merging conflict on 📌 Update templates so 11.0 is the default/current branch RTBC , but it's great to have this in so that we can move on to the next one.

Regarding Step 2 here ( https://www.drupal.org/drupalorg/blog/gitlab-ci-templates-will-make-drup... → ) - given that the above issue will go into "Needs work", and that we are now too close to DrupalCon Barcelona, I think this won't happen until after the conference, which I think it's fine and it will give us more time to test the changes made in this issue.

... now too close to DrupalCon Barcelona, I think this won't happen until after the conference, which I think it's fine and it will give us more time to test the changes made in this issue.

Yes that's all good. Do you have a plan to release 1.5.8 before then? That should not be too much of a risk.

I can rebase and fix the conflicts in MR242 if you are busy with other things (prep for DrupalCon Barcelona, no doubt!)

I have updatde the issue summary with what was done, and linked the doc pages for quick reference to the instructions for using the legacy driver.

I did 1.5.8 right after the merge: https://git.drupalcode.org/project/gitlab_templates/-/tags/1.5.8
I'll set it as default too shortly.

I just did the rebase, but with all sort of conflicts, and I was going to review it now. I am happy for you to take over the rest in MR242, like addressing the "@todo", etc. I appreciate that.

Rebase complete, and I think it looks ok, so I'll let you do the rest in there. Thanks.

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