Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: extend test scripts to cover OpenBSD #18543

Closed
wants to merge 10 commits into from
Closed

tests: extend test scripts to cover OpenBSD #18543

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
13192a9
lib: set process.execPath on OpenBSD
qbit Feb 3, 2018
455dc60
test: update a few test scripts to work on OpenBSD
qbit Feb 3, 2018
42f51a5
test: define sigterm
qbit Feb 3, 2018
de838fa
fix comment for openbsd
qbit Feb 3, 2018
59edf9f
more descriptive comment
qbit Feb 3, 2018
22eb27c
check code 143 on openbsd
qbit Feb 3, 2018
d0c6dfa
check for non-elf obj on openbsd
qbit Feb 3, 2018
7aa9a7c
skip v8 debug symbol test on openbsd
qbit Feb 3, 2018
31d4c9a
expand comment in bootstrap_node.js
qbit Feb 6, 2018
ad7bf60
revert
qbit Feb 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions lib/internal/bootstrap_node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,13 @@
// URL::ToObject() method is used.
NativeModule.require('internal/url');

// On OpenBSD process.execPath will be relative unless we
// get the full path before hand.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before what?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before something uses process.execPath

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks for elaborating.

if (process.platform === 'openbsd') {
const { realpathSync } = NativeModule.require('fs');
process.execPath = realpathSync.native(process.execPath);
}

Object.defineProperty(process, 'argv0', {
enumerable: true,
configurable: false,
Expand Down
1 change: 1 addition & 0 deletions test/common/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ exports.isLinuxPPCBE = (process.platform === 'linux') &&
(os.endianness() === 'BE');
exports.isSunOS = process.platform === 'sunos';
exports.isFreeBSD = process.platform === 'freebsd';
exports.isOpenBSD = process.platform === 'openbsd';
exports.isLinux = process.platform === 'linux';
exports.isOSX = process.platform === 'darwin';

Expand Down
9 changes: 7 additions & 2 deletions test/parallel/test-child-process-exec-timeout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,20 @@ const cmd = `"${process.execPath}" "${__filename}" child`;

// Test the case where a timeout is set, and it expires.
cp.exec(cmd, { timeout: 1 }, common.mustCall((err, stdout, stderr) => {
let sigterm = 'SIGTERM';
assert.strictEqual(err.killed, true);
assert.strictEqual(err.code, null);
// TODO OpenBSD returns a null signal and 143 for code
if (!common.isOpenBSD)
assert.strictEqual(err.code, null);
else
sigterm = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm not wrong this is now a global, the test should fail.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we assert that err.code is 143 then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably. Updated.

// At least starting with Darwin Kernel Version 16.4.0, sending a SIGTERM to a
// process that is still starting up kills it with SIGKILL instead of SIGTERM.
// See: https://github.com/libuv/libuv/issues/1226
if (common.isOSX)
assert.ok(err.signal === 'SIGTERM' || err.signal === 'SIGKILL');
else
assert.strictEqual(err.signal, 'SIGTERM');
assert.strictEqual(err.signal, sigterm);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sigterm is undefined on non-OpenBSD.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops - defined now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed the comment as well.

assert.strictEqual(err.cmd, cmd);
assert.strictEqual(stdout.trim(), '');
assert.strictEqual(stderr.trim(), '');
Expand Down
6 changes: 5 additions & 1 deletion test/parallel/test-child-process-spawnsync-validation-errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,17 @@ const spawnSync = require('child_process').spawnSync;
const signals = process.binding('constants').os.signals;

let invalidArgTypeError;
let errCode = 56;
if (common.isOpenBSD)
errCode = 46;

if (common.isWindows) {
invalidArgTypeError =
common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 36);
} else {
invalidArgTypeError =
common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 56);
common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError },
errCode);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is not an errCode. It is actually the amount of times this error has to be called so the test will work. So the name should be changed.

But more importantly: I am very confused that it will not be triggered the same amount of times as on other systems. Windows skips a couple ones, that is the reason why it has a different call number. But OpenBSD should be called in all cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah my bad. I will revert this one and just leave it erroring for future investigation.

There are some other strange things happening that might be related:

in test/sequential/test-child-process-execsync.js I am hitting assert(end < SLEEP); on line 60 - end is consistently + ~300ms. Maybe it's a timing issue (no idea where to look / troubleshoot to find out)?

}

const invalidRangeError =
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-fs-utimes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,8 +178,8 @@ process.on('exit', function() {
const path = `${tmpdir.path}/test-utimes-precision`;
fs.writeFileSync(path, '');

// test Y2K38 for all platforms [except 'arm', and 'SunOS']
if (!process.arch.includes('arm') && !common.isSunOS) {
// test Y2K38 for all platforms [except 'arm', 'OpenBSD' and 'SunOS']
if (!process.arch.includes('arm') && !common.isOpenBSD && !common.isSunOS) {
// because 2 ** 31 doesn't look right
// eslint-disable-next-line space-infix-ops
const Y2K38_mtime = 2**31;
Expand Down
8 changes: 6 additions & 2 deletions test/parallel/test-http-dns-error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ const https = require('https');
const host = '*'.repeat(256);
const MAX_TRIES = 5;

let errCode = 'ENOTFOUND';
if (common.isOpenBSD)
errCode = 'EAI_FAIL';

function tryGet(mod, tries) {
// Bad host name should not throw an uncatchable exception.
// Ensure that there is time to attach an error listener.
Expand All @@ -41,7 +45,7 @@ function tryGet(mod, tries) {
tryGet(mod, ++tries);
return;
}
assert.strictEqual(err.code, 'ENOTFOUND');
assert.strictEqual(err.code, errCode);
}));
// http.get() called req1.end() for us
}
Expand All @@ -57,7 +61,7 @@ function tryRequest(mod, tries) {
tryRequest(mod, ++tries);
return;
}
assert.strictEqual(err.code, 'ENOTFOUND');
assert.strictEqual(err.code, errCode);
}));
req.end();
}
Expand Down
8 changes: 6 additions & 2 deletions test/parallel/test-net-dns-error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,21 @@ const net = require('net');

const host = '*'.repeat(256);

let errCode = 'ENOTFOUND';
if (common.isOpenBSD)
errCode = 'EAI_FAIL';

function do_not_call() {
throw new Error('This function should not have been called.');
}

const socket = net.connect(42, host, do_not_call);
socket.on('error', common.mustCall(function(err) {
assert.strictEqual(err.code, 'ENOTFOUND');
assert.strictEqual(err.code, errCode);
}));
socket.on('lookup', function(err, ip, type) {
assert(err instanceof Error);
assert.strictEqual(err.code, 'ENOTFOUND');
assert.strictEqual(err.code, errCode);
assert.strictEqual(ip, undefined);
assert.strictEqual(type, undefined);
});
2 changes: 1 addition & 1 deletion test/parallel/test-setproctitle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ exec(cmd, common.mustCall((error, stdout, stderr) => {
assert.strictEqual(stderr, '');

// freebsd always add ' (procname)' to the process title
if (common.isFreeBSD)
if (common.isFreeBSD || common.isOpenBSD)
title += ` (${path.basename(process.execPath)})`;

// omitting trailing whitespace and \n
Expand Down