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.

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

Conversation

qbit
Copy link
Contributor

@qbit qbit commented Feb 3, 2018

Checklist
  • make -j4 test
  • tests and/or benchmarks are included ( \o/ )
  • commit message follows commit guidelines
Affected core subsystem(s)

test


Currently a large portion of the tests fail on OpenBSD. Most fail because node expects to be a to determine the full path of a executable, which isn't possible on OpenBSD. This PR works around that issue by setting common.execPathOpenBSD as early as possible. Then we use this variable later in the tests when we need to know the full path of the executable.

A few of the other tests expose some bugs in libuv that I will track down pretty soon here.

Anyway - with this patch set, the tests complete as expected:

[----------] Global test environment tear-down
[==========] 68 tests from 8 test cases ran. (193 ms total)
[  PASSED  ] 68 tests.
/usr/local/bin/python2.7 tools/test.py --mode=release -J \
        default \
        addons addons-napi \
        doctool
[06:05|% 100|+ 2125|-   0]: Done

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 3, 2018
@@ -55,6 +54,8 @@ exports.isLinuxPPCBE = (process.platform === 'linux') &&
(os.endianness() === 'BE');
exports.isSunOS = process.platform === 'sunos';
exports.isFreeBSD = process.platform === 'freebsd';
exports.isOpenBSD = process.platform === 'openbsd';
exports.execPathOpenBSD = path.join(__dirname, '../../', process.execPath);
Copy link
Member

@lpinca lpinca Feb 3, 2018

Choose a reason for hiding this comment

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

What do think you about exporting execPath like this

exports.execPath = exports.isOpenBSD
  ? path.join(__dirname, '../../', process.execPath)
  : process.execPath;

and use common.execPath instead of process.execPath in the tests? This should slightly reduce the diff noise.

@bnoordhuis
Copy link
Member

node expects to be a to determine the full path of a executable, which isn't possible on OpenBSD

Untested, but shouldn't this patch fix that?

diff --git a/lib/internal/bootstrap_node.js b/lib/internal/bootstrap_node.js
index 33de5ae4be..4c7cf41530 100644
--- a/lib/internal/bootstrap_node.js
+++ b/lib/internal/bootstrap_node.js
@@ -75,6 +75,11 @@
     // URL::ToObject() method is used.
     NativeModule.require('internal/url');
 
+    if (process.platform === 'openbsd') {
+      const { realpathSync } = NativeModule.require('fs');
+      process.execPath = realpathSync.native(process.execPath);
+    }
+
     Object.defineProperty(process, 'argv0', {
       enumerable: true,
       configurable: false,

@qbit
Copy link
Contributor Author

qbit commented Feb 3, 2018

@bnoordhuis correct me if I am wrong, but if libuv can't get the full path of a process (there is no KERN_PROC_PATHNAME or similar in OpenBSD), this NativeModule thing won't work, right?

OpenBSD doesn't have a canonical way to get the full path of an executable from inside said executable.

edit maybe it will work.. I will give it a test.

@qbit
Copy link
Contributor Author

qbit commented Feb 3, 2018

Yep - that works! I will re-work the diff!

@qbit
Copy link
Contributor Author

qbit commented Feb 3, 2018

Updated to use the method @bnoordhuis pointed out - much cleaner now!

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.

// 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.

@qbit
Copy link
Contributor Author

qbit commented Feb 3, 2018

Updated. I will squash the last few commits later on (in case there are more issues found :P)

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.

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.

@qbit
Copy link
Contributor Author

qbit commented Feb 3, 2018

re-added a few tests that I seem to have clobbered with git.. also made a few actually test for specific things vs skipping the test entirely (except the v8 debug symbols, skipping that one).

@@ -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 (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)?

@qbit
Copy link
Contributor Author

qbit commented Feb 8, 2018

Movning this here for visibility:

There are some other strange things happening that might be related to the issue @BridgeAR brought up with my incorrect patch on test-child-process-spawnsync-validation-errors.js :

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)? @bnoordhuis any suggestions?

@BridgeAR
Copy link
Member

@BridgeAR
Copy link
Member

@qbit I suggest to open some issues for your findings. Because I guess this can otherwise land as is?

@qbit
Copy link
Contributor Author

qbit commented Feb 13, 2018

Yeah, I will open a new issues for the stuff I found.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 13, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 16, 2018
PR-URL: nodejs#18543
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 16, 2018
PR-URL: nodejs#18543
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR
Copy link
Member

Landed in bd4350c, ef0e92e 🎉

@BridgeAR BridgeAR closed this Feb 16, 2018
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
PR-URL: #18543
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
PR-URL: #18543
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
PR-URL: #18543
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
PR-URL: #18543
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 13, 2018
PR-URL: #18543
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 13, 2018
PR-URL: #18543
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 13, 2018
PR-URL: #18543
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 13, 2018
PR-URL: #18543
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 13, 2018
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#18543
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#18543
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants