build: make addon testing part of make test

[bnoordhuis]

Checklist

• tests and code linting passes
• a test and/or benchmark is included
• documentation is changed or added
• the commit message follows commit guidelines

Affected core subsystem(s)

test

Description of change

build: make addon testing part of make test

Otherwise it's too easy to miss breaking changes to node.h and other
public headers until the CI catches them. vcbuild test tests addons
so there is precedence.

( test: is perhaps a better prefix but most commits to Makefile prefix with build. I follow slavishly.)

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

[MylesBorins]

I'm very much in favor of this change. One thing worth mentioning is the flakyness of test-addons

When testing on osx I fairly often have to run the command twice in a row due to some issue I've never quite hunted down. Am I the only one experiencing this? @bnoordhuis I can try apply this patch to a couple different parts of the tree and see if I can repro the failures if you like

[bnoordhuis]

I spent some time last year sorting out build dependencies and make test-addons has been pretty solid for me on Linux ever since. If you can tell me how to reproduce on OS X, I'll look into it.

[jasnell]

I've seen the same flakiness on OS X but like @thealphanerd I haven't had a chance to track it down. I'll see if I can get some info later on today.

[agnat]

I haven't had a chance to track it down.

All of this sounds terrible familiar, so I believe I have...

To clarify, on OS X once in a while we catch a "bad add-on build". That is plugging this "bad add-on" crashes reproducibly. It is not flaky in the sense that plugging may or may not work. Only rebuilding the add-on resolves the issue, right?

I tracked this down to a variant of what is sometimes called the C++ static initialization order fiasco. IIRC, it boils down to this: Both the initialization of the node::node_module struct and the NODE_C_CTOR() are run at init-time... under certain circumstances. Since the order of initialization is undefined those builds that run the ctor before initializing the struct fail.

Apparently this is no problem on ELF systems. The order is deterministic and correct. However, on Mach-O systems the order is non-deterministic and once in while we catch a bad build.

I fixed this in this commit while working on #2329, mostly as an afterthought. It popped up in the CI build.

So, why is this not a problem in the real world? This has to do with the circumstances mentioned above. Most add-ons use simple node_module structs that only contain compile-time constants. Such a struct is not initialized at init-time but at load-time. No order issues there. My guess is that one of the tests, being thorough, does something more interesting and thus triggers the issue. IIRC, one easy way to produce an add-on that shows the effect is to use a non-constant private field. Untested:

void Init() {}
void* GetPrivate() { return new int; }
NODE_MODULE_X(bad_add_on, Init, GetPrivate(), 0);

It still isn't easy to catch, though. I used an infinite rebuild-test-clean-loop...

[agnat]

I spent some time trying to reproduce this but it seems gone ...

So unless anyone else is able to reproduce, forget what I said. ;)

[bnoordhuis]

@thealphanerd @jasnell Any luck? If not, I'd like to land this.

[jasnell]

@bnoordhuis ... don't let me hold it up, I haven't had a chance to investigate further.

[MylesBorins]

We may as well land this and if we start hitting the edge case often we can then start to fix it. I am pretty sure that we will, but cannot reliably reproduce

[bnoordhuis]

Rebase and new CI: https://ci.nodejs.org/job/node-test-pull-request/2450/

[jbergstroem]

LGTM