Support choice of runner(s)

[jsa34]

Just giving a stab at adding caps for axe runners as a config - feedback welcome!

[jsa34]

noticed this approach passes unit tests, works with config file array passing, but trying to start with command line args, i.e. RUNNERS=['axe,'htmlcs'] or any other format. Will close and try and work out how to get this to work

[joeyciechanowicz]

This is a great start. But yes, env('RUNNERS', ['htmlcs', 'axe']) won't work as it will just be the string "['axe','htmlcs']".

To get it working with the appraoch you've gone for I'd suggess simply accepting a CSV list of runners.

function csvToArray(value) {
   const parts = value.split(/,/g)
      .filter(part => part.match(/^\s+$/))
     .map(part => part.trim());
   return parts;   
}

/// later
runners: csvToArray(env('RUNNERS', 'htmlcs,axe'))

[joeyciechanowicz]

I think there will be additional work to display this on the dashboard correctly.

[josebolos]

Yup! There's also a bigger discussion to be had regarding how do we expose this in the dashboard.

When we added axe support to the CLI we decided to make it use only one runner per call, this was to keep the code clean and simple.

However, there's argument to be made for using both runners in the dashboard in order to capture as many issues as possible, so we definitely need to discuss if/how that could happen.

[jsa34]

When I tried running in pa11y-dashboard with config file, it appears to correctly display axe, htmlcs or both axe AND htmlcs issues and collate them in the categories without any necessary code changes. The cli obviously did not work, but as I use the config file it was only by chance I noticed this. It doesn't, other than the error produced itself, label issues as axe or htmlcs, but perhaps the error details are sufficient?

[jsa34]

Added suggestion of a string to array function.
Feedback welcome!

[joeyciechanowicz]

Looking good. We can't (IMO) default to using both htmlcs and axe as that would be a "breaking" change for people who upgrade. Their dashboards will potentially start reporting errors that weren't there previously for them.

[joeyciechanowicz]

You need to use map to project the values from the split array.

Suggested change

	return value.split(',').forEach(item => item.trim());
	return value.split(',').map(item => item.trim());

[joeyciechanowicz]

Would this not need to use possibleCsvListToArray? Additionally we should not default to using both htmlcs and axe as this will break peoples dashboards who upgrade but don't add this configuration.

Suggested change

	runners: env('RUNNERS', ['htmlcs', 'axe']),
	runners: possibleCsvListToArray(env('RUNNERS', 'htmlcs')),

[jasonday]

@jsa34 status of requested changes?

[Ruluk]

Hey there! I just wanted to know if I can be of any help to address the remaining comments and leave this PR ready.

[bconnelly-niche]

Hi! I was wondering if there has been any movement on this PR? Being able to use Axe in the dashboard would be incredible!

[danyalaytekin]

Hi all, will revisit this during 5.x once pa11y-webservice@5 has been released. Potentially some overlap with #144 but I haven't thought too much about this yet, from the point of view of any changes along these lines (a) making sense for the service alone, since we've seen some recent interest in this service without the dashboard, while (b) also making sense for the dashboard.

edit: a third related PR, which has since been closed:

• add runners option for axe support #127

4 years passed