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.

Sign up for GitHub

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

Jump to bottom

test: fix test-bootstrap-modules to account for workers module #23876

Closed
wants to merge 221 commits into from
Closed

test: fix test-bootstrap-modules to account for workers module #23876

wants to merge 221 commits into from

Conversation

refack
Copy link
Contributor

@refack refack commented Oct 25, 2018

Fixed test-bootstrap-modules to pass CI when run with --experimental-worker.
(Example of current fail)

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

styfle and others added 30 commits October 10, 2018 23:56
@styfle @targos
doc: add WebAssembly to globals
4153114 
PR-URL: nodejs#23339
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@Trott @targos
doc: remove style instruction that is not followed
dd161ca 
Remove style guide instruction to place punctuation inside parentehses
and quotation marks. Those are not universally correct and we do not
follow those rules in cases where they are not correct.

PR-URL: nodejs#23346
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
@Trott @targos
doc: remove personal pronoun from domain.md
d61cb65 
Per the Style Guide, remove the use of the personal pronoun _you_ from
the formal API documentation for `domain`.

PR-URL: nodejs#23347
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
@Trott @targos
doc: remove personal pronoun from worker_threads
080b015 
Per the Style Guide, remove the use of the personal pronoun _you_ from
the formal API documentation for `worker_threads`.

PR-URL: nodejs#23347
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
@Trott @targos
doc: edit for minor fixes to prcoess.md
2f17cdb 
Remove use of personal pronoun _you_ from formal documentation for
`process`. Fix up grammar in nearby text.

PR-URL: nodejs#23347
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
@danbev @targos
test: fix compiler warning in doc/api/addons.md
ebf04c1 
Currently there are compiler warnings is emitted:
../addon.cc:17:58:
warning: 'ToString' is deprecated:
Use maybe version [-Wdeprecated-declarations]
obj->Set(String::NewFromUtf8(isolate, "msg"),
                             args[0]->ToString(isolate));
                                                         ^
deps/v8/include/v8.h:2537:3:
note: 'ToString' has been explicitly marked deprecated here
  V8_DEPRECATED("Use maybe version",
  ^

This commit updates examples to use the non-deprecated versions.

PR-URL: nodejs#23323
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@danbev @targos
http2: make Http2Settings constructors delegate
907ebd2 
This commit extracts the common code in the existing Http2Settings
constructors into a private constructor, allowing the existing ones to
delegate to the private constructor it and avoid code duplication.

PR-URL: nodejs#23326
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@conectado @targos
test: add test for a vm indexed property
e34761f 
Adds a single test for a vm with an indexed property.

PR-URL: nodejs#23318
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@targos
src: fix usage of deprecated v8::Date::New
8c04c37 
PR-URL: nodejs#23288
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos
vm: pass parsing_context to ScriptCompiler::CompileFunctionInContext
8624cba 
ContextifyContext::CompileFunction in src/node_contextify.cc
was incorrectly passing the context variable to
ScriptCompiler::CompileFunctionInContext

This meant that the parsingContext option in vm.compileFunction
was not being applied properly to the compiled function.

fixes: nodejs#23194

doc: clarify parsingContext option for vm.compileScript

test: usage of parsingContext in vm.compileFunction

PR-URL: nodejs#23206
Fixes: nodejs#23194
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@Trott @targos
test: add logging to test-worker-memory
b279b96 
To help troubleshoot CI failures, add some logging.

Refs: nodejs#23277 (comment)

PR-URL: nodejs#23418
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@Trott @targos
doc: rewrite consensus seeking in guide
650e924 
Rewrite the Consensus Seeking section of the Collaborators Guide for
easier reading, more clarity, shorter sentences, less passive voice,
etc.

PR-URL: nodejs#23349
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@refack @targos
src: replace NO_RETURN with [[noreturn]]
772709c 
PR-URL: nodejs#23337
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@Trott @targos
tools: update node-lint-md-cli-rollup
5c9ef5e 
Update node-lint-md-cli-rollup. This silences some minor warnings that
appear with `npm audit`. It also updates remark-preset-lint-node to
1.0.3 (from 1.0.2).

PR-URL: nodejs#23358
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@refack @targos
tools,icu: read full ICU version info from file
9cc73d3 
* ATM on every ICU version bump we need to update these data. Reading
  it from a file makes it independant of `configre.py` changes.
* Update guide.

PR-URL: nodejs#23269
Refs: nodejs#23245
Reviewed-By: Steven R Loomis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@Trott @targos
test: rename WPT console test
cdba84e 
test-console-is-a-namespace.js is a WPT Console test. It is the only
WPT Console test file that we have that doesn't start with
`test-whatwg-`. Rename it to test-whatwg-console-is-a-namespace.js
so that WPT tests can be found relatively easily. Just as it is useful
to separate the WPT URL tests from our URL tests, it will likely be
useful to separate our Console tests from WPT Console tests if we add
more WPT Console tests (which I hope we do).

PR-URL: nodejs#23340
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@Trott @targos
test: add WPT console-label-conversion test
9efe84e 
Add console-label-conversion.any.js from WPT to the test suite.

PR-URL: nodejs#23340
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@Trott @targos
test: separate WPT console test from other test
3a5887f 
Remove one test from test-whatwg-console-is-a-namespace that is not part
of the WPT test. Put it in test-console-self-assign.

PR-URL: nodejs#23340
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@danbev @addaleax
src: remove unused limits header from util-inl.h
cea396a 
This commit removes the inclusion of limits.h which was introduced in
commit e812be4 ("src: make CLI options
programatically accesible"), but as far as I can see it was not used
there either so it look like it can safely be removed.

PR-URL: nodejs#23353
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@oyyd @targos
tls: make StreamWrap work correctly in "drain" callback
3721f14 
When an instance of StreamWrap is shutting down and a "drain" event
is emitted, the instance will abort as its
`this[kCurrentShutdownRequest]` is already set. The following test
will fail before this commit.

PR-URL: nodejs#23294
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@dyatlov @targos
src: revert removal of SecureContext _external getter
f681691 
This `_external` getter is essential for some libs to work:
uWebSockets as an example.

PR-URL: nodejs#21711
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@sagitsofan @targos
lib: http server, friendly error messages
efe4f5e 
Improved error message description for the http server binding errors.

Currently changed only in `setupListenHandle`, but needs to be
change all over.

Added new `uvExceptionWithHostPort` function (+export) in
`lib/internal/error.js` that extracts the error message defined by
libuv, using the error code, and returns an error object with the
full error description.

example:
old error message: `listen EADDRINUSE`
new error message: `listen EADDRINUSE: Address already in use`

Removed exportable function `_exceptionWithHostPort` from
`lib/util.js` - exported by accident

Replaced `exceptionWithHostPort` to the new function
`uvExceptionWithHostPort` for a more detailed error.

Fixes: nodejs#22936

PR-URL: nodejs#22995
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@gireeshpunathil @targos
src: improve SSL version extraction logic
05108eb 
The openssl version as defined in ssl libraries is complex.
The current logic to extract the major.minor.patch format
uses C semantics to loop through the text and search for
specific patterns. Use C++ string to tidy it up.

PR-URL: nodejs#23050
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@tniessen @targos
src: fix bug in MallocedBuffer constructor
678019c 
This should be a generic type even though we are
currently only using `char` as `T`.

PR-URL: nodejs#23434
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott @targos
doc: revise security-reporting text in README
dd3dd5f 
Simplify and clarify the security-reporting text in the README. Now is
also probably a good time to ping the security triage folks to make sure
the text is still accurate.

PR-URL: nodejs#23407
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@joesepi @targos
test: fixing assertion value order
d025420 
PR-URL: nodejs#23574
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@et4891 @targos
test: fix assert.strictEqual argument order
243c227 
PR-URL: nodejs#23518
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@lundibundi @targos
test: improve test-gc-http-client-connaborted
9197a72 
* refactor out usage of 'function' for scoping
* wait till server is up to start firing requests

PR-URL: nodejs#23193
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@lundibundi @targos
test: improve test-gc-http-client-onerror
e402b22 
* refactor out usage of 'function' for scoping
* inline runTest function

PR-URL: nodejs#23196
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@targos
test: move some gc tests back to parallel/, unmark flaky
dc52a51 
These should no longer be flaky after the libuv update.

Refs: nodejs#23336

PR-URL: nodejs#23356
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax
Copy link
Member

@refack … If we do it this way, I’d prefer to land it on master. We update the test file frequently, and it would get somewhat annoying to have a merge conflict every time we want a commit that touches it backported to v10.x?

@addaleax addaleax mentioned this pull request Oct 25, 2018
Closed
3 tasks
jasnell
jasnell approved these changes Oct 25, 2018
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM for landing in master.

fwiw, I really don't like this test.

@refack refack mentioned this pull request Oct 25, 2018
Closed
2 tasks
@refack
Copy link
Contributor Author

refack commented Oct 25, 2018

If we do it this way, I’d prefer to land it on master. We update the test file frequently, and it would get somewhat annoying to have a merge conflict every time we want a commit that touches it backported to v10.x?

I was rebasing but couldn't get a consistent result when it hit me. The + 1 side effect doesn't happen on master. The worker suite already passes.

So this PR is all we need.

@richardlau
Copy link
Member

On master the magic constant is less (77) than on v10.x (78):

node/test/parallel/test-bootstrap-modules.js

Line 14 in 1523111

assert(list.length <= 77, list);

I think I remember reading somewhere that an extra module is loaded if the test is run throught the test runner as opposed to running directly on the command line.

@refack
Copy link
Contributor Author

refack commented Oct 25, 2018

On master the magic constant is less (77) than on v10.x (78):

But it stay the same (77) whether run with --experimental-worker tools/run-worker.js or not.
It does go to 86 if run just with --experimental-worker (without the runner), but that's not how (or what) we test in the worker suite.

@devsnek
Copy link
Member

devsnek commented Oct 25, 2018

what if we just removed this test... it's intention is undermined by the fact that no human can reason with its behaviour.

perhaps we can set up reports from the CI or add something informative to the test runner output?

@addaleax
Copy link
Member

Yeah, I’d also be totally okay with removing that test, but I don’t think that would go through as a fast-track PR to unbreak something (we’ve discussed this in the past).

It might be a better idea to blacklist some modules that we know we don’t want to have loaded unconditionally, rather than set a fixed number.

@jasnell
Copy link
Member

jasnell commented Oct 25, 2018

One key issue with test is that the number of modules loaded changes based on how the test is run, making it extremely flaky.

@refack
Copy link
Contributor Author

refack commented Oct 25, 2018

what if we just removed this test... it's intention is undermined by the fact that no human can reason with its behaviour.

perhaps we can set up reports from the CI or add something informative to the test runner output?

IMHO the test has merit, it's not flaky it is deterministic, it just depend on how you run it. If we run it the same way, we get it's benefit.
@BridgeAR has been working hard to keep the bootstrap loaded module count low. I've also seen others who place bootstrap time in high priority.
https://github.com/nodejs/node/pull/23081/files/56c2099197e08f46d4bb4e021b40955dd8663d95#diff-b35754f7aa079e299b8ac8c51ea88959
#23050 (comment)

P.S. On Windows bootstrap time is much worse.

@devsnek
Copy link
Member

devsnek commented Oct 25, 2018

@refack i'm not saying that bootstrap module count is unimportant, in fact i quite agree that it is important. i just don't think this test - in its current form - is the most helpful way to enforce the count, and just because it's deterministic doesn't mean it's easy to understand or maintain.

i suggested changing from a test to a non-fatal reporting mechanism, and anna recommended keeping a list of modules we expect to exist or not exist. personally i like anna's idea, i think keeping a big list would provide reasonable verbosity (which modules should pass) and verbosity (it would be clear which module doesn't belong thanks to @BridgeAR's array diffing)

@refack
Copy link
Contributor Author

refack commented Oct 25, 2018 •
edited
Loading

@devsnek big 👍, but that's scope creep.
This PR is just a band-aid to get the CI green again 🤷‍♂️

@refack refack mentioned this pull request Oct 25, 2018
Closed
@refack
Copy link
Contributor Author

refack commented Oct 25, 2018

P.S. #23884

@richardlau richardlau mentioned this pull request Oct 25, 2018
Closed
3 tasks
@refack refack added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 29, 2018
@Trott
Copy link
Member

Trott commented Nov 2, 2018

Since this is for 10.x and that's now an LTS branch, I guess a member of @nodejs/releasers would have to land this? (Also: needs a rebase?)

@refack
Copy link
Contributor Author

refack commented Nov 2, 2018

IIUC the underlying issue was resolved so I'm abandoning this.

@refack refack closed this Nov 2, 2018
@refack refack added abandoned and removed flaky-test Issues and PRs related to the tests with unstable failures on the CI. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. labels Nov 2, 2018
@refack refack deleted the fix-bootstrap-test branch November 2, 2018 02:52
@refack refack changed the title test: fix test-bootstrap-modules for account for workers test: fix test-bootstrap-modules to account for workers module Nov 4, 2018
@Trott Trott mentioned this pull request Dec 14, 2018
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@BridgeAR BridgeAR Awaiting requested review from BridgeAR

@targos targos Awaiting requested review from targos

Assignees
No one assigned
Labels
test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.