dequelabs / axe-cli

[Deprecated] A command-line interface for the aXe accessibility testing engine
Mozilla Public License 2.0
430 stars 44 forks source link

Improved exit codes #20

Closed tay1orjones closed 7 years ago

tay1orjones commented 7 years ago

Currently the node process is not exited properly via process.exit().

My thinking is ideally:

There's probably some other cases that an error exit code would apply?

Ultimately for my use case, when axe-cli is integrated into CI tooling such as Travis CI, these incorrect exit codes result in the build "passing" checks/tests, when it should instead fail.

WilcoFiers commented 7 years ago

I think this is an excellent suggestion. If you've got the time, feel free to create a PR for this. If not I think we'll try to make this change ourselves in due time.

jaimeiniesta commented 7 years ago

With this change, will we still be able to tell if axe-cli exited abnormally?

In our integration we use it like this:

axe http://example.com --save report.json && cat report.json

Well, instead of running cat we send the saved JSON via POST to a callback url, but you get the idea: what we expect is that axe-cli runs the report, and saves the results in that JSON file, both for a page with A11Y issues, or without any A11Y issue found. In that case, the saved JSON report will say "0 issues", that's all.

So I think that the exit code from axe-cli should reflect if it was able to complete successfully (exit code 0) or crashed (exit code 1), and not deal with the outcome of the A11Y checks.

tay1orjones commented 7 years ago

@jaimeiniesta IMO that's a substandard behavior, although your use case should still be supported.

I suggest adding an optional flag to disable the default behavior. So, If you wish to see a list of errors and not have the CLI exit, use the -q or --no-exit flag.

dylanb commented 7 years ago

I am not sure this should be the default behavior - the idea behind axe-cli is that it can be run to generate a list of issues. If it runs successfully and is able to do this, then the behavior is as expected. Also, defaulting to this would preclude the ability to run multiple axe-cli commands one after the other.

So if you implement this, I would like to see it behind a flag like axe-core --exit or something appropriate

tay1orjones commented 7 years ago

That makes sense. I'll take a stab at a PR today with failing a11y tests exit(1) behind a flag.

There are three other potential areas for failure:

  1. No URL specified (:40)
  2. Output to file fails (:99)
  3. Any other error surfaced at the end of the promise chain (:117)

Should these actions exit(1) regardless of the flag?

tay1orjones commented 7 years ago

@dylanb PR Isssued 👍