Migrate Nightwatch Axe tests to PHPUnit

Not sure if this is a valid test but I reused MediaSourceImageTest::testMediaImageSource
Disabled required alt text
Commented out line $page->fillField("{$source_field_id}[0][alt]", ''); so the image had no alt text

Then ran

    $this->drupalGet($this->assertLinkToCreatedMedia());

    $this->executeAxe();;

Expected it to fail but I didn't and just continued along.

Is there something I'm missing?

Now we have https://www.drupal.org/project/drupal/issues/3293469

Do we need this issue? Is PHPUnit going to give us different or better results?

That I don't know. You've got the PHP developers & the JavaScript developers. Do you think PHPUnit is better supported by modules/themes than Nightwatch?

I think there's some split between nightwatch/cypress in the JS testing community. I'll ask around and see if we can get some other thoughts on this.

I'd suggest that we might want to test different things with them.

I think since axe-core package is now available in version 10.1.x (with the nightwatch update) I think a PHP example could be good to have but I am not sure if we need the same setup for an install profile and such.

Is the AccessibilityTest being picked up by the pipeline? I am not seeing it https://dispatcher.drupalci.org/job/drupal_patches/164094/console if it is.

I think it would be great if a11y tests ran automatically for most paths requested by a test. Currently, it appears that Axe tests in Nightwatch are only run for explicitly-defined paths in dedicated tests.

Based on limited experience with Nightwatch, setting up test conditions appears to be easier for PHPUnit functional javascript tests than it is for Nightwatch tests.

It also appears that there are currently more functional javascript tests than Nightwatch tests (at least, going by the number of test files turned up with a quick find): 242 vs 34.

Doing the a11y tests with PHPUnit might help cast the widest net.

In light of #3467492-15: [policy, no patch] Replace Nightwatch with Playwright, I'm working on this against 11.x and planning to add support for options so that we can use it like we use Nightwatch's axeRun().

I need implementation advice.

The axe core javascript is an npm package.

In a Slack discussion debugging the failed job, @fjgarlin said that core functional javascript tests don't have access to the core/node_modules directory. So, the file must be provided some other way.

It has to be built and the dist files aren't in the repo, so it can't be retrieved directly from the repo.

I didn't find any other cases like this, so I experimented with installing it locally via Composer into vendor by defining a repository in composer.json as follows and then requiring it as a Composer dev dependency. This is working locally.

Is this a viable solution, or are there other preferred solutions?

        {
            "type": "package",
            "package": {
                "name": "npmjs/axe-core",
                "version": "4.10.2",
                "dist": {
                    "url": "https://registry.npmjs.org/axe-core/-/axe-core-4.10.2.tgz",
                    "type": "tar",
                    "shasum": "85228e3e1d8b8532a27659b332e39b7fa0e022df"
                }
            }
        }

Adding to my last comment:

In case it's preferable to commit the axe javascript file into core, AFAICT it is Mozilla Public License 2.0 and is 541K minified.

Hid the original MR because the new MR builds on it.

Bumping to major due to @catch's comment #8 on #3542476-08: [Policy] Officially move to WCAG 2.2 AA Support.

Adding to #16:

AFAICT, to move this forward we just need to make a built version of the axe-core javascript available to these functional javascript tests.

It doesn't matter whether the script is stored in the repo, passed in as an artifact from another part of the gitlab build process, installed with yarn install for these tests, installed via php-forge/foxy, or something else.

What is the desired mechanism for that?

We'd normally add it as a JavaScript development dependency (same as cspell, eslint etc.) and then it will be installed in the general yarn install that CI pipelines already do. That way it's also available for people running the tests locally.

@catch,

It's already a JavaScript dev dependency as a dependency of nightwatch-axe-verbose, but it wasn't available to the tests.

Here are snippets from pipeline.yml.

'🖱️️️ PHPUnit Functional Javascript':
  <<: [ *with-composer, *run-tests, *default-job-settings ]
  ... etc ...

'🦉️️️ Nightwatch':
  <<: [ *with-composer-and-yarn, *default-job-settings ]
  ... etc ...

Based on your comment, I'll assume that it's OK to change pipeline.yml and play around with it from there.

Not sure if this plays into anything but know there's been talks for a little bit about removing nightwatch eventually.

@smustgrave, this will add axe-core as a main dependency so that nightwatch-axe-verbose isn't required anymore.

It probably won't remove nightwatch-axe-verbose though, unless all of the existing Axe tests are also to be converted in this issue (rather than a followup).

I pulled the changes and started to test, all was looking good until I realised all I was doing was replicating what was already being done in the pipeline.

I realized that the Nightwatch Axe tests need to be removed. And when I reviewed them, I realized the breadcrumb case that was added for #3223147: Claro breadcrumb doesn't meet minimum target-size isn't represented here.

Also, the current Nightwatch a11y tests can switch themes based on CLI arguments. These replacement's don't.

It doesn't seem necessary right now, but when Gin comes to core it would be handy. I think it would be better in a follow-up issue.

Ok, now I think it's ready.

I removed the existing Nightwatch a11y tests, but I left the yarn nightwatch-axe-verbose dependency to make the transition smoother in case other tests are added in the near future.

I think there is a bit of boilerplate we can remove, have marked up in the MR. Also wondering about a base class, see comment in the MR. These are not blockers though so I'd leave the status as NR, except I think the $session->wait() needs attention, so setting to NW.

Looking into the comments.

I made some changes based on @mstrelan's suggestions and raised some questions in comments on the MR.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Updated the fork.

This needs a rebase. Heads-up: you'll need to add the #[RunTestsInSeparateProcesses] attribute to all of the new test classes when you do since they're all children of BrowserTestBase. I'm setting the status to Needs Work for that.

I didn't do a completely thorough review of the MR, but I did read most of it. This is looking good so far. I resolved a couple of MR comment threads and left one comment reply.

I rebased the MR, added required attributes, and replied to @dcam's comment on the MR.

Pipeline is failing because .with-composer-and-yarn was removed from pipeline.yml.

It needs some gitlab expertise. I made some failed attempts to follow the pattern used in .gitlab-ci.yml for .yarn-install-from-cache.

The solution to getting yarn install back for functional JS tests may be affected by #3572375: [ci] Run tests in top level pipeline, use recursion for secondary test runs.

Ready for another review.

Addendum FTR: The blocking issue #3572375: [ci] Run tests in top level pipeline, use recursion for secondary test runs was fixed.

Fixing bugs early reduces the costs in a project. With accessibility that this can scale up quickly. We do need to shift left to see that we're maximizing our text coverage as much as we can, while the ideas are still "fresh" in our heads. If we wait for a few months, or even longer for a user on a live site to report the bug it becomes much more expensive.

Early automated testing is key. Lets get this patch in!

Looks like there are merge conflicts.

Also, there were recent additions to the Navigation module's a11yTest.js that need to be copied to the corresponding AccessibilityTest.php here.

Something that's also needed in the summary is a dependency evaluation since this ticket is proposing a new package that would have to be kept up with by the team. So need to know things like how often it does release, number of open issues (any issues we should be concerned with), does it rely on any dependencies, etc.

@smustgrave

Thanks for looking at this and changing the status. I should have changed that previously.

This is now using the Axe-core dependency that's already in core for Nightwatch. I updated the IS.