fix(exit-code): Exit with status 1 when there are deprecated rules

[randycoulman]

In #286, I added support for reporting on deprecated rules. According to the documentation and tests I wrote, eslint-find-rules is supposed to exit with code 1 when there are deprecated rules (unless the --no-error flag is also provided). However, in real usage, this didn't happen.

I discovered that the exit status assertions in the tests were not actually being called in all cases, resulting in the tests passing incorrectly. Looks like I forgot to make sure that the test failed for the right reason in this case. Mea culpa :-(.

I made the following changes:

• Modify the tests to always capture the process exit code
• Add assertions against the captured exit code where necessary
• Re-work how processExitCode is managed in the code

[ljharb]

I guess I’m confused by this line - why do we need this list? Any option could error and should exit nonzero in that case, no?

[randycoulman]

If I understand correctly, the other options are informative only: show me my current rules, rules exposed by a plugin, etc. In those cases, having rules to output isn't really an error.

The way the code was before I fixed this bug, only getUnusedRules would fail with an exit code of 1.

[ljharb]

Hmm. I guess I’d assume that each option should be able to determine its own success or failure state; but here it seems like the overarching runner is doing that?

In that case, maybe this is fine for now, but it should probably be rearchitechted later.

[randycoulman]

I'd argue that there should be process.exit(1) on this line as well: https://github.com/sarbbottam/eslint-find-rules/blob/master/src/bin/find.js#L44

but I thought that might be out-of-scope for this PR.

[randycoulman]

@ljharb Yeah, that does make more sense. I think we'd have to change what the various rule finder methods return, which would be a breaking change if anyone is using the rule finder methods programmatically.

[ta2edchimp]

Hey there,
is there anything left that stops this review from being approved? It looks good to me.

I totally agree that the determination about success or failure of the options' runners should be thought over and eventually reworked (as well as exiting with an exit code of 1 when no option has been provided), but I also agree that this is out of scope for this PR that should only fix the result of looking for deprecated rules.

[ljharb]

If you want to approve it, please do; I intentionally opened this as a non-blocking review :-)

[ta2edchimp]

I didn't realize that 🙈

[ta2edchimp]

Hmm ... @randycoulman could you please rebase on master? I am not able to push the merged master after rebasing the pr on it. Unless @ljharb has another idea — I basically wanted to ff merge the pr-branch, but of course the rebase altered the commit's hash, so it's unknown to github, thus being rejected.
Maybe I'm just not seeing something here ... will get back at it tomorrow, possibly it's already simply too late for me and the brain of mine for today ...

[ta2edchimp]

I feel dumb now ... may as well just press »rebase and merge« on the web ui.
How do we want to handle this in the future?

[ljharb]

@ta2edchimp you can rebase and force push onto the fork. i'll take care of it.

[ljharb]

done

[ta2edchimp]

Thanks a lot!
I assumed to be lacking write permission to a fork, any fork. That's interesting.
I'll do the rest tomorrow, as well as preparing a release / raising the usual PR to bump the version.

[ljharb]

Did you clone using https or ssh? I’ve found that only using ssh solves lots of issues.

[ta2edchimp]

SSH. I have to admit, I never even tried to push to a fork / to a branch over at a fork during a PR because I would have assumed write permissions to be optionally per default.