Skip to content

tools: fix exit code when linting from CI - #6412

Closed
mscdex wants to merge 1 commit into
nodejs:masterfrom
mscdex:tools-fix-jslint-ci-exit-code
Closed

tools: fix exit code when linting from CI#6412
mscdex wants to merge 1 commit into
nodejs:masterfrom
mscdex:tools-fix-jslint-ci-exit-code

Conversation

@mscdex

Copy link
Copy Markdown
Contributor
Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)
  • tools
Description of change

Before this, if there were lint errors reported by make jslint-ci, the process would still exit with an exit code of zero.

This commit fixes that to align with make jslint (exit with code 1 on lint errors).

@mscdexmscdex added the tools Issues and PRs related to the tools directory. label Apr 27, 2016
@mscdex

Copy link
Copy Markdown
ContributorAuthor

/cc @jbergstroem

@Trott

Copy link
Copy Markdown
Member

This probably should only land after #6411 lands or the CI lint VM is updated to Node.js 6.0.0.

Before this, if there were lint errors reported by `make jslint-ci`,
the process would still exit with an exit code of zero.
This commit fixes that to align with `make jslint` (exit with
non-zero on lint errors).
@mscdex
mscdexforce-pushed the tools-fix-jslint-ci-exit-code branch from 7052427 to c4d4423CompareApril 27, 2016 06:01
@mscdex

Copy link
Copy Markdown
ContributorAuthor

I've pushed some additional changes to exit early when a worker dies unexpectedly (e.g. in case of a SyntaxError or similar when eslint actually starts linting files inside a worker).

@phillipj

Copy link
Copy Markdown
Member

LGTM

2 similar comments
@jbergstroem

Copy link
Copy Markdown
Member

LGTM

@santigimeno

Copy link
Copy Markdown
Member

LGTM

@jasnell

Copy link
Copy Markdown
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/2413/
LGTM if CI is green

@Trott

Copy link
Copy Markdown
Member

@jasnell CI for this won't pass until #6411 lands (or changes are made to the CI linting VM).

@jasnell

Copy link
Copy Markdown
Member

thanks for the heads up. may want to indicate that in the PR description :-)

@Trott

Copy link
Copy Markdown
Member

@jasnell Additional complication is that requirement is only true on master. v4.x can get this PR right now and it will be just fine.

@Trott

Copy link
Copy Markdown
Member

#6411 landed so this can be run in CI now.

Ci: https://ci.nodejs.org/job/node-test-pull-request/2427/

Trott pushed a commit to Trott/io.js that referenced this pull request Apr 29, 2016
Before this, if there were lint errors reported by `make jslint-ci`,
the process would still exit with an exit code of zero.
This commit fixes that to align with `make jslint` (exit with
non-zero on lint errors).
PR-URL: nodejs#6412
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Phillip Johnsen <johphi@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott

Copy link
Copy Markdown
Member

Landed in 1264cec

@TrottTrott closed this Apr 29, 2016
@mscdex
mscdex deleted the tools-fix-jslint-ci-exit-code branch April 29, 2016 17:44
Fishrock123 pushed a commit that referenced this pull request May 4, 2016
Before this, if there were lint errors reported by `make jslint-ci`,
the process would still exit with an exit code of zero.
This commit fixes that to align with `make jslint` (exit with
non-zero on lint errors).
PR-URL: #6412
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Phillip Johnsen <johphi@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request May 4, 2016
Before this, if there were lint errors reported by `make jslint-ci`,
the process would still exit with an exit code of zero.
This commit fixes that to align with `make jslint` (exit with
non-zero on lint errors).
PR-URL: nodejs#6412
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Phillip Johnsen <johphi@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins

Copy link
Copy Markdown
Contributor

@mscdex safe to assume this would be included with other linter changes if we were to backport?

@mscdex

Copy link
Copy Markdown
ContributorAuthor

@thealphanerd if by 'linter changes' you mean the addition of tools/jslint.js and PRs related to that, then yes.

@MylesBorins

Copy link
Copy Markdown
Contributor

that is what I mean

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

Labels

toolsIssues and PRs related to the tools directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@mscdex@Trott@phillipj@jbergstroem@santigimeno@jasnell@MylesBorins