build: run linter before running tests #16284
Conversation
nodejs-github-bot
added
the
build
|
cc @nodejs/testing |
|
Another option, if people do object to this, might be to make it somehow configurable or have a task separate from |
|
Great idea! Should make things easier for new contributors too (number of people running |
|
Looking at #5470 cc @bnoordhuis @Fishrock123 I am thinking maybe we can have two targets
Doesn't have to be these two, any suggestions are welcomed. (I wouldn't even object to |
I like the "linter as test" idea (or some other way to make |
Something like this seems like it would resolve @bnoordhuis 's objection to the previous PR, while making new contributor's lives easier.
I like |
|
Another possible color for the bikeshed: A |
|
@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... |
|
How about "test-no-lint" for the target that does everything except for the linter ? But no strong feeling, just another suggestion to consider. |
joyeecheung
force-pushed
the
run-linter-first
branch
from
19b2223 to
ddec5d5
Compare
|
Rebased and implemented Also @bnoordhuis do you still object to this idea? Although we can always revert if we hate it ¯\(ツ)/¯ |
Makefile
Outdated
| endif | ||
|
|
||
| # For a quick test, does not run linter or build doc | ||
| t: all |
There was a problem hiding this comment.
Does t need to be added to .PHONY?
Makefile
Outdated
| endif | ||
|
|
||
| # For a quick test, does not run linter or build doc | ||
| t: all |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
But this isn't something most people will want to use. It looks like a shortcut, which is what it is.
There was a problem hiding this comment.
I would prefer test-only, personally... I think anyways. Maybe not.
¯\_(ツ)_/¯
|
@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 |
|
Landed in 2875459, thanks! |
PR-URL: #16284 Fixes: https://github.com/node/issues/16283 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Richard Lau <[email protected]>
PR-URL: #16284 Fixes: https://github.com/node/issues/16283 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Richard Lau <[email protected]>
PR-URL: #16284 Fixes: https://github.com/node/issues/16283 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Richard Lau <[email protected]>
PR-URL: #16284 Fixes: https://github.com/node/issues/16283 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Richard Lau <[email protected]>
PR-URL: nodejs/node#16284 Fixes: https://github.com/node/issues/16283 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Richard Lau <[email protected]>
PR-URL: nodejs/node#16284 Fixes: https://github.com/node/issues/16283 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Richard Lau <[email protected]>
|
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 <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
build, test
Fixes: #16283