build: run linter before running tests by joyeecheung · Pull Request #16284 · nodejs/node · GitHub

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

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

cc @nodejs/testing

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!

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?!?!

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.

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).

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)

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.

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.

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.

@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...

LGTM

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

LGTM

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

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/

Does t need to be added to .PHONY?

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.
¯\_(ツ)_/¯

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

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

@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?

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>

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

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

build: run linter before running tests by joyeecheung · Pull Request #16284 · nodejs/node · GitHub

Uh oh!

Uh oh!

build: run linter before running tests - #16284

build: run linter before running tests#16284joyeecheung wants to merge 1 commit intonodejs:masternodejs/node:masterfrom joyeecheung:run-linter-firstjoyeecheung/node:run-linter-firstCopy head branch name to clipboard

Conversation

joyeecheung commented Oct 18, 2017

Checklist

Affected core subsystem(s)

Uh oh!

joyeecheung commented Oct 18, 2017

Uh oh!

joyeecheung commented Oct 18, 2017

Uh oh!

Trott commented Oct 18, 2017

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Oct 18, 2017

Uh oh!

gibfahn commented Oct 18, 2017

Uh oh!

joyeecheung commented Oct 18, 2017

Uh oh!

refack commented Oct 18, 2017

Uh oh!

gibfahn commented Oct 18, 2017

Uh oh!

jasnell commented Oct 18, 2017

Uh oh!

joyeecheung commented Oct 18, 2017

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Oct 18, 2017

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Oct 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gibfahn Oct 19, 2017

Choose a reason for hiding this comment

Uh oh!

richardlau Oct 19, 2017

Choose a reason for hiding this comment

Uh oh!

gibfahn Oct 19, 2017

Choose a reason for hiding this comment

Uh oh!

Fishrock123 Oct 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Oct 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Oct 29, 2017

Uh oh!

MylesBorins commented Nov 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

17 participants

build: run linter before running tests#16284
joyeecheung wants to merge 1 commit into
nodejs:masterfrom
joyeecheung:run-linter-first

joyeecheung commented Oct 19, 2017 •
edited
Loading

Fishrock123 Oct 20, 2017 •
edited
Loading

joyeecheung commented Oct 19, 2017 •
edited
Loading