Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support choice of runner(s) #105

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

jsa34
Copy link
Contributor

@jsa34 jsa34 commented Oct 4, 2019

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

@jsa34
Copy link
Contributor Author

jsa34 commented Oct 7, 2019

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

@jsa34 jsa34 closed this Oct 7, 2019
@joeyciechanowicz
Copy link
Member

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
Copy link
Member

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

@josebolos
Copy link
Member

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
Copy link
Contributor Author

jsa34 commented Oct 7, 2019

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?
Screenshot 2019-10-07 at 10 45 49

@jsa34 jsa34 changed the title Start adding axe caps - feedback welcome [WIP] Start adding axe caps - feedback welcome! Oct 7, 2019
@jsa34
Copy link
Contributor Author

jsa34 commented Oct 7, 2019

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

@jsa34 jsa34 reopened this Oct 7, 2019
Copy link
Member

@joeyciechanowicz joeyciechanowicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (Array.isArray(value)) {
return value;
} else if (typeof value === 'string') {
return value.split(',').forEach(item => item.trim());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());

@@ -32,6 +33,7 @@ if (fs.existsSync(jsonPath)) {
database: env('DATABASE', 'mongodb://localhost/pa11y-webservice'),
host: env('HOST', '0.0.0.0'),
port: Number(env('PORT', '3000')),
runners: env('RUNNERS', ['htmlcs', 'axe']),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

@jsa34 status of requested changes?

@Ruluk
Copy link

Ruluk commented May 18, 2020

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
Copy link

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
Copy link
Member

danyalaytekin commented Mar 17, 2024

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:

@danyalaytekin danyalaytekin added this to the 5.x milestone Mar 17, 2024
@danyalaytekin danyalaytekin changed the title [WIP] Start adding axe caps - feedback welcome! Support choice of runner(s) Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants