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

test: verify input flags #24876

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@BridgeAR
Copy link
Member

BridgeAR commented Dec 6, 2018

This makes sure all required flags are passed through to the test.
If that's not the case an error is thrown to inform the user what
flag is missing.

This is especially helpful for people new to Node core who start some
tests and don't know why they fail.

I actually intend to improve this further to automatically starting a child_process
with the correct flags without any further user interaction necessary (the main
process would just end as soon as the child_process is done).
In that case we could likely improve our instrumentation as well by stop reading
the source in python and to parse the flags there.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Show resolved Hide resolved test/common/index.js Outdated
Show resolved Hide resolved test/common/index.js Outdated
Show resolved Hide resolved test/common/index.js Outdated
@richardlau

This comment has been minimized.

Copy link
Member

richardlau commented Dec 6, 2018

This is kind of edge-casey, but doesn't this preclude the ability to set the required flags in NODE_OPTIONS? (Admittedly things like --expose-internals isn't allowed, but --no-warnings is).

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 6, 2018

@richardlau I don't really worry about that. Right now all tests passed and it's always possible to change things, when necessary.

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 7, 2018

There are a couple of related test failures that I don't understand that are only triggered on a couple of machines. Could someone else have a look at them? @nodejs/build maybe someone of you could also check the difference on the machines?

parallel/test-inspector-port-zero-cluster
parallel/test-warn-sigprof
parallel/test-cli-syntax
parallel/test-cluster-dgram-bind-fd

@richardlau

This comment has been minimized.

Copy link
Member

richardlau commented Dec 7, 2018

There are a couple of related test failures that I don't understand that are only triggered on a couple of machines. Could someone else have a look at them? @nodejs/build maybe someone of you could also check the difference on the machines?

parallel/test-inspector-port-zero-cluster
parallel/test-warn-sigprof

For these two they are on builds without intl which disables the inspector. The testcase set up detects this and ignores adding the --inspect flag (as it's not a valid option in the without intl builds):

if flags_match:
flags = flags_match.group(1).strip().split()
# The following block reads config.gypi to extract the v8_enable_inspector
# value. This is done to check if the inspector is disabled in which case
# the '--inspect' flag cannot be passed to the node process as it will
# cause node to exit and report the test as failed. The use case
# is currently when Node is configured --without-ssl and the tests should
# still be runnable but skip any tests that require ssl (which includes the
# inspector related tests). Also, if there is no ssl support the options
# '--use-bundled-ca' and '--use-openssl-ca' will also cause a similar
# failure so such tests are also skipped.
if (any(flag.startswith('--inspect') for flag in flags) and
not self.context.v8_enable_inspector):
print('Skipping as node was compiled without inspector support')
elif (('--use-bundled-ca' in flags or
'--use-openssl-ca' in flags or
'--tls-v1.0' in flags or
'--tls-v1.1' in flags) and
not self.context.node_has_crypto):
print('Skipping as node was compiled without crypto support')
else:
result += flags

The tests themselves then skip if the inspector is disabled, but they don't get to that code with this PR as the flag validation bails out early:

common.skipIfInspectorDisabled();

common.skipIfInspectorDisabled();

@richardlau

This comment has been minimized.

Copy link
Member

richardlau commented Dec 7, 2018

parallel/test-cluster-dgram-bind-fd

This is a failure with workers and I can recreate this locally:

-bash-4.2$ ./tools/test.py --worker parallel/test-cluster-dgram-bind-fd
=== release test-cluster-dgram-bind-fd ===
Path: parallel/test-cluster-dgram-bind-fd
(node:73712) internal/test/binding: These APIs are exposed only for testing and are not tracked by any versioning system or deprecation process.
/home/users/riclau/sandbox/github/nodejs/test/common/index.js:72
        throw new Error(`Test has to be started with the flag: '${flag}'`);
        ^

Error: Test has to be started with the flag: '--expose-internals'
    at Object.<anonymous> (/home/users/riclau/sandbox/github/nodejs/test/common/index.js:72:15)
    at Module._compile (internal/modules/cjs/loader.js:723:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:734:10)
    at Module.load (internal/modules/cjs/loader.js:620:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
    at Function.Module._load (internal/modules/cjs/loader.js:552:3)
    at Module.require (internal/modules/cjs/loader.js:659:17)
    at require (internal/modules/cjs/helpers.js:22:18)
    at Object.<anonymous> (/home/users/riclau/sandbox/github/nodejs/test/parallel/test-cluster-dgram-bind-fd.js:3:16)
    at Module._compile (internal/modules/cjs/loader.js:723:30)
/home/users/riclau/sandbox/github/nodejs/test/common/index.js:72
        throw new Error(`Test has to be started with the flag: '${flag}'`);
        ^

Error: Test has to be started with the flag: '--expose-internals'
    at Object.<anonymous> (/home/users/riclau/sandbox/github/nodejs/test/common/index.js:72:15)
    at Module._compile (internal/modules/cjs/loader.js:723:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:734:10)
    at Module.load (internal/modules/cjs/loader.js:620:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
    at Function.Module._load (internal/modules/cjs/loader.js:552:3)
    at Module.require (internal/modules/cjs/loader.js:659:17)
    at require (internal/modules/cjs/helpers.js:22:18)
    at Object.<anonymous> (/home/users/riclau/sandbox/github/nodejs/test/parallel/test-cluster-dgram-bind-fd.js:3:16)
    at Module._compile (internal/modules/cjs/loader.js:723:30)
/home/users/riclau/sandbox/github/nodejs/test/common/index.js:72
        throw new Error(`Test has to be started with the flag: '${flag}'`);
        ^

Error: Test has to be started with the flag: '--expose-internals'
    at Object.<anonymous> (/home/users/riclau/sandbox/github/nodejs/test/common/index.js:72:15)
    at Module._compile (internal/modules/cjs/loader.js:723:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:734:10)
    at Module.load (internal/modules/cjs/loader.js:620:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
    at Function.Module._load (internal/modules/cjs/loader.js:552:3)
    at Module.require (internal/modules/cjs/loader.js:659:17)
    at require (internal/modules/cjs/helpers.js:22:18)
    at Object.<anonymous> (/home/users/riclau/sandbox/github/nodejs/test/parallel/test-cluster-dgram-bind-fd.js:3:16)
    at Module._compile (internal/modules/cjs/loader.js:723:30)
/home/users/riclau/sandbox/github/nodejs/test/common/index.js:72
        throw new Error(`Test has to be started with the flag: '${flag}'`);
        ^

Error: Test has to be started with the flag: '--expose-internals'
    at Object.<anonymous> (/home/users/riclau/sandbox/github/nodejs/test/common/index.js:72:15)
    at Module._compile (internal/modules/cjs/loader.js:723:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:734:10)
    at Module.load (internal/modules/cjs/loader.js:620:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
    at Function.Module._load (internal/modules/cjs/loader.js:552:3)
    at Module.require (internal/modules/cjs/loader.js:659:17)
    at require (internal/modules/cjs/helpers.js:22:18)
    at Object.<anonymous> (/home/users/riclau/sandbox/github/nodejs/test/parallel/test-cluster-dgram-bind-fd.js:3:16)
    at Module._compile (internal/modules/cjs/loader.js:723:30)

events.js:174
      throw er; // Unhandled 'error' event
      ^
AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

0 !== 10

    at Worker.worker.on.common.mustCall (/home/users/riclau/sandbox/github/nodejs/test/parallel/test-cluster-dgram-bind-fd.js:83:14)
    at Worker.<anonymous> (/home/users/riclau/sandbox/github/nodejs/test/common/index.js:376:15)
    at Worker.emit (events.js:189:13)
    at ChildProcess.worker.process.once (internal/cluster/master.js:192:12)
    at Object.onceWrapper (events.js:277:13)
    at ChildProcess.emit (events.js:189:13)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:254:12)
Emitted 'error' event at:
    at Worker.[kOnErrorMessage] (internal/worker.js:332:10)
    at Worker.[kOnMessage] (internal/worker.js:342:37)
    at MessagePort.Worker.(anonymous function).on (internal/worker.js:279:57)
    at MessagePort.emit (events.js:189:13)
    at MessagePort.onmessage (internal/worker.js:84:8)
Command: out/Release/node --expose-internals --experimental-worker /home/users/riclau/sandbox/github/nodejs/tools/run-worker.js /home/users/riclau/sandbox/github/nodejs/test/parallel/test-cluster-dgram-bind-fd.js
[00:00|% 100|+   0|-   1]: Done
-bash-4.2$

Instrumenting the flag validation, it looks like process.execArgv is empty, but I haven't yet worked out why.

@richardlau

This comment has been minimized.

Copy link
Member

richardlau commented Dec 7, 2018

Instrumenting the flag validation, it looks like process.execArgv is empty, but I haven't yet worked out why.

Yeah, I'm lost. cc @nodejs/workers

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 12, 2018

@richardlau since you are able to reproduce the issue locally, would you be so kind and log module and process for the failure cases? I really fail to see why it's not detected as worker.

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 12, 2018

@bnoordhuis @cjihrig I believe you two have a bit more insight into the cluster module. Do you have another idea what's going on?

@richardlau

This comment has been minimized.

Copy link
Member

richardlau commented Dec 12, 2018

@richardlau since you are able to reproduce the issue locally, would you be so kind and log module and process for the failure cases? I really fail to see why it's not detected as worker.

With this diff:

diff --git a/test/common/index.js b/test/common/index.js
index 937ecbc..08c0662 100644
--- a/test/common/index.js
+++ b/test/common/index.js
@@ -71,6 +71,8 @@ if (module.parent &&
       .replace(/_/g, '-')
       .split(' ');
     const args = process.execArgv.map((arg) => arg.replace(/_/g, '-'));
+    console.log(module);
+    console.log(process);
     for (const flag of flags) {
       if (!args.includes(flag) &&
           // If the binary is build without `intl` the inspect option is
Output ``` Module { id: '/home/users/riclau/sandbox/github/nodejs/test/common/index.js', exports: {}, parent: Module { id: '.', exports: {}, parent: null, filename: '/home/users/riclau/sandbox/github/nodejs/test/parallel/test-cluster-dgram-bind-fd.js', loaded: false, children: [ [Circular] ], paths: [ '/home/users/riclau/sandbox/github/nodejs/test/parallel/node_modules', '/home/users/riclau/sandbox/github/nodejs/test/node_modules', '/home/users/riclau/sandbox/github/nodejs/node_modules', '/home/users/riclau/sandbox/github/node_modules', '/home/users/riclau/sandbox/node_modules', '/home/users/riclau/node_modules', '/home/users/node_modules', '/home/node_modules', '/node_modules' ] }, filename: '/home/users/riclau/sandbox/github/nodejs/test/common/index.js', loaded: false, children: [ Module { id: '/home/users/riclau/sandbox/github/nodejs/test/common/tmpdir.js', exports: [Object], parent: [Circular], filename: '/home/users/riclau/sandbox/github/nodejs/test/common/tmpdir.js', loaded: true, children: [], paths: [Array] } ], paths: [ '/home/users/riclau/sandbox/github/nodejs/test/common/node_modules', '/home/users/riclau/sandbox/github/nodejs/test/node_modules', '/home/users/riclau/sandbox/github/nodejs/node_modules', '/home/users/riclau/sandbox/github/node_modules', '/home/users/riclau/sandbox/node_modules', '/home/users/riclau/node_modules', '/home/users/node_modules', '/home/node_modules', '/node_modules' ] } process { title: '/home/users/riclau/sandbox/github/nodejs/out/Release/node', version: 'v12.0.0-pre', versions: { node: '12.0.0-pre', v8: '7.1.302.28-node.5', uv: '1.24.0', zlib: '1.2.11', ares: '1.15.0', modules: '68', nghttp2: '1.34.0', napi: '3', llhttp: '1.0.1', http_parser: '2.8.0', openssl: '1.1.0j', icu: '63.1', unicode: '11.0', cldr: '34.0', tz: '2018e' }, arch: 'x64', platform: 'linux', release: { name: 'node' }, argv: [ '/home/users/riclau/sandbox/github/nodejs/out/Release/node', '/home/users/riclau/sandbox/github/nodejs/test/parallel/test-cluster-dgram-bind-fd.js' ], execArgv: [], env: { SSH_CONNECTION: '9.79.0.45 50287 9.23.44.12 22', MAIL: '/var/spool/mail/riclau', PWD: '/home/users/riclau/sandbox/github/nodejs', XDG_DATA_DIRS: '/home/users/riclau/.local/share/flatpak/exports/share/:/var/lib/flatpak/exports/share/:/usr/local/share/:/usr/share/', HISTCONTROL: 'ignoredups', HOSTNAME: 'drx-hemera', OLDPWD: '/home/users/riclau', SSH_TTY: '/dev/pts/12', CXX: '/local_1tb/sxa/gcc-7.3.0/bin/g++', NODE_OPTIONS: '', LS_COLORS: 'rs=0:di=01;34:ln=01;36:mh=00:pi=40;33:so=01;35:do=01;35:bd=40;33;01:cd=40;33;01:or=40;31;01:mi=01;05;37;41:su=37;41:sg=30;43:ca=30;41:tw=30;42:ow=34;42:st=37;44:ex=01;32:*.tar=01;31:*.tgz=01;31:*.arc=01;31:*.arj=01;31:*.taz=01;31:*.lha=01;31:*.lz4=01;31:*.lzh=01;31:*.lzma=01;31:*.tlz=01;31:*.txz=01;31:*.tzo=01;31:*.t7z=01;31:*.zip=01;31:*.z=01;31:*.Z=01;31:*.dz=01;31:*.gz=01;31:*.lrz=01;31:*.lz=01;31:*.lzo=01;31:*.xz=01;31:*.bz2=01;31:*.bz=01;31:*.tbz=01;31:*.tbz2=01;31:*.tz=01;31:*.deb=01;31:*.rpm=01;31:*.jar=01;31:*.war=01;31:*.ear=01;31:*.sar=01;31:*.rar=01;31:*.alz=01;31:*.ace=01;31:*.zoo=01;31:*.cpio=01;31:*.7z=01;31:*.rz=01;31:*.cab=01;31:*.jpg=01;35:*.jpeg=01;35:*.gif=01;35:*.bmp=01;35:*.pbm=01;35:*.pgm=01;35:*.ppm=01;35:*.tga=01;35:*.xbm=01;35:*.xpm=01;35:*.tif=01;35:*.tiff=01;35:*.png=01;35:*.svg=01;35:*.svgz=01;35:*.mng=01;35:*.pcx=01;35:*.mov=01;35:*.mpg=01;35:*.mpeg=01;35:*.m2v=01;35:*.mkv=01;35:*.webm=01;35:*.ogm=01;35:*.mp4=01;35:*.m4v=01;35:*.mp4v=01;35:*.vob=01;35:*.qt=01;35:*.nuv=01;35:*.wmv=01;35:*.asf=01;35:*.rm=01;35:*.rmvb=01;35:*.flc=01;35:*.avi=01;35:*.fli=01;35:*.flv=01;35:*.gl=01;35:*.dl=01;35:*.xcf=01;35:*.xwd=01;35:*.yuv=01;35:*.cgm=01;35:*.emf=01;35:*.axv=01;35:*.anx=01;35:*.ogv=01;35:*.ogx=01;35:*.aac=01;36:*.au=01;36:*.flac=01;36:*.mid=01;36:*.midi=01;36:*.mka=01;36:*.mp3=01;36:*.mpc=01;36:*.ogg=01;36:*.ra=01;36:*.wav=01;36:*.axa=01;36:*.oga=01;36:*.spx=01;36:*.xspf=01;36:', _: './tools/test.py', XDG_SESSION_ID: '63466', CC: '/local_1tb/sxa/gcc-7.3.0/bin/gcc', XDG_RUNTIME_DIR: '/run/user/1487', HISTSIZE: '1000', SHLVL: '1', TEST_PARALLEL: '1', SHELL: '/bin/bash', TERM: 'xterm', LANG: 'en_US.UTF-8', LD_LIBRARY_PATH: '/local_1tb/sxa/gcc-7.3.0/lib64/:', TEST_THREAD_ID: '0', HOME: '/home/users/riclau', PATH: '/usr/local/bin:/usr/bin:/usr/local/sbin:/usr/sbin', USER: 'riclau', LOGNAME: 'riclau', SSH_CLIENT: '9.79.0.45 50287 22', LESSOPEN: '||/usr/bin/lesspipe.sh %s' }, pid: 101792, features: { debug: false, uv: true, ipv6: true, tls_alpn: true, tls_sni: true, tls_ocsp: true, tls: true }, ppid: 101784, execPath: '/home/users/riclau/sandbox/github/nodejs/out/Release/node', debugPort: 9229, _debugProcess: [Function: _debugProcess], _debugEnd: [Function: _debugEnd], _startProfilerIdleNotifier: [Function: _startProfilerIdleNotifier], _stopProfilerIdleNotifier: [Function: _stopProfilerIdleNotifier], abort: [Function: abort], chdir: [Function: chdir], umask: [Function: umask], _getActiveRequests: [Function: _getActiveRequests], _getActiveHandles: [Function: _getActiveHandles], _kill: [Function: _kill], cwd: [Function: cwd], dlopen: [Function: dlopen], reallyExit: [Function: reallyExit], uptime: [Function: uptime], getuid: [Function: getuid], geteuid: [Function: geteuid], getgid: [Function: getgid], getegid: [Function: getegid], getgroups: [Function: getgroups], _rawDebug: [Function], moduleLoadList: [ 'Internal Binding module_wrap', 'Internal Binding native_module', 'Internal Binding config', 'Internal Binding worker', 'Internal Binding trace_events', 'NativeModule events', 'NativeModule internal/async_hooks', 'NativeModule internal/errors', 'Internal Binding uv', 'Internal Binding buffer', 'Internal Binding async_wrap', 'Internal Binding icu', 'NativeModule util', 'NativeModule internal/util/inspect', 'Internal Binding util', 'NativeModule internal/util', 'Internal Binding constants', 'Internal Binding types', 'NativeModule internal/util/types', 'NativeModule internal/validators', 'NativeModule internal/encoding', 'NativeModule buffer', 'NativeModule internal/buffer', 'NativeModule internal/process/per_thread', 'NativeModule internal/process/main_thread_only', 'NativeModule internal/process/stdio', 'NativeModule assert', 'NativeModule internal/assert', 'NativeModule fs', 'NativeModule path', 'NativeModule internal/constants', 'Internal Binding fs', 'NativeModule internal/fs/utils', 'NativeModule internal/url', 'NativeModule internal/querystring', 'Internal Binding url', 'NativeModule internal/process/warning', 'NativeModule internal/process/next_tick', 'NativeModule internal/process/promises', 'NativeModule internal/fixed_queue', 'Internal Binding performance', 'NativeModule internal/inspector_async_hook', 'NativeModule internal/safe_globals', 'Binding inspector', 'NativeModule internal/options', 'Internal Binding options', 'NativeModule child_process', 'Internal Binding pipe_wrap', 'NativeModule internal/child_process', 'NativeModule net', 'NativeModule stream', 'NativeModule internal/streams/pipeline', 'NativeModule internal/streams/end-of-stream', 'NativeModule internal/streams/legacy', 'NativeModule _stream_readable', 'NativeModule internal/streams/buffer_list', 'NativeModule internal/streams/destroy', 'NativeModule internal/streams/state', 'NativeModule _stream_writable', 'NativeModule _stream_duplex', 'NativeModule _stream_transform', 'NativeModule _stream_passthrough', 'NativeModule internal/net', 'Internal Binding tty_wrap', 'Internal Binding stream_wrap', 'Internal Binding tcp_wrap', 'NativeModule internal/stream_base_commons', 'NativeModule internal/timers', 'NativeModule dgram', 'NativeModule internal/dgram', 'Internal Binding udp_wrap', 'Internal Binding process_wrap', 'NativeModule internal/socket_list', 'Internal Binding spawn_sync', 'NativeModule string_decoder', 'Internal Binding string_decoder', 'NativeModule timers', 'Internal Binding timers', 'NativeModule internal/linkedlist', 'NativeModule internal/priority_queue', 'NativeModule internal/console/global', 'NativeModule internal/console/constructor', 'NativeModule internal/console/inspector', 'NativeModule internal/modules/cjs/loader', 'NativeModule vm', 'Internal Binding contextify', 'NativeModule url', 'NativeModule internal/modules/cjs/helpers', 'NativeModule cluster', 'NativeModule internal/cluster/child', 'NativeModule internal/cluster/worker', 'NativeModule internal/cluster/utils', 'NativeModule os', 'Binding os', 'NativeModule internal/fs/sync_write_stream' ], binding: [Function: binding], _linkedBinding: [Function: _linkedBinding], _events: [Object: null prototype] { newListener: [ [Function], [Function: onNewListener] ], removeListener: [ [Function], [Function: onRemoveListener] ], warning: [Function], internalMessage: [ [Function], [Function: onInternalMessage] ], error: [Function], message: [Function], disconnect: { [Function: bound onceWrapper] listener: [Function] } }, _eventsCount: 7, _maxListeners: undefined, _fatalException: [Function], domain: null, _exiting: false, assert: [Function: deprecated], config: { target_defaults: { cflags: [], default_configuration: 'Release', defines: [], include_dirs: [], libraries: [] }, variables: { asan: 0, build_v8_with_gn: false, coverage: false, debug_nghttp2: false, enable_lto: false, enable_pgo_generate: false, enable_pgo_use: false, force_dynamic_crt: 0, gas_version: '2.27', host_arch: 'x64', icu_data_in: '../../deps/icu-small/source/data/in/icudt63l.dat', icu_endianness: 'l', icu_gyp_path: 'tools/icu/icu-generic.gyp', icu_locales: 'en,root', icu_path: 'deps/icu-small', icu_small: true, icu_ver_major: '63', llvm_version: 0, node_byteorder: 'little', node_debug_lib: false, node_enable_d8: false, node_enable_v8_vtunejit: false, node_install_npm: true, node_module_version: 68, node_no_browser_globals: false, node_prefix: '/usr/local', node_release_urlbase: '', node_shared: false, node_shared_cares: false, node_shared_http_parser: false, node_shared_libuv: false, node_shared_nghttp2: false, node_shared_openssl: false, node_shared_zlib: false, node_tag: '', node_target_type: 'executable', node_use_bundled_v8: true, node_use_dtrace: false, node_use_etw: false, node_use_large_pages: false, node_use_openssl: true, node_use_pch: false, node_use_v8_platform: true, node_with_ltcg: false, node_without_node_options: false, openssl_fips: '', shlib_suffix: 'so.68', target_arch: 'x64', v8_enable_gdbjit: 0, v8_enable_i18n_support: 1, v8_enable_inspector: 1, v8_no_strict_aliasing: 1, v8_optimized_debug: 1, v8_promise_internal_field_count: 1, v8_random_seed: 0, v8_trace_maps: 0, v8_typed_array_max_size_in_heap: 0, v8_use_snapshot: true, want_separate_host_toolset: 0 } }, setUncaughtExceptionCaptureCallback: [Function], hasUncaughtExceptionCaptureCallback: [Function], emitWarning: [Function], nextTick: [Function: nextTick], _tickCallback: [Function: _tickCallback], stdout: [Getter], stderr: [Getter], stdin: [Getter], openStdin: [Function], initgroups: [Function: initgroups], setegid: [Function: setegid], seteuid: [Function: seteuid], setgid: [Function: setgid], setuid: [Function: setuid], setgroups: [Function: setgroups], hrtime: { [Function: hrtime] bigint: [Function] }, cpuUsage: [Function: cpuUsage], memoryUsage: [Function: memoryUsage], exit: [Function], kill: [Function], channel: Pipe { buffering: false, pendingHandle: null, onread: [Function], sockets: { got: {}, send: {} } }, _channel: [Getter/Setter], _handleQueue: null, _pendingMessage: null, send: [Function], _send: [Function], connected: true, disconnect: [Function], _disconnect: [Function], argv0: '/home/users/riclau/sandbox/github/nodejs/out/Release/node', allowedNodeEnvironmentFlags: [Getter/Setter], mainModule: Module { id: '.', exports: {}, parent: null, filename: '/home/users/riclau/sandbox/github/nodejs/test/parallel/test-cluster-dgram-bind-fd.js', loaded: false, children: [ [Module] ], paths: [ '/home/users/riclau/sandbox/github/nodejs/test/parallel/node_modules', '/home/users/riclau/sandbox/github/nodejs/test/node_modules', '/home/users/riclau/sandbox/github/nodejs/node_modules', '/home/users/riclau/sandbox/github/node_modules', '/home/users/riclau/sandbox/node_modules', '/home/users/riclau/node_modules', '/home/users/node_modules', '/home/node_modules', '/node_modules' ] } } ```

Full instrumented test output.

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 17, 2018

@richardlau thanks, this did indeed help me to figure out what was going on.

You passed in the --worker flag to the test runner which is going to start a specific worker JS file that receives all execArgv. That in turn starts the test file as worker. The process started as such does not have any execArgv but it does have access to the internals in case that flag was passed to the file that starts the worker.

I am not sure if this behavior is intended or if it's a bug. I personally would expect the worker to either receive the execArgv as well or that things like internals would not be exposed to the worker.

This fails in this specific bug due to the combination of cluster and workers as the worker starts the test and it is detected, that it's not the main thread. That again starts the cluster which starts children. Those children do not set the process.env.NODE_UNIQUE_ID (which might or might not be another bug [Update]: they do, but the environment variable is removed in the bootstrap process. I wanted to use the environment variable to prevent loading another module by default but it seems like that's not possible[/Update]) and the test fails. There is an easy solution: cluster.isMaster is set to false and I can check for that.

@addaleax could you please have a look at the execArgv issue here? Is that intentional?

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Dec 17, 2018

I personally would expect the worker to either receive the execArgv as well or that things like internals would not be exposed to the worker.

So … it’s tricky. One thing I’d like to implement which I was talking about with @yaelhe is the ability to provide Workers with their own execArgv, either additionally or in replacement of the main thread’s execArgv, at least as far as per-Isolate/per-Environment options are concerned… that would probably be conflicting with either of the two options you’re proposing?

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 17, 2018

@addaleax I would like to mimic the child_process or cluster behavior: using the main threads execArgs as default by passing them through explicitly (so they would show up as such). That way the user could set them to what ever they like. A consequence of that should be that e.g., internal modules should not be accessible from the worker thread in case the user decided to pass through an empty array.

Example:

// main.js
// Flags: --expose-internals

const { Worker } = require('worker_threads');
const { internalBinding } = require('internal/test/binding');
// ... do something ...

new Worker('./test.js', { execArgv: [] })
  .on('exit', (code) => process.exitCode = code);

// test.js

// This should fail due to the lack of the correct flag.
const { internalBinding } = require('internal/test/binding');

@BridgeAR BridgeAR force-pushed the BridgeAR:check-flags branch from 5a9e68f to be7ee12 Dec 18, 2018

test: verify input flags
This makes sure all required flags are passed through to the test.
If that's not the case an error is thrown to inform the user what
flag is missing.

@BridgeAR BridgeAR force-pushed the BridgeAR:check-flags branch from be7ee12 to ddc9acf Dec 18, 2018

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 18, 2018

Rebased.

@nodejs/testing PTAL

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 18, 2018

There was a force push since the last CI, so... CI: https://ci.nodejs.org/job/node-test-pull-request/19662/ ✔️

@Trott

Trott approved these changes Dec 19, 2018

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 19, 2018

@Trott ah sorry, I just updated the post above instead of adding a new comment with the other CI :D

(As a hint: when (force) pushing all checks besides up to three will be gone. So if there are more checks, a full CI was run. That can be verified by looking at the node-test-commit check which should point to the commit that was pushed last).

BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 19, 2018

test: verify input flags
This makes sure all required flags are passed through to the test.
If that's not the case an error is thrown to inform the user what
flag is missing.

PR-URL: nodejs#24876
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Dec 19, 2018

Landed in 240065f

@BridgeAR BridgeAR closed this Dec 19, 2018

MylesBorins added a commit that referenced this pull request Dec 25, 2018

test: verify input flags
This makes sure all required flags are passed through to the test.
If that's not the case an error is thrown to inform the user what
flag is missing.

PR-URL: #24876
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>

@MylesBorins MylesBorins referenced this pull request Dec 25, 2018

Merged

v11.6.0 proposal #25175

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

test: verify input flags
This makes sure all required flags are passed through to the test.
If that's not the case an error is thrown to inform the user what
flag is missing.

PR-URL: nodejs#24876
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment