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

Handle non-integer error codes as exit code 1 (#2115) #2140

Merged
merged 1 commit into from Dec 1, 2016

Conversation

5 participants
@nkabardin
Contributor

nkabardin commented Nov 28, 2016

Fixes #2115

When there is an error in require() in configuration file, cli gets an error code which is not actually integer, so passing it to process.exit is not a good idea.

Now value is getting checked if it's a number or not, and if it's not a number, it's replaced with plain 1.

System test added that runs cli in ensures that exit code != 0

@davidtheclark

Nice work @nkabardin! 👍

@nkabardin

This comment has been minimized.

Contributor

nkabardin commented Nov 28, 2016

@davidtheclark thanks!

something is frozen on ci though. Can't understand what's happening.

@nkabardin

This comment has been minimized.

Contributor

nkabardin commented Nov 29, 2016

Checked log on Travis, and there might be issues with running this system test on jenkins — any ideas what's happening there?

@ntwb

This comment has been minimized.

Member

ntwb commented Nov 29, 2016

@nkabardin I've restarted the CI tests so lets see what happens...

@ntwb

This comment has been minimized.

Member

ntwb commented Nov 29, 2016

Coveralls is failing in both 5569.1 and 5569.2 jobs even though #5569.1 is reporting as passed and #5569.2 is failing with a Jest test timing out.

@nkabardin

This comment has been minimized.

Contributor

nkabardin commented Nov 29, 2016

So I guess it's something related to child_process.spawn, are there any CI-related limitations for that?

@nkabardin

This comment has been minimized.

Contributor

nkabardin commented Nov 29, 2016

Playing around with error logging in system test, let's see what CI will tell us...

@nkabardin nkabardin force-pushed the nkabardin:2115-fix-cli-exit-code branch 2 times, most recently from 2d5400b to b05bef1 Nov 29, 2016

@nkabardin

This comment has been minimized.

Contributor

nkabardin commented Nov 29, 2016

did several attempts trying to log out the problems with running stuff, added three console.log to test, and didn't catch the problem, nothing logged.

Any ideas?

@nkabardin nkabardin force-pushed the nkabardin:2115-fix-cli-exit-code branch 2 times, most recently from efae32d to 02afd14 Nov 29, 2016

@nkabardin

This comment has been minimized.

Contributor

nkabardin commented Nov 30, 2016

Rebased branch over current master (so, support babel drop and no need to use babel-node to run cli) and also and removed all debug stuff and it's currently ready and well tested, and surprisingly issues on CI are also solved, so we got green build there, so yay!

@ntwb

This comment has been minimized.

Member

ntwb commented Nov 30, 2016

LGTM, though let's get another review from @davidtheclark 👍

Edit: Coveralls coverage is pretty volatile at the moment with the "drop babel" changes, we can redress any coverage issues in follow up PRs

@ntwb

This comment has been minimized.

Member

ntwb commented Dec 1, 2016

Coverage tests were failing if less than 95%, I've dropped that value to 90%

So ignore the Coveralls failure here in this PR (I couldn't find a way to restart the job)

@davidtheclark

One comment, then let's merge.

const localPath = path.resolve(__dirname)
const cliPath = path.join(localPath, "../../lib/cli.js")
const cliProcess = spawn("node", [ cliPath, `${localPath}/*.css` ], { cwd: localPath })

This comment has been minimized.

@davidtheclark

davidtheclark Dec 1, 2016

Contributor

Could you add an 'error' listener to this process?

This comment has been minimized.

@nkabardin

nkabardin Dec 1, 2016

Contributor

done

@nkabardin nkabardin force-pushed the nkabardin:2115-fix-cli-exit-code branch from 02afd14 to 006e775 Dec 1, 2016

@nkabardin nkabardin force-pushed the nkabardin:2115-fix-cli-exit-code branch from 006e775 to e606bbb Dec 1, 2016

@jeddy3

jeddy3 approved these changes Dec 1, 2016

const cliProcess = spawn("node", [ cliPath, `${localPath}/*.css` ], { cwd: localPath })
cliProcess.on("error", function (error) {
console.log("error running cli:", error) // eslint-disable-line no-console

This comment has been minimized.

@evilebottnawi

evilebottnawi Dec 1, 2016

Member

I think we should have failed test if have error in error event

This comment has been minimized.

@nkabardin

nkabardin Dec 1, 2016

Contributor

it fails anyway if there is an error, it just added logging for that.
Couldn't reproduce a case when it didn't fail on error

This comment has been minimized.

@evilebottnawi

evilebottnawi Dec 1, 2016

Member

@nkabardin we can use mock for reproduce error, we got non zero code (thoretically 😄 ) and test failed, lgtm

This comment has been minimized.

@nkabardin

nkabardin Dec 1, 2016

Contributor

I didn't get it, current code looks good to you, or it will look good to you when some mocking will be added? :)

This comment has been minimized.

@evilebottnawi
@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Dec 1, 2016

Thanks again @nkabardin!

@davidtheclark davidtheclark merged commit 987b800 into stylelint:master Dec 1, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 91.891%
Details
@davidtheclark

This comment has been minimized.

Contributor

davidtheclark commented Dec 1, 2016

Changelog entry

  • Fixed: bug where some Node errors in special cases did not cause the CLI to exit with a non-zero code (#2140)

@nkabardin nkabardin deleted the nkabardin:2115-fix-cli-exit-code branch Dec 1, 2016

@nkabardin

This comment has been minimized.

Contributor

nkabardin commented Dec 1, 2016

@davidtheclark glad I was able to help the project! thanks for stylelint ❤️

sergesemashko pushed a commit to sergesemashko/stylelint that referenced this pull request Mar 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment