Skip to content

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

Merged
straker merged 3 commits intodevelopfrom
no-option-mutation
Jul 23, 2021
Merged

fix(run,finishRun): don't mutate options, set default reporter to v1#3088
straker merged 3 commits intodevelopfrom
no-option-mutation

Conversation

@straker
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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