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

[v10.x] test: mark test-repl-envvars as flaky on AIX #25219

Closed

Conversation

MylesBorins
Copy link
Contributor

Refs: #21451
Refs: nodejs/build#1377

/cc @nodejs/platform-aix

chichiwang and others added 30 commits December 13, 2018 09:39
also... return TextDecoder directly from factories

PR-URL: nodejs#23625
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
The fixed-size buffer on the stack is unnecessary and way too large
for most applications. This change removes it and allocates the
required memory directly instead of copying into heap later.

PR-URL: nodejs#23427
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
PR-URL: nodejs#23734
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Introduced use of smart pointers instead of MallocedBuffer to manage
memory allocated in the cares library.

PR-URL: nodejs#23628
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Picking a path according to a boolean is essentially free,
compared to the cost of a function call. Also, remove an
unnecessary `TryCatch`.

PR-URL: nodejs#23782
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
The convertProtocols() function now throws a range error when the byte
length of a protocol is too long to fit in a Buffer.

Also added a test case in test/parallel/test-tls-basic-validations.js
to cover this.

PR-URL: nodejs#23606
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Using `process.nextTick()` or `Promise`, it
is possible to escape the `timeout` set when running code with
`vm.runInContext()`, `vm.runInThisContext()`, and
`vm.runInNewContext()`.

This documents the issue and adds two known_issues tests.

Refs: nodejs#3020
PR-URL: nodejs#23743
Refs: nodejs#3020
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
These are known issues that can be flaky on certain platforms
because they rely entirely on timing differences.

PR-URL: nodejs#23743
Refs: nodejs#3020
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Those categories are necessary to build addons that depends
 on libcurl and libssh, the following were the missing symbols:
libcurl:
OCSP_cert_status_str
OCSP_check_validity
OCSP_basic_verify
OCSP_RESPONSE_free
OCSP_single_get0_status
OCSP_response_get1_basic
OCSP_BASICRESP_free
OCSP_crl_reason_str
OCSP_resp_count
OCSP_response_status
OCSP_response_status_str
OCSP_resp_get0
d2i_OCSP_RESPONSE
SSL_CTX_set_next_proto_select_cb

libssh:
EVP_ripemd160
EVP_cast5_cbc

Fixes: nodejs#23293

PR-URL: nodejs#23344
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Instead of getting a reference to the main `AliasedBuffer`, which
would always unnecesarily allocate and destroy a `Persistent`
handle, store and use a reference to the owning object.

PR-URL: nodejs#23844
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Instead of calling into C++ each time we need to check the value
of a command line option, cache the option map in a new
`internal/options` module for faster access to the values in JS land.

PR-URL: nodejs#24091
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Right now `node -p process.versions.openssl` always gets run
in the Makefile even when it's not needed by the target
(e.g. `make clean`, `make test-only`). This patch makes it
a run time call instead of part of the global expansion.

PR-URL: nodejs#24115
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
PR-URL: nodejs#24130
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
PR-URL: nodejs#24131
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Lazy load _http_common and HTTPParser so that the 'http_parser' binding
can be monkey patched before any internal modules require it. This also
probably improves startup performance minimally for programs that never
require the HTTP stack.

Fixes: nodejs#23716
Fixes: creationix/http-parser-js#57

PR-URL: nodejs#24006
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: nodejs#24133
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Swapped the actual and expected arguments in `assert.strictEqual()`
calls.  Arguments are now in correct order.
Literal value is now the second argument and the value returned by the
function is the first argument.

PR-URL: nodejs#24134
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
PR-URL: nodejs#24137
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
`assert.strictEqual` expects arguments in the following order:

actual, expected[, message]

PR-URL: nodejs#24138
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
PR-URL: nodejs#24142
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Switched arguments in assert.strictEqual()

PR-URL: nodejs#24144
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: nodejs#24146
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
These tests ensure that OutgoingMessage setTimeout method
will call setTimeout on its socket

Co-authored-by: ZauberNerd <zaubernerd@zaubernerd.de>

PR-URL: nodejs#24148
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: nodejs#24152
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
File: test/parallel/test-http-client-upload-buf.js

PR-URL: nodejs#24140
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
the actual and expected arguments in assert.strictEqual() calls are in
the wrong order. Any literal value should be the second argument while
the first argument should be the value returned by a function/be the
calculated value.

PR-URL: nodejs#24151
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
In the `test/parallel/test-vm-create-and-run-in-context.js`
test the actual and expected arguments in the `assert.strictEqual()`
call on line 32 are in the wrong order and they have to be switched
around.

PR-URL: nodejs#24141
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
PR-URL: nodejs#24147
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
PR-URL: nodejs#24155
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: nodejs#24213
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
cjihrig and others added 7 commits December 21, 2018 17:21
This commit adds http_parser_set_max_header_size() to the
http-parser for overriding the compile time maximum HTTP
header size.

Backport-PR-URL: nodejs#25168
PR-URL: nodejs#24811
Fixes: nodejs#24692
Refs: nodejs/http-parser#453
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
This commit adds support for uint64_t option parsing.

Backport-PR-URL: nodejs#25168
PR-URL: nodejs#24811
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Allow the maximum size of HTTP headers to be overridden from
the command line.

Backport-PR-URL: nodejs#25168
co-authored-by: Matteo Collina <hello@matteocollina.com>
PR-URL: nodejs#24811
Fixes: nodejs#24692
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Remove magic numbers (500, 10, 100) from the test. Instead, detect when
GC has started and stop sending requests at that point.

On my laptop, this results in 16 or 20 requests per run instead of 500.

Fixes: nodejs#23089

PR-URL: nodejs#24943
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Document that the limit was changed from 80KB to 8KB in 1860352.

Fixes: nodejs#24693

PR-URL: nodejs#24700
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
This commit exposes the value of --max-http-header-size
as a property of the http module.

PR-URL: nodejs#24860
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. v10.x labels Dec 26, 2018
@MylesBorins
Copy link
Contributor Author

MylesBorins commented Dec 26, 2018

@Trott
Copy link
Member

Trott commented Dec 26, 2018

@addaleax addaleax changed the title test: mark test-repl-envvars as flaky on AIX [v10.x] test: mark test-repl-envvars as flaky on AIX Dec 26, 2018
addaleax added a commit to addaleax/node that referenced this pull request Dec 26, 2018
In 180f865, the test was changed
so that the `env` argument of `createInternalRepl()` also contained
external environment variables, because keeping them can be necessary
for spawning processes on some systems.

However, this test does not spawn new processes, and relies on the
fact that the environment variables it tests are not already set
(and fails otherwise); therefore, reverting to the original state
should fix this.

Fixes: nodejs#21451
Fixes: nodejs/build#1377
Refs: nodejs#25219
@addaleax addaleax mentioned this pull request Dec 26, 2018
3 tasks
@addaleax
Copy link
Member

@MylesBorins Are these failures permanent (on a single machine) or is it more of a works-sometimes things?

I think the changes 180f865 made to this particular file broke it, and reverting them should be a clean way to resolve this. I’ve opened #25226 to do that.

Trott pushed a commit to Trott/io.js that referenced this pull request Dec 27, 2018
In 180f865, the test was changed
so that the `env` argument of `createInternalRepl()` also contained
external environment variables, because keeping them can be necessary
for spawning processes on some systems.

However, this test does not spawn new processes, and relies on the
fact that the environment variables it tests are not already set
(and fails otherwise); therefore, reverting to the original state
should fix this.

Fixes: nodejs#21451
Fixes: nodejs/build#1377
Refs: nodejs#25219

PR-URL: nodejs#25226
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
addaleax added a commit that referenced this pull request Dec 30, 2018
In 180f865, the test was changed
so that the `env` argument of `createInternalRepl()` also contained
external environment variables, because keeping them can be necessary
for spawning processes on some systems.

However, this test does not spawn new processes, and relies on the
fact that the environment variables it tests are not already set
(and fails otherwise); therefore, reverting to the original state
should fix this.

Fixes: #21451
Fixes: nodejs/build#1377
Refs: #25219

PR-URL: #25226
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
addaleax added a commit that referenced this pull request Dec 30, 2018
In 180f865, the test was changed
so that the `env` argument of `createInternalRepl()` also contained
external environment variables, because keeping them can be necessary
for spawning processes on some systems.

However, this test does not spawn new processes, and relies on the
fact that the environment variables it tests are not already set
(and fails otherwise); therefore, reverting to the original state
should fix this.

Fixes: #21451
Fixes: nodejs/build#1377
Refs: #25219

PR-URL: #25226
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
addaleax added a commit that referenced this pull request Dec 30, 2018
In 180f865, the test was changed
so that the `env` argument of `createInternalRepl()` also contained
external environment variables, because keeping them can be necessary
for spawning processes on some systems.

However, this test does not spawn new processes, and relies on the
fact that the environment variables it tests are not already set
(and fails otherwise); therefore, reverting to the original state
should fix this.

Fixes: #21451
Fixes: nodejs/build#1377
Refs: #25219

PR-URL: #25226
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
addaleax added a commit that referenced this pull request Dec 30, 2018
In 180f865, the test was changed
so that the `env` argument of `createInternalRepl()` also contained
external environment variables, because keeping them can be necessary
for spawning processes on some systems.

However, this test does not spawn new processes, and relies on the
fact that the environment variables it tests are not already set
(and fails otherwise); therefore, reverting to the original state
should fix this.

Fixes: #21451
Fixes: nodejs/build#1377
Refs: #25219

PR-URL: #25226
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Jan 2, 2019
In 180f865, the test was changed
so that the `env` argument of `createInternalRepl()` also contained
external environment variables, because keeping them can be necessary
for spawning processes on some systems.

However, this test does not spawn new processes, and relies on the
fact that the environment variables it tests are not already set
(and fails otherwise); therefore, reverting to the original state
should fix this.

Fixes: nodejs#21451
Fixes: nodejs/build#1377
Refs: nodejs#25219

PR-URL: nodejs#25226
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins
Copy link
Contributor Author

Closing as it looks like test has been fixed on all branches

@MylesBorins MylesBorins closed this Jan 3, 2019
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
In 180f865, the test was changed
so that the `env` argument of `createInternalRepl()` also contained
external environment variables, because keeping them can be necessary
for spawning processes on some systems.

However, this test does not spawn new processes, and relies on the
fact that the environment variables it tests are not already set
(and fails otherwise); therefore, reverting to the original state
should fix this.

Fixes: nodejs#21451
Fixes: nodejs/build#1377
Refs: nodejs#25219

PR-URL: nodejs#25226
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
In 180f865, the test was changed
so that the `env` argument of `createInternalRepl()` also contained
external environment variables, because keeping them can be necessary
for spawning processes on some systems.

However, this test does not spawn new processes, and relies on the
fact that the environment variables it tests are not already set
(and fails otherwise); therefore, reverting to the original state
should fix this.

Fixes: #21451
Fixes: nodejs/build#1377
Refs: #25219

PR-URL: #25226
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
In 180f865, the test was changed
so that the `env` argument of `createInternalRepl()` also contained
external environment variables, because keeping them can be necessary
for spawning processes on some systems.

However, this test does not spawn new processes, and relies on the
fact that the environment variables it tests are not already set
(and fails otherwise); therefore, reverting to the original state
should fix this.

Fixes: #21451
Fixes: nodejs/build#1377
Refs: #25219

PR-URL: #25226
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BethGriggs pushed a commit that referenced this pull request Mar 7, 2019
In 180f865, the test was changed
so that the `env` argument of `createInternalRepl()` also contained
external environment variables, because keeping them can be necessary
for spawning processes on some systems.

However, this test does not spawn new processes, and relies on the
fact that the environment variables it tests are not already set
(and fails otherwise); therefore, reverting to the original state
should fix this.

Fixes: #21451
Fixes: nodejs/build#1377
Refs: #25219

PR-URL: #25226
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.