tests: extend test scripts to cover OpenBSD #18543

qbit · 2018-02-03T03:39:37Z

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

lpinca · 2018-02-03T07:30:49Z

test/common/index.js

@@ -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);


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 · 2018-02-03T10:06:12Z

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 · 2018-02-03T14:50:04Z

@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 · 2018-02-03T15:38:05Z

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

qbit · 2018-02-03T17:03:02Z

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

lpinca · 2018-02-03T17:16:32Z

test/parallel/test-child-process-exec-timeout.js

+  if (!common.isOpenBSD)
+    assert.strictEqual(err.code, null);
+  else
+    sigterm = null;


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

richardlau · 2018-02-03T17:19:29Z

test/parallel/test-child-process-exec-timeout.js

  // 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);


sigterm is undefined on non-OpenBSD.

Whoops - defined now.

changed the comment as well.

qbit · 2018-02-03T18:02:24Z

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

lpinca · 2018-02-03T20:47:00Z

test/parallel/test-child-process-exec-timeout.js

+  if (!common.isOpenBSD)
+    assert.strictEqual(err.code, null);
+  else
+    sigterm = null;


Should we assert that err.code is 143 then?

Probably. Updated.

qbit · 2018-02-03T21:51:05Z

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

gibfahn · 2018-02-05T07:09:22Z

lib/internal/bootstrap_node.js

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


Before what?

before something uses process.execPath

Makes sense, thanks for elaborating.

BridgeAR · 2018-02-06T21:42:21Z

test/parallel/test-child-process-spawnsync-validation-errors.js


 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);


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.

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 · 2018-02-08T14:39:35Z

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 · 2018-02-10T19:54:45Z

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

BridgeAR · 2018-02-12T19:26:18Z

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

qbit · 2018-02-13T00:20:49Z

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

PR-URL: nodejs#18543 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR · 2018-02-16T17:02:45Z

Landed in bd4350c, ef0e92e 🎉

PR-URL: #18543 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#18543 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 3, 2018

lpinca reviewed Feb 3, 2018

View reviewed changes

qbit added 2 commits February 3, 2018 09:56

lib: set process.execPath on OpenBSD

13192a9

test: update a few test scripts to work on OpenBSD

455dc60

qbit force-pushed the openbsd_tests branch from 5671b15 to 455dc60 Compare February 3, 2018 17:00

lpinca reviewed Feb 3, 2018

View reviewed changes

richardlau reviewed Feb 3, 2018

View reviewed changes

qbit added 2 commits February 3, 2018 10:29

test: define sigterm

42f51a5

fix comment for openbsd

de838fa

more descriptive comment

59edf9f

lpinca reviewed Feb 3, 2018

View reviewed changes

qbit added 3 commits February 3, 2018 14:46

check code 143 on openbsd

22eb27c

check for non-elf obj on openbsd

d0c6dfa

skip v8 debug symbol test on openbsd

7aa9a7c

lpinca approved these changes Feb 3, 2018

View reviewed changes

gibfahn reviewed Feb 5, 2018

View reviewed changes

jasnell approved these changes Feb 5, 2018

View reviewed changes

expand comment in bootstrap_node.js

31d4c9a

BridgeAR reviewed Feb 6, 2018

View reviewed changes

revert

ad7bf60

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

lib: set process.execPath on OpenBSD

bd4350c

PR-URL: nodejs#18543 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 16, 2018

test: update a few tests to work on OpenBSD

ef0e92e

PR-URL: nodejs#18543 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR closed this Feb 16, 2018

MylesBorins pushed a commit that referenced this pull request Feb 21, 2018

lib: set process.execPath on OpenBSD

7bd5eb3

PR-URL: #18543 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 21, 2018

test: update a few tests to work on OpenBSD

1a97840

PR-URL: #18543 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

MylesBorins pushed a commit that referenced this pull request Feb 21, 2018

lib: set process.execPath on OpenBSD

1557d93

PR-URL: #18543 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Feb 21, 2018

test: update a few tests to work on OpenBSD

ed55374

PR-URL: #18543 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Apr 13, 2018

lib: set process.execPath on OpenBSD

77a405b

PR-URL: #18543 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Apr 13, 2018

test: update a few tests to work on OpenBSD

d799b1c

PR-URL: #18543 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Apr 13, 2018

lib: set process.execPath on OpenBSD

9129bc4

PR-URL: #18543 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request Apr 13, 2018

test: update a few tests to work on OpenBSD

9236332

PR-URL: #18543 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins mentioned this pull request Apr 13, 2018

v6.14.2 proposal #19996

Merged

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018

lib: set process.execPath on OpenBSD

9752795

PR-URL: nodejs#18543 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018

test: update a few tests to work on OpenBSD

a349093

PR-URL: nodejs#18543 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax mentioned this pull request Aug 31, 2018

OpenBSD bad node executable. #22629

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: extend test scripts to cover OpenBSD #18543

tests: extend test scripts to cover OpenBSD #18543

qbit commented Feb 3, 2018

lpinca Feb 3, 2018 •

edited

bnoordhuis commented Feb 3, 2018

qbit commented Feb 3, 2018 •

edited

qbit commented Feb 3, 2018

qbit commented Feb 3, 2018 •

edited

lpinca Feb 3, 2018

richardlau Feb 3, 2018

qbit Feb 3, 2018

qbit Feb 3, 2018

qbit commented Feb 3, 2018

lpinca Feb 3, 2018

qbit Feb 3, 2018

qbit commented Feb 3, 2018

gibfahn Feb 5, 2018

qbit Feb 6, 2018

gibfahn Feb 6, 2018

BridgeAR Feb 6, 2018

qbit Feb 8, 2018

qbit commented Feb 8, 2018

BridgeAR commented Feb 10, 2018

BridgeAR commented Feb 12, 2018

qbit commented Feb 13, 2018

BridgeAR commented Feb 16, 2018

tests: extend test scripts to cover OpenBSD #18543

tests: extend test scripts to cover OpenBSD #18543

Conversation

qbit commented Feb 3, 2018

Checklist

Affected core subsystem(s)

lpinca Feb 3, 2018 • edited

Choose a reason for hiding this comment

bnoordhuis commented Feb 3, 2018

qbit commented Feb 3, 2018 • edited

qbit commented Feb 3, 2018

qbit commented Feb 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qbit commented Feb 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qbit commented Feb 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qbit commented Feb 8, 2018

BridgeAR commented Feb 10, 2018

BridgeAR commented Feb 12, 2018

qbit commented Feb 13, 2018

BridgeAR commented Feb 16, 2018

lpinca Feb 3, 2018 •

edited

qbit commented Feb 3, 2018 •

edited

qbit commented Feb 3, 2018 •

edited