test: whitelist the expected modules in test-bootstrap-modules.js by richardlau · Pull Request #26531 · nodejs/node · GitHub

Be explicit on the modules that are expected to be found and add an
appropriate assertion failure message to help debug when the list
changes.

Fixes: #23884

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

@richardlau build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2823/pipeline

This is a draft because I highly suspect it will fail --without-ssl and --without-intl builds and I won't be able to work on this further until after the weekend.

The list(s) of modules are what is currently there at the moment (basically I kept running with and without worker threads and added the modules shown in the assertion failure message until the test passed).

(As an aside I found the default assert.deepStrictEqual() message unhelpful for sets because it didn't show all the differences in one go.)

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

Pushed a small fixup. New CI is yellow: https://ci.nodejs.org/job/node-test-pull-request/21416/

This should be good to go, pending reviews.

Forced pushed to fix after recent changes (#26523 and #26513 (not shown below as Travis doesn't run coverage)) changed the list:

https://travis-ci.com/nodejs/node/jobs/184020354#L5847-L5868

=== release test-bootstrap-modules ===
Path: parallel/test-bootstrap-modules
assert.js:85
  throw new AssertionError(obj);
  ^
AssertionError [ERR_ASSERTION]: These modules were not loaded:
  NativeModule async_hooks,
  NativeModule internal/process/next_tick,
  NativeModule internal/queue_microtask
These modules were unexpectedly loaded:
  NativeModule internal/process/task_queues
    at Object.<anonymous> (/home/travis/build/nodejs/node/test/parallel/test-bootstrap-modules.js:126:8)
    at Module._compile (internal/modules/cjs/loader.js:813:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:824:10)
    at Module.load (internal/modules/cjs/loader.js:680:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:612:12)
    at Function.Module._load (internal/modules/cjs/loader.js:604:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:876:12)
    at internal/main/run_main_module.js:21:11
Command: out/Release/node --expose-internals /home/travis/build/nodejs/node/test/parallel/test-bootstrap-modules.js

Note that #26523 actually reduced the number of modules loaded (🎉), but didn't reduce the count in the version of test-bootstrap-modules.js prior to this PR.

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

Sorry, I was reprovisioning a smartos17 machine when this was running. Submitted a resume @ https://ci.nodejs.org/job/node-test-commit-smartos/24405/

RSLGTM

Need to adjust the test for #26501 (comment).

For posterity, these are the adjustments required:

-bash-4.2$ ./tools/test.py --worker test/parallel/test-bootstrap-modules.js
=== release test-bootstrap-modules ===
Path: parallel/test-bootstrap-modules
events.js:174
      throw er; // Unhandled 'error' event
      ^
AssertionError [ERR_ASSERTION]: These modules were unexpectedly loaded:
  Internal Binding heap_utils,
  Internal Binding stream_wrap,
  Internal Binding uv,
  NativeModule internal/stream_base_commons

    at Object.<anonymous> (/home/users/riclau/sandbox/github/nodejs/test/parallel/test-bootstrap-modules.js:123:8)
    at Module._compile (internal/modules/cjs/loader.js:813:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:824:10)
    at Module.load (internal/modules/cjs/loader.js:680:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:612:12)
    at Function.Module._load (internal/modules/cjs/loader.js:604:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:876:12)
    at MessagePort.port.on (internal/main/worker_thread.js:119:25)
    at MessagePort.emit (events.js:198:13)
    at MessagePort.onmessage (internal/worker/io.js:69:8)
Emitted 'error' event at:
    at Worker.[kOnErrorMessage] (internal/worker.js:147:10)
    at Worker.[kOnMessage] (internal/worker.js:157:37)
    at MessagePort.Worker.(anonymous function).on (internal/worker.js:90:57)
    at MessagePort.emit (events.js:198:13)
    at MessagePort.onmessage (internal/worker/io.js:69:8)
Command: out/Release/node --expose-internals /home/users/riclau/sandbox/github/nodejs/tools/run-worker.js /home/users/riclau/sandbox/github/nodejs/test/parallel/test-bootstrap-modules.js
[00:00|% 100|+   0|-   1]: Done
-bash-4.2$

New CI: https://ci.nodejs.org/job/node-test-pull-request/21512/ (✔️)

Could I get another review? This test is obviously sensitive to bootstrap refactorings that have been going on and I have had to adjust the lists of modules in the test twice now as other PRs have landed (arguably those PRs should have been adjusting the test with or without the changes in this PR as they affected which modules are loaded by the bootstrap process).

Be explicit on the modules that are expected to be loaded and add an appropriate assertion failure message to help debug when the list changes. Fixes: nodejs#23884 PR-URL: nodejs#26531 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Landed in 0c1e93b.

Be explicit on the modules that are expected to be loaded and add an appropriate assertion failure message to help debug when the list changes. Fixes: nodejs#23884 PR-URL: nodejs#26531 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

Be explicit on the modules that are expected to be loaded and add an appropriate assertion failure message to help debug when the list changes. Fixes: #23884 PR-URL: #26531 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 8, 2019

richardlau commented Mar 8, 2019

View reviewed changes

Comment thread test/parallel/test-bootstrap-modules.js Outdated

This comment has been minimized.

Sign in to view

richardlau force-pushed the whitelist-bootstrap branch from ede623a to 0cefbac Compare March 11, 2019 01:04

richardlau marked this pull request as ready for review March 11, 2019 01:06

richardlau force-pushed the whitelist-bootstrap branch 2 times, most recently from 4c06af2 to 4ce459a Compare March 12, 2019 00:14

richardlau requested a review from joyeecheung March 12, 2019 01:45

refack approved these changes Mar 12, 2019

View reviewed changes

richardlau added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 12, 2019

richardlau mentioned this pull request Mar 12, 2019

util: integrate node-heapdump into core #26501

Closed

4 tasks

richardlau added wip Issues and PRs that are still a work in progress. and removed author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Mar 13, 2019

richardlau force-pushed the whitelist-bootstrap branch from 4ce459a to d568ae4 Compare March 13, 2019 19:03

richardlau removed the wip Issues and PRs that are still a work in progress. label Mar 13, 2019

richardlau added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 13, 2019

sam-github approved these changes Mar 13, 2019

View reviewed changes

richardlau force-pushed the whitelist-bootstrap branch from d568ae4 to 0c1e93b Compare March 13, 2019 20:37

richardlau merged commit 0c1e93b into nodejs:master Mar 13, 2019

targos mentioned this pull request Mar 27, 2019

v11.13.0 release proposal #26949

Merged

richardlau mentioned this pull request Mar 28, 2019

test: simplify test-async-hooks-http-parser-destroy #26963

Closed

2 tasks

richardlau deleted the whitelist-bootstrap branch June 26, 2024 20:52

test: whitelist the expected modules in test-bootstrap-modules.js by richardlau · Pull Request #26531 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: whitelist the expected modules in test-bootstrap-modules.js - #26531

test: whitelist the expected modules in test-bootstrap-modules.js#26531richardlau merged 1 commit intonodejs:masternodejs/node:masterfrom richardlau:whitelist-bootstraprichardlau/node-1:whitelist-bootstrapCopy head branch name to clipboard

Conversation

richardlau commented Mar 8, 2019

Checklist

Uh oh!

nodejs-github-bot commented Mar 8, 2019

Uh oh!

richardlau commented Mar 8, 2019

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

richardlau commented Mar 11, 2019

Uh oh!

richardlau commented Mar 11, 2019

Uh oh!

richardlau commented Mar 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardlau commented Mar 12, 2019

Uh oh!

rvagg commented Mar 12, 2019

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

richardlau commented Mar 13, 2019

Uh oh!

richardlau commented Mar 13, 2019

Uh oh!

richardlau commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardlau commented Mar 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

test: whitelist the expected modules in test-bootstrap-modules.js#26531
richardlau merged 1 commit into
nodejs:masterfrom
richardlau:whitelist-bootstrap

richardlau commented Mar 12, 2019 •
edited
Loading

richardlau commented Mar 13, 2019 •
edited
Loading