doc: run tests before landing changes

[Trott]

The Collaborator Guide suggests running tests after pushing changes to
the main repository. Tests should be run before then so move the
instructions earlier.

Instructions are also simplified and re-written to not exclude Windows.

Checklist

• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

doc

[thefourtheye]

We are losing the -j8 information now :(

[gibfahn]

+1, I think we standardised on -j4, so it should probably be make -j4 test. I know it doesn't make the tests any faster, but if a rebuild is necessary it will help with that.

[Trott]

Right now:

make test shows up in:

• BUILDING.md
• CONTRIBUTING.md
• doc/guides/building-node-with-ninja.md:

make -j4 test shows up in:

• .github/PULL_REQUEST_TEMPLATE.md
• CONTRIBUTING.md
• doc/onboarding-extras.md:

But this doc is the only place where make -j8 test shows up.

I think we should standardize on something in a different PR. And in fact, I might go off and do that right...about....now!

[refack]

Thanks for the Windows inclusion. We are people too 😉 (maybe foolish but people).
Also #12281 vcbuild test can easily take 30 minutes after even a minor change.

[refack]

Also #12425 when I solve it, need to update this.

[gibfahn]

I think we should standardize on something in a different PR. And in fact, I might go off and do that right...about....now!

Looks like that became: #12437

[benjamingr]

Am I expected to run the test locally for changes? I don't run the tests locally for most changes I land - I only do so if there were changes since the CI run and if it's not a doc change or similar (or if I wrote the changed code).

[gibfahn]

Am I expected to run the test locally for changes?

I usually run the linter locally to make sure nothing weird happened, but I agree that the default should be to rerun CI if the PR has been updated since it was last run, at which point there's no real need to do more than core-validate-commit @.

[benjamingr]

Also, I don't trust the tests on my computer (since it's just one platform) and tests are run by the release team anyway before releases - right?

[addaleax]

So … I don’t know about others, but I do run make test before pushing to master. I’m not usually worried about newer commits in the PR itself, but more afraid of some possible interaction between the PR and commits that have landed on master since the CI was run which might break master.

I don’t think that has ever really happened to me, though. 😄

[refack]

Also, I don't trust the tests on my computer (since it's just one platform) and tests are run by the release team anyway before releases - right?

@benjamingr IMHO the repo should be green. Always. Not just for releases, but out of respect for the other devs. You need a clear table to work on, so if you see a problem you can be sure it's caused by you, and not something you inherited.
I know running the test suite locally is cumbersome and a incomplete, so how about running another CI job after the land, but waiting for the results and fielding any fallback?

@Trott I'm in the opinion that landing should not be a cut & run thing (re last Friday night and #12279). Landing should be done when the responsible party is available to field issues that might that will arise from the new code. I hope something of this spirit finds its way into the docs.

[Trott]

So … I don’t know about others, but I do run make test before pushing to master. I’m not usually worried about newer commits in the PR itself, but more afraid of some possible interaction between the PR and commits that have landed on master since the CI was run which might break master.

☝️ This, exactly. I know some Collaborators don't run the tests locally when landing a PR. But I do recommend that you do that, but if not, then please at least run the linter as @gibfahn suggests. That will catch the most frequent cause of issues: Since the last CI run, a new lint rule has landed and the code in the PR doesn't conform to the new lint rule, causing CI to go red after the PR lands.

My ideal solution to the "changes have happened since the last CI run" issue is to automate the landing process: Basically, tell Jenkins, "time to land PR 12345." Jenkins checks the commit message formatting etc., runs CI, and only after a clean CI, lands on master. If something else landed on master while the CI was running, Jenkins sees that and starts over again.

Node.js experimented with a Jenkins job like this about (I think) 2 years ago or so. At the time, CI was considerably more brittle than it is now, and so it was kind of a disaster. I think the experience has permanently scared some people from wanting to try again. I hope we do get there, though. More automation of our processes that ought to be mechanical, please, kthxbai.

[jasnell]

At the very least, I will make lint for any code changes. I will base the decision to make test on how substantial the change is and on what else has landed since the CI run. If other changes that affect the same area of code, then I will definitely make test before pushing.

[Trott]

I'll preserve -j4 here (down from -j8 because we use -j4 everywhere else) and if #12437 or similar lands, it can be removed at that time.

[Trott]

Also took the opportunity to clarify that the "other changes may have landed" is "on master" and not on the PR branch. So hopefully that will clear up some of the confusion that occurred above!

[jasnell]

Landed in 1e7f6b1