Node: 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: https://github.com/nodejs/node/issues/8286, https://github.com/nodejs/node/pull/8678, https://github.com/nodejs/node/pull/9961, https://github.com/nodejs/node/pull/9881, https://github.com/nodejs/node/pull/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 https://github.com/nodejs/node/pull/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.

[apapirovski]

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.

Yes but wouldn't the build be the bigger problem here? It's painfully slow with make test last I checked.

[chrismilleruk]

Yes but wouldn't the build be the bigger problem here? It's painfully slow with make test last I checked.

It's definitely slow. Presumably any build prerequisites would run in single job mode with make test.

[Trott]

Maybe it should be this?

make -j`node -p os.cpus\(\).length` test

I'm kidding. Maybe.

[spwg]

Can I take this? I plan to make a small, docs-only change to BUILDING.md (https://github.com/nodejs/node/blob/master/BUILDING.md#running-tests) at line 179, the $ make test part.

[rvagg]

It's even more messy than that @Trott .. this is something I'd like to see fixed up because it's a mess in CI already.

You can -j X with make but it doesn't propagate to tools/test.py, it only does the _build_ in parallel. The call to tools/test.py is given a -J, it doesn't pass X on. Here's what test.py does when it's run with -J:

• If JOBS is set in the current environment then use that as the parallel count
• If there is no sensible JOBS env var then use the Python multiprocessing.cpu_count() call which can be inaccurate on some VMs or containers (like os.cpus().length .. SmartOS is one of these).

So, what we tend to do in CI is be explicit about it, with something like:

if [ -z ${JOBS+x} ]; then
  JOBS=$(getconf _NPROCESSORS_ONLN) #reported cores, kind of like `os.cpus().length`
fi
JOBS=$JOBS make test-ci -j $JOBS

And in some configurations we explicitly set a JOBS, like on SmartOS and our 96-core ARMv8 hosts that shouldn't be running with a parallellism of 96.

My wish is that -j X was definitive for both building and testing, but I don't believe you can fetch the -j value from inside a Makefile and you'd have to do some ppid hackery to look it up from the tools/test.py subprocess.

I don't think this helps this discussion but it's worth being aware of this behaviour if you're tinkering in this area.

[bnoordhuis]

My wish is that -j X was definitive for both building and testing, but I don't believe you can fetch the -j value from inside a Makefile

That's right, make treats -jX specially. You can't fetch it from MAKEFLAGS or GNUMAKEFLAGS, you'll only see -j without the count.

(There are reasons for that, it lets the supervisor control parallelism in nested invocations.)

[chrismilleruk]

You can -j X with make but it doesn't propagate to tools/test.py
My wish is that -j X was definitive for both building and testing

I created a separate issue for this: https://github.com/nodejs/node/issues/20065