build: run linter before running tests #16284

joyeecheung · 2017-10-18T05:59:51Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

build, test

Fixes: #16283

joyeecheung · 2017-10-18T06:00:40Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10807/

joyeecheung · 2017-10-18T06:13:27Z

cc @nodejs/testing

Trott · 2017-10-18T06:37:28Z

A bit of probably-relevant back-story: I made this change over a year ago in d9f7a59 and people hated it enough that I felt compelled to revert it in #5602.

I like it, though. +1 from me!

Trott

LGTM. Maybe the thing to do to make everyone happy is put the linter into a test file so it can run in parallel with other tests. :-D I'm just joking. OR AM I?!?!

Trott · 2017-10-18T06:41:16Z

Another option, if people do object to this, might be to make it somehow configurable or have a task separate from make test that has this ordering.

gibfahn · 2017-10-18T07:41:55Z

Great idea! Should make things easier for new contributors too (number of people running make test and finding linter errors at the end at code-and-learn was large).

joyeecheung · 2017-10-18T07:52:47Z

Looking at #5470 cc @bnoordhuis @Fishrock123

I am thinking maybe we can have two targets

make test-only for running tests (better for people who are familiar with our process)
make test for running lint + tests (better for new contributors)

Doesn't have to be these two, any suggestions are welcomed. (I wouldn't even object to make t because hey, less typing)

refack · 2017-10-18T11:33:21Z

LGTM. Maybe the thing to do to make everyone happy is put the linter into a test file so it can run in parallel with other tests. :-D I'm just joking. OR AM I?!?!

I like the "linter as test" idea (or some other way to make lint not stop the rest of the make), since it will allow to get both outputs with a single run.

gibfahn · 2017-10-18T12:22:07Z

I am thinking maybe we can have two targets

make test-only for running tests (better for people who are familiar with our process)

make test for running lint + tests (better for new contributors)

Something like this seems like it would resolve @bnoordhuis 's objection to the previous PR, while making new contributor's lives easier.

(I wouldn't even object to make t because hey, less typing)

I like make t, it's not obvious, but then this isn't something people getting started are likely to need.

jasnell · 2017-10-18T15:30:23Z

Another possible color for the bikeshed: A ./configure --lint-first type of option may be worthwhile... that is, some configuration option that would instruct the Makefile to run linting first.

joyeecheung · 2017-10-18T16:00:43Z

@jasnell That works as well but I think the default should be lint-first instead? Also this could require some hack because the main Makefile is not actually generated...

mhdawson

LGTM

mhdawson · 2017-10-18T21:57:30Z

How about "test-no-lint" for the target that does everything except for the linter ? But no strong feeling, just another suggestion to consider.

apapirovski

LGTM

The make t & make test suggestion seems good to me, FWIW.

joyeecheung · 2017-10-19T05:08:30Z

Rebased and implemented make t for people who just want a quick check. @apapirovski @refack @Trott @lpinca @BridgeAR @mhdawson @gibfahn Can people who approved before take another look please? Thanks!

Also @bnoordhuis do you still object to this idea? Although we can always revert if we hate it ¯\(ツ)/¯

CI: https://ci.nodejs.org/job/node-test-pull-request/10845/

gibfahn · 2017-10-19T08:50:57Z

Makefile

 endif

+# For a quick test, does not run linter or build doc
+t: all


Does t need to be added to .PHONY?

richardlau · 2017-10-19T09:54:57Z

Makefile

 endif

+# For a quick test, does not run linter or build doc
+t: all


t is a terrible name for a make target (that what it does has to be described in a comment is a good indicator that it is badly named). We're already at 40+ phony targets -- It should be in our interest that the target names be reasonably self descriptive.

But this isn't something most people will want to use. It looks like a shortcut, which is what it is.

I would prefer test-only, personally... I think anyways. Maybe not.
¯\_(ツ)_/¯

tniessen

Basically LGTM, but I would prefer not to use single-character target names.

mhdawson

LGTM. I'd prefer a more descriptive name but don't want to bikeshed so I'm ok with that everybody else agrees to.

joyeecheung · 2017-10-19T17:45:01Z

@richardlau @tniessen @mhdawson Yeah...took the name because it's meant to be a shortcut, couldn't think of anything better at that point. How about make check?

joyeecheung · 2017-10-29T02:50:47Z

Landed in 2875459, thanks!

PR-URL: #16284 Fixes: https://github.com/node/issues/16283 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

PR-URL: nodejs/node#16284 Fixes: https://github.com/node/issues/16283 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

MylesBorins · 2017-11-16T21:26:06Z

This doesn't land cleanly on v6.x-staging can someone manually backport?

PR-URL: nodejs/node#16284 Fixes: https://github.com/node/issues/16283 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 18, 2017

Trott approved these changes Oct 18, 2017

View reviewed changes

gibfahn approved these changes Oct 18, 2017

View reviewed changes

vsemozhetbyt added the tools Issues and PRs related to the tools directory. label Oct 18, 2017

refack approved these changes Oct 18, 2017

View reviewed changes

lpinca approved these changes Oct 18, 2017

View reviewed changes

mhdawson approved these changes Oct 18, 2017

View reviewed changes

BridgeAR approved these changes Oct 18, 2017

View reviewed changes

trivikr approved these changes Oct 19, 2017

View reviewed changes

apapirovski approved these changes Oct 19, 2017

View reviewed changes

joyeecheung force-pushed the run-linter-first branch from 19b2223 to ddec5d5 Compare October 19, 2017 05:01

BridgeAR approved these changes Oct 19, 2017

View reviewed changes

gibfahn approved these changes Oct 19, 2017

View reviewed changes

gibfahn reviewed Oct 19, 2017

View reviewed changes

Makefile Outdated

endif

# For a quick test, does not run linter or build doc

t: all

Copy link

Member

gibfahn Oct 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does t need to be added to .PHONY?

richardlau reviewed Oct 19, 2017

View reviewed changes

tniessen approved these changes Oct 19, 2017

View reviewed changes

mhdawson approved these changes Oct 19, 2017

View reviewed changes

richardlau approved these changes Oct 28, 2017

View reviewed changes

joyeecheung closed this Oct 29, 2017

joyeecheung mentioned this pull request Oct 29, 2017

tools: don't lint files that have not changed #16581

Closed

2 tasks

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

gibfahn mentioned this pull request Oct 31, 2017

v8.9.0 proposal #16630

Merged

MylesBorins added the backport-requested-v6.x label Nov 16, 2017

joyeecheung self-assigned this Nov 17, 2017

joyeecheung added dont-land-on-v6.x and removed backport-requested-v6.x labels Jan 24, 2018

joyeecheung added dont-land-on-v6.x and removed backport-requested-v6.x labels Mar 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: run linter before running tests #16284

build: run linter before running tests #16284

joyeecheung commented Oct 18, 2017

joyeecheung commented Oct 18, 2017

joyeecheung commented Oct 18, 2017

Trott commented Oct 18, 2017

Trott left a comment

Trott commented Oct 18, 2017

gibfahn commented Oct 18, 2017

joyeecheung commented Oct 18, 2017

refack commented Oct 18, 2017

gibfahn commented Oct 18, 2017

jasnell commented Oct 18, 2017

joyeecheung commented Oct 18, 2017

mhdawson left a comment

mhdawson commented Oct 18, 2017

apapirovski left a comment

joyeecheung commented Oct 19, 2017 •

edited

Loading

gibfahn Oct 19, 2017

richardlau Oct 19, 2017

gibfahn Oct 19, 2017

Fishrock123 Oct 20, 2017 •

edited

Loading

tniessen left a comment

mhdawson left a comment

joyeecheung commented Oct 19, 2017 •

edited

Loading

joyeecheung commented Oct 29, 2017

MylesBorins commented Nov 16, 2017

build: run linter before running tests #16284

build: run linter before running tests #16284

Conversation

joyeecheung commented Oct 18, 2017

Checklist

Affected core subsystem(s)

joyeecheung commented Oct 18, 2017

joyeecheung commented Oct 18, 2017

Trott commented Oct 18, 2017

Trott left a comment

Choose a reason for hiding this comment

Trott commented Oct 18, 2017

gibfahn commented Oct 18, 2017

joyeecheung commented Oct 18, 2017

refack commented Oct 18, 2017

gibfahn commented Oct 18, 2017

jasnell commented Oct 18, 2017

joyeecheung commented Oct 18, 2017

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Oct 18, 2017

apapirovski left a comment

Choose a reason for hiding this comment

joyeecheung commented Oct 19, 2017 • edited Loading

gibfahn Oct 19, 2017

Choose a reason for hiding this comment

richardlau Oct 19, 2017

Choose a reason for hiding this comment

gibfahn Oct 19, 2017

Choose a reason for hiding this comment

Fishrock123 Oct 20, 2017 • edited Loading

Choose a reason for hiding this comment

tniessen left a comment

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

joyeecheung commented Oct 19, 2017 • edited Loading

joyeecheung commented Oct 29, 2017

MylesBorins commented Nov 16, 2017

joyeecheung commented Oct 19, 2017 •

edited

Loading

Fishrock123 Oct 20, 2017 •

edited

Loading

joyeecheung commented Oct 19, 2017 •

edited

Loading