doc: make test commands a little more consistent

[ryzokuken]

In the PULL_REQUEST_TEMPLATE.md file, we ask contributors (who are now submitting a PR) to test the codebase using the command make -j4 test, employing 4 jobs. (https://github.com/nodejs/node/blob/master/.github/PULL_REQUEST_TEMPLATE.md#checklist)

On the other hand, the BUILDING.md file, we use make test only, which uses a single job, IIRC. (https://github.com/nodejs/node/blob/master/BUILDING.md#running-tests)

This might not be a major issue per-se, but we could make it more consistent.

[chrismilleruk]

+1

I got caught by this on my first contribution and it was painfully slow to the point I was reconsidering whether to contribute at all.

It looks like it's been discussed a couple of times before: #8286, #8678, #9961, #9881, #12437.

My vote would be a sensible default such as -j4 and to update all mentions in the docs so that those new users who copy/paste commands will [probably] get a speed boost.

Happy to help with the updates once a consensus is reached.

[vsemozhetbyt]

cc @nodejs/build @nodejs/documentation

[apapirovski]

Seems like a good change. Added labels.

[gibfahn]

So the suggestion is that we change make test-only to make -j4 test-only? Sounds like a great idea to me.

We did standardize on -j4 everywhere after a fair amount of discussion previously, I think it was in #9961.

So a PR that adds a -j4 to every make command sounds like a good idea to me.

[ryzokuken]

@gibfahn I was thinking somewhere along the lines of doing a repository-wide search for make test (and maybe even make -j4 test) and replace it with make -j4 test and a helpful sidenote about make test, why it's an option and when you should choose either.

Because sometimes someone might need just make test, you know. That's a really small subset, I think, but a subset nonetheless. Plus we really want newer contributors to know the reason they're using the -j4 flag so they could either use it or drop it at will depending on the situation.

[gibfahn]

The PR I referenced above (#9961) did exactly that, it used -j4 everywhere, and added a note to BUILDING.md to explain why to newer contributors. There's also discussion in there about how likely it is that adding it would be detrimental to new contributors building node on single or dual core platforms (conclusion is that it would be very unlikely).

Running make with the -j4 flag will cause it to run 4 compilation jobs
concurrently which may significantly reduce build time. The number after -j
can be changed to best suit the number of processor cores on your machine. If
you run into problems running make with concurrency, try running it without
the -j4 flag. See the GNU Make Documentation for more information.

I think we just need to add -j4 to any new sections of the docs that were added since then, and didn't include the -j4 section.

I don't think we should add the "This is what -j4 does" paragraph to everywhere -j4 is used though, that sounds like a lot of duplication.

[ryzokuken]

Agreed. If it's there in BUILDING.md, people will find it anyway.

[chrismilleruk]

I did a bit of analysis on this ⬇️and I'm wondering if it might be better to promote the use of export MAKEFLAGS=-j4 in BUILDING.md#Building Node.js than to update all doc references with the explicit -j4 option.

e.g.

To build Node.js:

$ ./configure
$ export MAKEFLAGS=-j4
$ make

This has the same performance boost, applies to all subsequent make commands in that session, keeps the docs clean and feels like a more elegant change.

I'm certainly no make expert so not sure if there are any drawbacks to this vs. the more explicit approach?

...

ℹ️ I extended the search to make coverage* and make doc* in addition to make and make test*. I found 52 matches of which around half of them look to be sensible changes.

Regex: (^(\$ )?|`|&& )([A-Z_]+=.+ )*make( test| coverage| doc|$|`)

Note also, as a nit, there is inconsistent use of the $ prefix within terminal/console code blocks.

[ryzokuken]

I'm in favor of explicitly using the -j4 flag (come on, it's just three characters) because it allows you to change the number of jobs easily (as compared to exporting again).

[addaleax]

Fwiw, make test does run the parallel JS test suite using the number of cores registered of the machine, regardless of make flags.

I remember believing otherwise first, until @Trott told me that that’s what really happens – I would be okay with changing that, but for now, make test in the BUILDING.md seems okay.

[Trott]

I remember believing otherwise first, until @Trott told me that that’s what really happens

Oh, right, I think test.py spawns the right number of processes. I forgot about that myself.