fix(cli): provide a more robust error message if analysis fails

[Zidious]

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

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

Closes issue: #207

[stephenmathieson]

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

[Zidious]

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

Including --show-errors flag does not either:

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

[stephenmathieson]

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

[stephenmathieson]

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 🤷

[Zidious]

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)

[stephenmathieson]

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

[stephenmathieson]

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

[Zidious]

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

[stephenmathieson]

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

[Zidious]

For reference issue has been raised: #422

[WilcoFiers]

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

resolved