build: run lint before tests

[Trott]

Have make test run linting tools before tests rather than after. Lint
is likely to find issues quickly. Tests may take a while to run. So do
the linting first.

Interestingly, it appears that vcbuild.bat is already set up this way
on Windows.

Refs: #4546 (comment)

[jbergstroem]

LGTM

[ronkorving]

Thanks for this change. LGTM.

[MylesBorins]

LGTM

[evanlucas]

LGTM

[joaocgreis]

https://github.com/nodejs/node/blob/3c79bbd/vcbuild.bat#L249-L257 vcbuild.bat runs cctest, then test.py and only then jslint. It does not run cpplint. I'm ok with landing this and leaving vcbuild.bat for later, since the change here only makes it fail faster if it was already going to fail.

[Trott]

Ah! I see I misunderstood the vcbuild.bat. I'll land this but open a ticket (with a good-first-contribution label!) to do the same thing in vcbuild.bat. This way, someone with easy access to a Windows machine can do it. And maybe even someone new.

[Trott]

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

[Trott]

Landed in d9f7a59

[bnoordhuis]

I really, really hate this change; it messes up my workflow. I'd like to move to revert it.

[Fishrock123]

I would like them to run independently. If one fails, still run the other. Not sure if that is possible in make or not.

[Trott]

@bnoordhuis wrote:

I really, really hate this change; it messes up my workflow. I'd like to move to revert it.

I don't have a problem with undoing this change, but I am curious how this messes up your workflow. Knowing that might help keep me or others from making similarly disruptive changes in the future.

[Fishrock123]

@Trott lots of us only follow the style as a finalization. No way I'm semicoloning code I'm trying to write/test.

I also don't really have this problem because I use a ninja alias and manually run the test runner when I'm doing core dev.

[bnoordhuis]

but I am curious how this messes up your workflow.

When I'm writing code, I run make test frequently as a quick sanity check (just a few tests, then ^C), but now it takes 30 seconds before the first test runs. I can run the build and the test runner manually but that's not very convenient.

[jasnell]

Not particularly a fan of this also but I can live with it. In general I'll typically get things working before linting as a final step.

[Trott]

This would run the tests first and then run the lint jobs whether or not the tests passed, and will fail if any of the tests or lint jobs fails. Unfortunately, it's also kind of hideous. And I'm not 100% certain of its portability.

test: | cctest  # Depends on 'all'.
    RC=0; \
    $(PYTHON) tools/test.py --mode=release message parallel sequential -J || RC=$$?; \
    $(MAKE) jslint || RC=$$?; \
    $(MAKE) cpplint || RC=$$?; \
    exit $$RC

I'm fine with a simple revert instead. Just exploring options. Aesthetics aside, I like this functionality if we can be reasonably sure it will work like it's supposed to everywhere.

[MylesBorins]

are you sure this works on windows? I've had weirdness before with ; and other bashities

[Trott]

@thealphanerd Node.js Windows build uses vcbuild.bat and not Makefile so I'm pretty sure this won't affect Windows.

[jbergstroem]

My preference is also disconnecting make test from make lint; but I get the impression we're not optimising for us developers here.

[Trott]

Revert PR is at #5602

[MylesBorins]

since this was reverted I'm removing lts-watch and adding dont land