updating accessibility tests to be compatible with latest axe release #42

kschuste · 2018-01-05T16:21:43Z

Summary

This fixes issue #41. This unlocks the axe-core dependency and will allow it to pull the latest released version of it. The tests have been updated to pass with the latest accessibility rule added by turning it off.

emilyrohrbough · 2018-01-05T19:01:44Z

I am wondering if it would make more sense to disable the rule in the axe-service here, such that

 const rules =  {
      'landmark-one-main': { enabled: false },
 };

const axeOptions = {
        runOnly: options.runOnly,
        rules: Object.assign({}, rules, options.rules),
      };

This would remove the need to update each accessibility test.

emilyrohrbough · 2018-01-05T19:09:12Z

Then again, maybe not if consumers use this to test at an app level.

mhemesath · 2018-01-05T19:14:08Z

@emilyrohrbough not far off. The service should be configured to allow global options to be passed in through the wdio.conf.js. Those global rules can be accesssed here. We can then append to the injected in script to add a axe.configure() call this tthose options. See doc

emilyrohrbough · 2018-01-05T19:15:50Z

Ah knew there at to be something at a quick glance

mhemesath · 2018-01-05T19:17:53Z

The other option of course is that Terra provides its own axe script and disables injection (will probably slightly speed up tests as less commands are executed). Terra can then configure landmarks off manually. FWIW - I think the globa option is a good idea though.

kschuste · 2018-01-08T19:57:12Z

@emilyrohrbough @mhemesath updated to apply a global rule for consumers, take a look

mhemesath · 2018-01-09T19:22:28Z

src/wdio/conf.js

@@ -10,6 +10,11 @@ exports.config = {
  capabilities: [
    {
      browserName: 'chrome',
+      axe: {
+        rules: {


I would have expected this to be present within the axe config block later in this file

I've looked more into this and the axe config block is not passed in with the config parameter in the AxeService.js file. The axe config here was previously setting inject = 'true', but in the AxeService.js, this was always hardcoded to true and whatever. The config param matches what is set here in the capabilities; hence, I added the axe config for the rules, I have also now moved the inject config to here and removed the hardcoding from the AxeService.js

mhemesath · 2018-01-09T19:23:44Z

src/wdio/services/AxeService.js

@@ -33,7 +33,7 @@ export default class AxeService {
      const specifiedViewports = options.viewports || [currentViewportSize];
      const axeOptions = {
        runOnly: options.runOnly,
-        rules: options.rules,
+        rules: Object.assign({}, axeConfig.rules, options.rules),


the global options should be set once upon initialization of the script by appending that config block to line 24

append the json config to the javascript source code that is read in on lines 23 and 24?

axeCoreSrc = fs.readFileSync(require.resolve('axe-core'), 'utf8'); axeCoreSrc = axeCoreSrc.replace(/^\/\*.*\*\//, '');

yeah, we want to append it so that we only need 1 injection command

mhemesath · 2018-01-10T14:33:40Z

src/wdio/conf.js

@@ -10,6 +10,12 @@ exports.config = {
  capabilities: [
    {
      browserName: 'chrome',
+      axe: {


I actually meant the inverse, why are you putting the config in the capabilities?

as you see in your comment below, I have the config in the capabilities as they are passed into the before function instead of the whole config so I have access to them. Switching to use onPrepare or another function that allows the config to be passed in will not give us access to the global browser object that we are applying the axe config too.

you aren't applying it to the global browser object. Your saving off the config as an instnace variable in the AxeService. Then when the browser command injects the axe config, it can call axe.configure with that config.

mhemesath · 2018-01-10T14:36:43Z

src/wdio/services/AxeService.js

@@ -11,7 +11,6 @@ export default class AxeService {
  // eslint-disable-next-line class-methods-use-this
  before(config) {


I think i see why you are putting config in capabilities... this should probably be onPrepare. You can then use the config param to pull the axe config so you don't have to put it in capabilities.

/** * Gets executed once before all workers get launched. * @param {Object} config wdio configuration object * @param {Array.<Object>} capabilities list of capabilities details */ onPrepare: function (config, capabilities) { },

Doc for the config is here http://webdriver.io/guide/testrunner/configurationfile.html

Moving to using onPrepare does not allow us to have access to the global browser variable.
Using before is the earliest we can have access to the browser variable, which before only has the two params of the capabilities and the specs.

mhemesath · 2018-01-10T14:42:05Z

src/wdio/conf.js

@@ -10,6 +10,12 @@ exports.config = {
  capabilities: [
    {
      browserName: 'chrome',
+      axe: {
+        inject: true,
+        rules: {


this should probably be

options: { rules: {} }

This allows the global options to be separated from our custom options (e.g. inject). That options object will then be directly passed to axe.configure. See https://github.com/dequelabs/axe-core/blob/develop/doc/API.md#api-name-axeconfigure

updated to place the rules inside an options hash

emilyrohrbough · 2018-01-10T21:01:07Z

src/wdio/conf.js

@@ -51,10 +59,6 @@ exports.config = {
    global.browser.click('#wdioMouseReset');
  },

-  axe: {


From what I understand of webdriver.io, when define a service, the webdriver.io configuration object will passed to that service when the runner starts. From there, since the server has access the configuration object, it can search for its designated config key to pull in its own configuration.

Given this, I feel the axe configuration should be defined with the axe key and moving it to capabilities would be a non-passive change for consumers defining their accessibility configuration.

I did look into the capabilities key. @mhemesath This configuration is being utilized by the selenium grid correct?

@kschuste I do agree in the AxeService.js file below, the defaulted inject: true was unnecessary given we also had it set here in the configuration and it was always overridden with true.

I did look into the capabilities key. @mhemesath This configuration is being utilized by the selenium grid correct?

Yes that should only be for selenium configuration, putting config in there is kinda hijacking that.

I do agree in the AxeService.js file below, the defaulted inject: true was unnecessary given we also had it set here in the configuration and it was always overridden with true.

The inject option I don't think ever worked, it always defaulted to true.

emilyrohrbough · 2018-01-10T21:04:07Z

So... This PR has generated a bit of discussion, but it also got me thinking. I am not sure we should be disabling the landmark rule in the default configuration. Here are my reasons why

Our consumers could be using this to test accessibility of applications, so they will want this rule enabled.
Consumers should knowingly disable accessibility rules
Few people are building and testing modular components.

Given this, we should probably disable the landmark rule from our webdriver.io configuration in each repository.

mhemesath · 2018-01-10T21:10:37Z

I am not sure we should be disabling the landmark rule in the default configuration.

@emilyrohrbough Yeah, it is an optional configuration... as you said should probably be pushed to the toolkit specific wdio config file.

mhemesath · 2018-01-10T21:15:14Z

src/wdio/services/AxeService.js

@@ -33,7 +32,7 @@ export default class AxeService {
      const specifiedViewports = options.viewports || [currentViewportSize];
      const axeOptions = {
        runOnly: options.runOnly,
-        rules: options.rules,
+        rules: Object.assign({}, axeConfig.options.rules, options.rules),


the axeConfig should be set globally using axe.configure when the script is injected.

kschuste · 2018-01-11T22:15:14Z

@emilyrohrbough @mhemesath Updated to move the axe configuration to the toolkit specific wdio file and inject the configuration into the axe script.

mhemesath · 2018-01-12T14:19:55Z

src/wdio/services/AxeService.js

    browser.addCommand('axe', (options = {}) => {
      // Conditionally inject axe. This allows consumers to inject it themselves
      // in the test examples which would slightly speed up test runs.
-      if (axeConfig.inject) {
+      const axeConfig = browser.options.axe;


we should default inject to true, change this to

const axeConfig = { inject: true, ...(browser.options.axe || {}), };

Agreed. Most will expect this to be true.

mhemesath · 2018-01-12T14:20:48Z

Other than defaulting inject: true, my only other comment is to update the service documentation so that others know they can set global axe options now.

emilyrohrbough · 2018-01-12T14:42:28Z

I agree with @mhemesath, default to true and updating docs should be good!

EDIT
~~Side note, I don't think we should be checking in the package.lock file~~
^ Matt says its fine.

mjhenkes · 2018-01-12T15:17:45Z

Checking in package.lock is fine. NPM tells you to do it. (And i made the change to allow it.) :)
a7e901e

kschuste · 2018-01-12T16:14:25Z

updated default to always be true and updated documentation

emilyrohrbough · 2018-01-15T22:56:53Z

src/wdio/services/AxeService.js

+
+            const axeOptions = axeConfig.options;
+            if (axeOptions) {
+              axeCoreSrc = `${axeCoreSrc}\naxe.configure(${JSON.stringify(axeOptions)});`;


${axeCoreSrc}\naxe.configure(${JSON.stringify(axeOptions)});;

Is there a cleaner options than the \n between ${axeCoreSrc} and axe.configure(... ?

is needed to place the json config on a new line to not have syntax errors when loading the script. maybe could break it apart and append each line separately, but I think this is ok as only appending 1 item to the end of the script

we might be able to get away with a newline in the string literal

One option for readability could be

const axeConfigrue = `axe.configure(${JSON.stringify(axeOptions)})`; axeCoreSrc = `${axeCoreSrc}\n${axeConfigure};`;

emilyrohrbough · 2018-01-15T22:57:38Z

docs/AxeService.md

@@ -5,7 +5,8 @@ Terra toolkit automatically includes the wdio-axe-service which enhances a webdr

 Under the key `axe` in the wdio.conf.js you can pass a configuration object with the following structure:

-* **inject** - True if the axe script should be injected by the test running. Disable if axe is already included in the test files, which slightly speed up runs. Defaults to true.


We should keep this documentation as a consumer can still set this to false if they wouldn't want to keep the default true.

this was removed as was always defaulting to true as we were always ignoring the setting so it never worked

with the changes made we are still hardcoding it to default to true so it is working the same; therefore, removed it from the documentation as was never valid.

well we should leave the documentation in place because the option should now actually work

kschuste · 2018-01-16T16:41:48Z

@mhemesath @emilyrohrbough updated to include documentation and readability

updating accessibility tests to be compatible with latest axe release

b70a017

kschuste added the tech-only label Jan 5, 2018

kschuste self-assigned this Jan 5, 2018

Merge branch 'master' into accessibility-testing-updates

e0c84bf

jmsv6d approved these changes Jan 5, 2018

View reviewed changes

Schuster,Kevin added 3 commits January 5, 2018 16:53

making axe rule of landmark-one-main: false to be global for wdio tests

9eab06d

removing the landmark rule from the specific tests

d4c5285

removing undeeded blank line

621dcf3

yuderekyu approved these changes Jan 9, 2018

View reviewed changes

mhemesath reviewed Jan 9, 2018

View reviewed changes

moving axe inject configuration

bdc8575

mhemesath reviewed Jan 10, 2018

View reviewed changes

moving axe rules under options hash, updated param name

4557675

emilyrohrbough reviewed Jan 10, 2018

View reviewed changes

mhemesath reviewed Jan 10, 2018

View reviewed changes

Schuster,Kevin added 3 commits January 11, 2018 15:56

injecting axe configuration into script

29e2c87

fixing linter and added null checks

1ba6bab

fixing unneeded code

0f693b5

mhemesath reviewed Jan 12, 2018

View reviewed changes

defaulting inject to true, updating documentation

fee7398

emilyrohrbough reviewed Jan 15, 2018

View reviewed changes

kschuste and others added 2 commits January 15, 2018 17:17

Merge branch 'master' into accessibility-testing-updates

5458c17

PR review updates

a58ffcb

mhemesath approved these changes Jan 16, 2018

View reviewed changes

emilyrohrbough approved these changes Jan 16, 2018

View reviewed changes

mjhenkes approved these changes Jan 16, 2018

View reviewed changes

kschuste merged commit abe5913 into master Jan 16, 2018

kschuste deleted the accessibility-testing-updates branch January 16, 2018 19:22

This was referenced Jan 30, 2018

Reflect axe-core unlock #52

Merged

Fix Danger File #53

Closed

		@@ -11,7 +11,6 @@ export default class AxeService {
		// eslint-disable-next-line class-methods-use-this
		before(config) {

		@@ -5,7 +5,8 @@ Terra toolkit automatically includes the wdio-axe-service which enhances a webdr

		Under the key `axe` in the wdio.conf.js you can pass a configuration object with the following structure:

		* inject - True if the axe script should be injected by the test running. Disable if axe is already included in the test files, which slightly speed up runs. Defaults to true.

updating accessibility tests to be compatible with latest axe release #42

updating accessibility tests to be compatible with latest axe release #42

Conversation

kschuste commented Jan 5, 2018

Summary

emilyrohrbough commented Jan 5, 2018

emilyrohrbough commented Jan 5, 2018

mhemesath commented Jan 5, 2018

emilyrohrbough commented Jan 5, 2018

mhemesath commented Jan 5, 2018

kschuste commented Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhemesath Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhemesath Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

mhemesath Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhemesath Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhemesath Jan 10, 2018 • edited Loading

Choose a reason for hiding this comment

emilyrohrbough commented Jan 10, 2018 • edited Loading

mhemesath commented Jan 10, 2018

Choose a reason for hiding this comment

kschuste commented Jan 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhemesath commented Jan 12, 2018

emilyrohrbough commented Jan 12, 2018 • edited Loading

mjhenkes commented Jan 12, 2018

kschuste commented Jan 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilyrohrbough Jan 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kschuste commented Jan 16, 2018

mhemesath Jan 10, 2018 •

edited

Loading

mhemesath Jan 10, 2018 •

edited

Loading

mhemesath Jan 10, 2018 •

edited

Loading

mhemesath Jan 10, 2018 •

edited

Loading

mhemesath Jan 10, 2018 •

edited

Loading

emilyrohrbough commented Jan 10, 2018 •

edited

Loading

kschuste commented Jan 11, 2018 •

edited

Loading

emilyrohrbough commented Jan 12, 2018 •

edited

Loading

emilyrohrbough Jan 16, 2018 •

edited

Loading