test: a small common.skip() improvement proposal

[vsemozhetbyt]

Currently, common.skip() only outputs a message, so after each call, we need to add return; in tests — this is +~400 lines of code. Would it be safe to make this function like common.skipIfInspectorDisabled(), i.e. to add process.exit(0); in common.skip() and remove all the return; in tests? If there are some +1 for this proposal, I can try to raise a PR.

[addaleax]

I think some test files are using it to partially skip tests, and let it print a skip message in the format that TAP understands; I’m not sure on the details, but you’d probably need to check the relevant files to see where there is actual code executed after the skip().

[Trott]

I think some test files are using it to partially skip tests, and let it print a skip message in the format that TAP understands; I’m not sure on the details, but you’d probably need to check the relevant files to see where there is actual code executed after the skip().

An example is test/parallel/test-buffer-alloc.js. (I'm actually not sure that isn't a code mistake TBH. I'm not sure TAP understands "partial skip". I think with TAP you either SKIP the test or you don't.)

[vsemozhetbyt]

It seems we have ~10 tests with partial skip now if I skim properly. So would it be OK to just add a message via console.log() in these cases?

[addaleax]

@vsemozhetbyt Either that, or you could just rename the current skip function to something else, common.printSkipMessage or whatever, then make skip a wrapper around that + process.exit.

[vsemozhetbyt]

@addaleax @Trott
I've found out one case when return; is more appropriate than process.exit(0): if common.skip() is called after a common.mustCall(), then return; exits gracefully (with async callbacks fired) while process.exit(0) causes error. There are 2 such files:

parallel/test-dgram-bind-default-address.js
sequential/test-net-server-address.js

I've left them with common.printSkipMessage() + return;.

PR: #14021