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

fix(run,finishRun): don't mutate options, set default reporter to v1 #3088

Merged
merged 3 commits into from
Jul 23, 2021

Conversation

straker
Copy link
Contributor

@straker straker commented Jul 20, 2021

This fixes an issue where not passing an options object to axe.finishRun would result in axe not using the v1 reporter even though the v1 reporter is the default for a axe.run. This caused the results of the axe.finishRun to not include failureSummary.

const results = await axe.runPartial()
await axe.finishRun([results])  // v1 reporter not used

The better question would be why we set the v2 reporter as default when axe.run uses the v1 by default.

@straker straker requested a review from a team as a code owner July 20, 2021 19:53
@straker
Copy link
Contributor Author

straker commented Jul 20, 2021

I'm not sure what we should do at this point. The test is failing because we need to mutate the options object to pass the normalized run options from partialRun to finishRun in order for the results of an axe.run and axe.finishRun to be the same.

The only thing I can think of is that we don't use the modified options object in the reporter but instead try to use the original options object the user uses for toolOptions. Then it will show exactly what the user used to run the results rather than our modified and normalized options.

const runResults = await axe.run({ runOnly: 'region' });
console.log(runResults.toolOptions.runOnly) //  { type: 'rule', values: ['region'] }

const partialResults = await axe.runPartial({ runOnly: 'region' });
const partialRunResults = await axe.finishRun({ runOnly: 'region' });
console.log(partialRunResults.toolOptions.runOnly) //  'region'

Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

LGTM, but can you also get rid of the use of clone in testUtils.runPartialRecursive. We're not going to need it there anymore now that we have this in.

@straker straker merged commit 90f0b27 into develop Jul 23, 2021
@straker straker deleted the no-option-mutation branch July 23, 2021 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants