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

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

Merged
merged 1 commit into from
Mar 13, 2019

Conversation

richardlau
Copy link
Member

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 8, 2019
@richardlau
Copy link
Member Author

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

@richardlau

This comment has been minimized.

@richardlau

This comment has been minimized.

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

@richardlau richardlau marked this pull request as ready for review March 11, 2019 01:06
@richardlau
Copy link
Member Author

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.

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

richardlau commented Mar 12, 2019

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.

@richardlau
Copy link
Member Author

@rvagg
Copy link
Member

rvagg commented Mar 12, 2019

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

@richardlau richardlau requested a review from joyeecheung March 12, 2019 01:45
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

RSLGTM

@richardlau richardlau added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 12, 2019
@richardlau 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 pending requests for changes, and a CI started. labels Mar 13, 2019
@richardlau
Copy link
Member Author

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

@richardlau
Copy link
Member Author

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$

@richardlau richardlau force-pushed the whitelist-bootstrap branch from 4ce459a to d568ae4 Compare March 13, 2019 19:03
@richardlau richardlau removed the wip Issues and PRs that are still a work in progress. label Mar 13, 2019
@richardlau
Copy link
Member Author

richardlau commented Mar 13, 2019

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

@richardlau richardlau added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 13, 2019
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>
@richardlau richardlau force-pushed the whitelist-bootstrap branch from d568ae4 to 0c1e93b Compare March 13, 2019 20:37
@richardlau
Copy link
Member Author

Landed in 0c1e93b.

@richardlau richardlau merged commit 0c1e93b into nodejs:master Mar 13, 2019
targos pushed a commit to targos/node that referenced this pull request Mar 27, 2019
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>
targos pushed a commit that referenced this pull request Mar 27, 2019
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>
@richardlau richardlau deleted the whitelist-bootstrap branch June 26, 2024 20:52
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.

test: make parallel/test-bootstrap-modules easier to reason about
5 participants