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(cli): provide a more robust error message if analysis fails #421

Merged
merged 5 commits into from
Jan 11, 2022

Conversation

Zidious
Copy link
Contributor

@Zidious Zidious commented Jan 10, 2022

The use of --include and providing an non-existent or invalid CSS selector would not notify user that was the case:

image
image

Providing an non-existent or invalid CSS selector will now notify the user of said error:

image

Closes issue: #207

@Zidious Zidious changed the title fix(cli): --include and --exclude to throw error on invalid CSS selectors fix(cli): --include and --exclude to throw error on invalid / not found CSS selectors Jan 10, 2022
@Zidious Zidious marked this pull request as ready for review January 10, 2022 18:00
@Zidious Zidious requested a review from a team January 10, 2022 18:00
@@ -91,7 +92,8 @@ const testPages = async (

/* istanbul ignore if */
if (err) {
return reject(err);
console.error(error(err));
process.exit(1);
Copy link
Member

Choose a reason for hiding this comment

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

Why doesn't reject(err) allow the caller to handle the error on their own? I don't understand how/why this fixes anything.

Copy link
Contributor Author

@Zidious Zidious Jan 10, 2022

Choose a reason for hiding this comment

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

reject(err) doesn't provide a useful error message to the user:

image

Including --show-errors flag does not either:

image

if we handle the error within console.error(..) we get a much better error message:

image

Copy link
Member

Choose a reason for hiding this comment

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

That seems like the problem is how we're handing rejections then.

Copy link
Member

@stephenmathieson stephenmathieson Jan 10, 2022

Choose a reason for hiding this comment

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

It looks like the issue is here:

https://github.com/dequelabs/axe-core-npm/blob/v4.3.2/packages/cli/src/bin/index.ts#L153

What is %j \n $s supposed to do? I don't see %j listed in the Console API's substitution strings, and $s isn't a thing.

Instead, that should probably be:

    if (!showErrors) {
      console.error(error('An error occurred while testing this page.'));
    } else {
      console.error(error('Error: %s \n %s'), e.message, e.stack);
    }

Which will still look weird, since the stack trace is weirdly indented (and on the next line), but that isn't what the linked issue is about 🤷

Copy link
Contributor Author

@Zidious Zidious Jan 10, 2022

Choose a reason for hiding this comment

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

I think the error is here https://github.com/dequelabs/axe-core-npm/blob/v4.3.2/packages/cli/src/lib/axe-test-urls.ts#L87 we use err: string so we don't have access to e.message or e.stack to do the the string formatting you mentioned

edit: %jJSON. Replaced with the string '[Circular]' if the argument contains circular references. (source)

Copy link
Member

Choose a reason for hiding this comment

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

If the types on the line you linked are correct (err: string), then we should do something like:

        axe.analyze((err: Error | string | null, results: AxeResults) => {
          if (config.timer) {
            events?.endTimer('axe-core execution time');
          }

          /* istanbul ignore if */
          if (err) {
            const error = typeof err === 'string' ? new Error(err) : err
            return reject(error);
          }

Copy link
Member

Choose a reason for hiding this comment

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

It looks like that type is wrong, as we pass back the whole Error (unless in legacy/callback mode).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya, looking at it a bit more, the type for the callback function should be Error | null and not string | null. So we can access e.message etc. However, your work around does allow the user to handle the error with --show-errors

image

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the type for that function is wrong. IMO it should be changed/corrected.

However, if we want a very simple fix for the issue at hand, we could just fix the console.error() call in src/bin/index.ts:

    if (!showErrors) {
      console.error(error('An error occurred while testing this page.'));
    } else {
      console.error(error('Error: %s'), e);
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For reference issue has been raised: #422

packages/cli/src/lib/axe-test-urls.ts Outdated Show resolved Hide resolved
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.

While I support the change, I don't see how the title describes what you changed, or how this closes the issue.

@Zidious Zidious changed the title fix(cli): --include and --exclude to throw error on invalid / not found CSS selectors fix(cli): provide more robust error message if analysis fails Jan 11, 2022
@Zidious Zidious requested a review from a team January 11, 2022 13:12
@Zidious Zidious changed the title fix(cli): provide more robust error message if analysis fails fix(cli): provide a more robust error message if analysis fails Jan 11, 2022
@Zidious Zidious merged commit 9f1fa5d into develop Jan 11, 2022
@Zidious Zidious deleted the fix/cli-include branch January 11, 2022 16:17
stephenmathieson added a commit that referenced this pull request Jan 25, 2022
* develop:
  docs(webdriverjs): add example using axe-core webdriverjs (#420)
  fix(cli): provide a more robust error message if analysis fails (#421)
stephenmathieson added a commit that referenced this pull request Jan 25, 2022
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.

3 participants