test: Improvement for testing of execSync #9133
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PR-URL: nodejs#8588 Reviewed-By: Yosuke Furukawa <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
Make it clear that atime and mtime should be in seconds. PR-URL: nodejs#8651 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Ilkka Myller <[email protected]>
PR-URL: nodejs#8628 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Changed var to const PR-URL: nodejs#8599 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ilkka Myller <[email protected]>
Fixes: nodejs#8426 PR-URL: nodejs#8502 Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
PR-URL: nodejs#8578 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ilkka Myller <[email protected]>
Changed vars to consts and lets, assert.equals to assert.strictEquals and added common.mustCall around callbacks. Switched to arrow functions. PR-URL: nodejs#8616 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ilkka Myller <[email protected]>
Replace equal with strictEqual, use const instead of var and improve test with use of assert.notStrictEqual PR-URL: nodejs#8600 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ilkka Myller <[email protected]>
Moves inflateSetDictionary right after inflateInit2 when mode is INFLATERAW, since without the wrapper in appears zlib won't return Z_NEED_DICT as it would otherwise, and will thus attempt inflating without the dictionary, leading to an error. Fixes: nodejs#8507 PR-URL: nodejs#8512 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Uses const where possible, removes inaccurate comments, prefers strictEqual where possible, ensures functions with assertions are called and enures the inflater has correct encoding set. PR-URL: nodejs#8512 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Changed vars to const / let, functions to arrow functions and a mustCall where appropriate. PR-URL: nodejs#8581 Reviewed-By: Robert Jefe Lindstaedt <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Robert Jefe Lindstaedt <[email protected]>
change var to const/let where appropriate use strictEqual instead of equal call toString on buffers in strictEqual asserts use common.mustCall on callbacks containing asserts PR-URL: nodejs#8648 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
PR-URL: nodejs#8622 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yorkie Liu <[email protected]> Reviewed-By: Jackson Tian <[email protected]> Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Teddy Katz <[email protected]>
This commit prevents thrown JavaScript exceptions from crashing the process in node_contextify's CopyProperties() function. Fixes: nodejs#8537 PR-URL: nodejs#8649 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Refactored the code to latest standards, where all var is changed to const, functions are changed to arrow functions and assert.equal chaned to assert.strictEqual PR-URL: nodejs#8582 Reviewed-By: Robert Jefe Lindstaedt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ilkka Myller <[email protected]>
PR-URL: nodejs#8620 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jackson Tian <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
This commit adds better handling of exceptional array formats passed to dns.setServers(). Prior to this commit, the input array was validated using map(), which preserves holes, allowing them to be passed to c-ares, crashing Node. This commit replaces map() with forEach(), which skips holes. Fixes: nodejs#8538 PR-URL: nodejs#8567 Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#8676 Reviewed-By: Rich Trott <[email protected]>
If calling `https.request()` with `options.headers.host` defined and `options.servername` undefined, `https.Agent.createSocket` mutates connection `options` after `https.Agent.addRequest` has created empty socket pool array with mismatching connection name. This results in two socket pool arrays being created and only the last one gets eventually deleted by `removeSocket` - causing a memory leak. This commit fixes the leak by making sure that `addRequest` does the same modifications to `options` object as the `createSocket`. `createSocket` is intentionally left unmodified to prevent userland regressions. Test case included. PR-URL: nodejs#8647 Fixes: nodejs#6687 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jackson Tian <[email protected]> Reviewed-By: James M Snell <[email protected]>
Moved from var to const. Moved from .equal to .strictEqual. Added more checks about the type of the returned values. PR-URL: nodejs#8606 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ilkka Myller <[email protected]>
PR-URL: nodejs#8643 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#8696 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Specify the plugin name as 'remark-lint/', as new remark-cli refused to find it when cwd is different from the directory where node_modules are put. Trailing slash fixes this, as the plugin name gets treated as a path, so `require` works on it. Explicitly specify the list of rules we want to enable, as the new remark-lint is opt-in and doesn't come with any rules by default. Reorder the rules alphabetically. PR-URL: nodejs#8666 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Change `Malloc()/Calloc()` so that size zero does not return a null pointer, consistent with prior behavior. Fixes: nodejs#8571 PR-URL: nodejs#8572 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yorkie Liu <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Pick up latest commit from the 5.4-lkgr branch. deps: edit V8 gitignore to allow trace event copy deps: update V8 trace event to 315bf1e2d45be7d53346c31cfcc37424a32c30c8 deps: edit V8 gitignore to allow gtest_prod.h copy deps: update V8 gtest to 6f8a66431cb592dad629028a50b3dd418a408c87 PR-URL: nodejs#8317 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>
The location of various gypfiles has changed in V8 5.2. PR-URL: nodejs#8317 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>
v8.gyp expects this to be defined by the embedder PR-URL: nodejs#8317 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
V8 now depends on C++11 runtime features. On OSX this requires us to link against the libc++ library rather than the deprecated default that is provided with -mmacosx-version-min=10.7. PR-URL: nodejs#8317 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
V8 needs it for case conversion. Ref: https://codereview.chromium.org/1812673005 PR-URL: nodejs#8317 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Ali Ijaz Sheikh <[email protected]>
mkpeephole is a binary that gets generated and run at V8 build time. On cross-compilation builds separate build toolchains are required. Similar to want_separate_host_toolset we default to disabled. PR-URL: nodejs#8317 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
This replaces TickObject with an object literal. This offers performance improvements of up to ~20%. PR-URL: nodejs#8932 Reviewed-By: Claudio Rodriguez <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
* var to const * add check that expected error is ENOENT * indexOf() to includes() PR-URL: nodejs#8999 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>
PR-URL: nodejs#9089 Reviewed-By: Colin Ihrig <[email protected]>
Wrapped the timer into class to ensure it is cleaned up properly. PR-URL: nodejs#8870 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Adds documentation and explicit reasons on why the GitHub web interface button is not used. This was explained in the referenced issue by @thealphanerd. Fixes: nodejs#8893 PR-URL: nodejs#9044 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Adds verbose reasons to the documentation on why the Reviewed-By metadata on a pull request is important. This was loosely mentioned as an issue in the referenced issue below, and answered by @addaleax. Ref: nodejs#8893 PR-URL: nodejs#9044 Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
tick-processor-base.js is a module used by three other tests. It is not a test fixture so move it out of the fixture directory. (One downside to having it in the fixture directory is that fixture code is not currently linted.) It is possible that the code in tick-processor-base.js should be integrated into common.js. This can potentially happen subsequently (and might make a reasonable good first contribution for a new contributor). PR-URL: nodejs#9022 Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]>
Highlight deprecated API methods/properties in "Table of Contents" for increasing understandability. Adapted code to eslint standards. PR-URL: nodejs#7189 Fixes: nodejs/nodejs.org#772 Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
The config.gypi target has a recipe that uses the control function error to report if the config.gypi file is missing or if it is stale (the configure file was updated which is a prerequisite of this rule). GNU make has two phases, immediate and deferred. During the first phase it will expand any variables or functions as the makefile is parsed. The recipe in this case is a shell if statement, which is a deferred construct. But the control function $(error) is an immediate construct which will cause the makefile processing to stop during the first phase of the Make process. If I understand this correctly the only possible outcome of this rule is the "Stale config.gypi, please re-run ./configure" message which will be done in the first phase and then exit. The shell condition will not be considered. So it will never report that the config.gypi is missing. bnoordhuis suggested that we simply change this into a single error message: "Missing or stale config.gypi, please run configure" PR-URL: nodejs#9053 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
`end` MUST always be emitted **before** `close`. However, if a handle will invoke `uv_close_cb` immediately, or in the same JS tick - `close` may be emitted first. PR-URL: nodejs#9066 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ilkka Myller <[email protected]>
PR-URL: nodejs#8988 Reviewed-By: James M Snell <[email protected]>
This adds a new ESLint tool to check for let declarations within the for, forIn, forOf expressions. Fixes: nodejs#9045 Ref: nodejs#8873 PR-URL: nodejs#9049 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: James M Snell <[email protected]>
This commit fixes a regression introduced in 0ed8839 that caused additional queued immediate callbacks to be ignored if `clearImmediate(immediate)` was called within the callback for `immediate`. PR-URL: nodejs#9086 Fixes: nodejs#9084 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Evan Lucas <[email protected]>
2a4b068 introduced a regression in where checking `instanceof` would fail for `Writable` subclasses inside the subclass constructor, i.e. before `Writable()` was called. Also, calling `null instanceof Writable` or `undefined instanceof Writable` would fail due to accessing the `_writableState` property of the target object. This fixes these problems. PR-URL: nodejs#9088 Ref: nodejs#8834 (comment) Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Evan Lucas <[email protected]>
Commit 782620f added the define only when building with the bundled zlib. Using a shared zlib results in build breakage: ../src/inspector_agent.cc:179:16: error: assigning to 'Bytef *' (aka 'unsigned char *') from incompatible type 'const uint8_t *' (aka 'const unsigned char *') strm.next_in = PROTOCOL_JSON + 3; ^ ~~~~~~~~~~~~~~~~~ 1 error generated. PR-URL: nodejs#9077 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Evan Lucas <[email protected]>
* build: Fix building with shared zlib. (Bradley T. Hughes) [nodejs#9077](nodejs#9077) * stream: fix `Writable` subclass instanceof checks (Anna Henningsen) [nodejs#9088](nodejs#9088) * timers: fix regression with clearImmediate() (Brian White) [nodejs#9086](nodejs#9086) PR-URL: nodejs#9104
Ref: nodejs#8913 PR-URL: nodejs#8993 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Small refactoring to make contextify more readable. Remove auto and inline FromJust(). Simplify if statement. PR-URL: nodejs#8909 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
The documentation erroneously described the errno property as an alias for the code property, but that is not the case in the implementation. errno is the error code of the error as a number, and code is the error code of the error as a string. PR-URL: nodejs#9007 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
Ref: nodejs#8913 PR-URL: nodejs#9047 Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Commit fdca79f ("test: enable addons test to pass with debug build") enabled the addons tests to pass when the build type is of type debug (configure --debug). test/addons/node-module-version/test.js was recently added and expects the the build type to be of type Release (like most of the others until recently). This commit allows this test to pass when the build type if of type debug. PR-URL: nodejs#9093 Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
In branch v4.x-staging, the error message is different:
('spawnSync exit -1 ENOENT' instead of 'spawnSync bad_shell ENOENT').
Since we care that ENOENT should be thrown, the regexp for ENOENT is enough.
Signed-off-by: Robert Chiras <[email protected]>
mscdex
added
the
child_process
Member
|
/cc @Trott |
Contributor
|
Can you please target this at the |
Member
|
Change is fine if targeted at v4.x-staging branch like @cjihrig requests. Let's leave this change off master, though. It's not enough to just check the code because error message changes are semver-major in Node.js (for now at least, and for better or for worse). So we do want tests to blow up when messages change. |
Contributor
Author
|
This patch won't apply on v4.x-staging, so I ammended the initial patch and created a new pull request: #9152 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
18 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
test
Description of change
In branch v4.x-staging, the error message is different:
('spawnSync exit -1 ENOENT' instead of 'spawnSync bad_shell ENOENT').
Since we care that ENOENT should be thrown, the regexp for ENOENT is enough.
Signed-off-by: Robert Chiras [email protected]