Addon tests aren’t run in debug-mode CI

[addaleax]

Continuing from nodejs/node#26754, this seems like an issue that affects only CI:

Addon tests aren’t built and run in the debug-mode CI. That’s bad, because they are precisely the tests that need debug-mode CI runs the most. And more, generally, I’d expect the same tests to be run on all non-fanned CI jobs or at least to have exceptions mentioned in test/README.md in nodejs/node.

[sam-github]

For my own edification, how does one trigger debug-mode CI? I assume its a way of doing ./configure --debug; make test?

[addaleax]

@sam-github Is node-test-pull-request → node-test-commit → node-test-commit-linux-containered → ubuntu1604_sharedlibs_debug_x64 the answer to that question? (And yes, it does basically that)

[rvagg]

Ah that would be because we do some manual test running. From https://ci.nodejs.org/view/All/job/node-test-commit-linux-containered/configure:

It gets built with:

PYTHON=python \
 NODE_TEST_DIR=${HOME}/node-tmp \
 CONFIG_FLAGS="$CONFIG_FLAGS --debug" \
 make build-ci -j $JOBS # --output-sync=target

Then after testing that we have a debug executable (by location @ out/Debug/node and by inspection with out/Debug/node -pe process.config.target_defaults.default_configuration) it gets tested with:

python tools/test.py -j $JOBS -p tap --logfile test.tap \
  --mode=debug --flaky-tests=skip \
  async-hooks default known_issues

I don't recall exactly why that's separate, but I suspect it's because of the --mode=debug thing, maybe that's not in Makefile, or maybe it wasn't in Makefile when this was put together. I do remember having lots of difficulty wiring this up so that's probably it.

What we should do is either fix up the Makefile to make it more straightforward to do a run-ci with a "debug" flag of some kind (CONFIG_FLAGS probably isn't it). Or, for now perhaps simply fix up that test.py call.

This is also in the --without-ssl test in the same group, it probably has the same problems because build/config and test are separate processes too. I'm not sure why they're separate in this case, however. It could be because it was important to test that we built the binary right before executing tests (because that was tricky to get right when we first set this up!). This should either be rolled into a single run-ci or the test.py call should be aligned with what it should do, or maybe there's a make that can be called to invoke this?

python tools/test.py -j $JOBS -p tap --logfile test.tap --flaky-tests=skip async-hooks default known_issues

[addaleax]

@sam-github @rvagg So … what’s the best way forward here for somebody who’s invested in making this happen but doesn’t have access to any of the Jenkins stuff?

[sam-github]

@addaleax You could join the working group, then you'd have access to troubleshoot anywhere, or open an issue asking for access to some specific machines, trust will obviously not be an issue.

[rvagg]

2 ways forward, not mutually exclusive:

1. Someone proposes fixes for Makefile for run-ci to handle --mode=debug (somehow) and --without-ssl (somehow). I don't think CONFIG_FLAGS covers it, at least for the debug case because it has to run the tests in debug mode too. Maybe the --without-ssl case can be achieved by CONFIG_FLAGS alone? I'd love for someone with the time to experiment with this.
2. Someone proposes replacements for the above commands being run in CI and I'll drop them in. If it's an easy fix by amending what we have then we can just do that and kick this can down the road. I'm fine with that too but this will come up again in the future when someone adds something else to run-ci and we don't use that for some of our tests.

I'm fine updating Jenkins, @sam-github can do it too. Just give me what it should be. I don't really have time right now to be spending debugging this to make sure it comes out right. I think I've given everything necessary above to replicate it locally.

[rvagg]

These two address Makefile and let us use run-ci: nodejs/node#28747, nodejs/node#28750.
I have a new node-test-commit-linux-containered that's heavily refactored, extracts a script to put into this repo and should be able to add --workers tests too. Still working on it but with those two PRs it's 👍 for master at least.

[github-actions]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

[addaleax]

I’ll remove the stale label because this has bitten us once again: nodejs/node#33276

I’ll try to see if I can do something about this myself now that I’m in the build WG but I’m not sure when I’ll have time to get to it.

[github-actions]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.