davglass / license-checker

Check NPM package licenses
Other
1.59k stars 215 forks source link

Replace `process.exit()` with throwing an error #233

Open evantahler opened 4 years ago

evantahler commented 4 years ago

Using process.exit() does not allow us to use this tool programmatically, as we cannot then handle the error in a custom way (ie: formatting the response for CI, logging it somewhere, etc).

This PR replaces console.error + using process.exit with throwing an error. This will have the same effect in CLI usage by immediately terminating the process with a message, but can also be try/catch handled in a parent program.

A downside might be that the error output is less terse... but it will be more noticeable!

evantahler commented 3 years ago

Can you give me a hint where to look? The test seem to be OK on this PR...

mojoaxel commented 3 years ago

@evantahler see e.g https://github.com/davglass/license-checker/blob/master/tests/failOn-test.js#L11-14 and https://github.com/davglass/license-checker/blob/master/tests/test.js#L215

evantahler commented 3 years ago

I imagine those tests are still passing because a throw that isn't caught will exit the process with 1. So... perhaps this is OK?

mojoaxel commented 3 years ago

I imagine those tests are still passing because a throw that isn't caught will exit the process with 1. So... perhaps this is OK?

I guess you are right, at least for the "bin/license-checker" tests. The parseAndFailOn function should be adopted to the new error handling IMHO.

evantahler commented 3 years ago

I'm sorry, but I still don't think I follow what you are asking for.

I'm looking at this example:

    describe('should exit on given list of onlyAllow licenses', function() {
        var result={};
        before(parseAndFailOn('onlyAllow', '../', "MIT; ISC", result));

        it('should exit on non MIT and ISC licensed modules from results', function() {
            assert.equal(result.exitCode, 1);
        });
    });

from (https://github.com/davglass/license-checker/blob/master/tests/test.js#L233-L240)

The test runs parseAndFailOn() and then asserts that the process exited with code 1 when being passed those license names - and it still does.

evantahler commented 3 years ago

OH! the test suite is not passing with these changes, even though travis reports success. I see the problem now :D

mojoaxel commented 3 years ago

The function parseAndFailOn explicitly checks if process.exit is called. IMHO this function should be changed to check if an error is thrown instead:

https://github.com/davglass/license-checker/blob/master/tests/test.js#L215-L231