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

[v6.x backport] Eslint 4.1.0 #14830

Closed
wants to merge 59 commits into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Aug 14, 2017

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
Affected core subsystem(s)

Backport of the first two of the four commits in #13895

jasnell and others added 30 commits August 14, 2017 11:05
Export a new common.noop no-operation function for general use.
Allow using common.mustCall() without a fn argument to simplify
test cases.

Replace various non-op functions throughout tests with common.noop

PR-URL: nodejs#12027
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Teddy Katz <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Prefer using `common.mustNotCall()` over `common.mustCall(fn, 0)`

PR-URL: nodejs#12027
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Teddy Katz <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
PR-URL: nodejs#12935
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
* add `mustCall` and `mustNotCall` to all callbacks
* added `known_issue` for port binding

Backport-PR-URL: nodejs#14327
PR-URL: nodejs#13100
Refs: nodejs#13055
Refs: nodejs#12999
Refs: nodejs#13526
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Fix the bug that you start process without network at first, but it
connected lately, `dns.resolve` will stay failed with ECONNREFUSED
because c-ares servers fallback to 127.0.0.1 at the very beginning.

If c-ares servers "127.0.0.1" is detected and its not set by user self,
and last query is not OK, recreating `ares_channel` operation will be
triggered to reload servers.

Fixes: nodejs#1644
Backport-PR-URL: nodejs#14434
PR-URL: nodejs#13076
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
A few of the CLI option values exposed as properties on the process
object were missing a comment, fix this.

Backport-PR-URL: nodejs#14483
PR-URL: nodejs#12245
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
bootstrap_node.js was directly parsing process.execArgv to see if
internals should be exposed, even though the argv was already parsed by
node. This is unusual and unnecessary, change it to set the option value
from the parser onto the config binding.

Backport-PR-URL: nodejs#14483
PR-URL: nodejs#12245
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: nodejs#14495
Fixes: nodejs#14430
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Backport-PR-URL: nodejs#14787
PR-URL: nodejs#14438
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
`path.resolve()` and `path.join()` are left alone in this commit due to
the lack of clear semantics.

Backport-PR-URL: nodejs#14787
PR-URL: nodejs#14440
Fixes: nodejs#14405
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
As the length of `path` is known at this point, there is no point in
making an exact copy using `slice`.

Backport-PR-URL: nodejs#14787
PR-URL: nodejs#14438
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
* we use u_setDataDirectory() in "unicode/putil.h"
* at present, this header is indirectly included,
  but this will change in ICU 59
* no impact on past ICUs.
* this is an exact analog to nodejs#11753

PR-URL: nodejs#12078
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Timers should work even if the user has monkey-patched `.call()` and
`.apply()` to undesirable values.

PR-URL: nodejs#12960
Ref: nodejs#12956
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Original commit message:

    This commit adds a getter for the private is_verbose_ member.
    The use case for this comes from Node.js where the ability to avoid
    calling FatalException if the TryCatch is verbose would be nice
    to have.

    BUG=

    Review-Url: https://codereview.chromium.org/2840803002
    Cr-Commit-Position: refs/heads/master@{nodejs#45018}

PR-URL: nodejs#12826
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
This commit attempts to address the TODO regarding not calling
FatalException if the try_catch is verbose.

PR-URL: nodejs#12826
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
`configure` will now call `node_gyp` as a module instead of forking
makes it easier to debug

PR-URL: nodejs#12653
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
When a no-op message event handler is used in a test, make it clear what
is expected by using `common.mustCall()` and `common.mustNotCall()`.

PR-URL: nodejs#13125
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Use common.mustCall() in test-fs-makeStatsCallback to confirm that the
callback is invoked.

PR-URL: nodejs#13132
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
In test/pummel/test-stream2-basic.js, check that noop functions are
called the expected number of times.

PR-URL: nodejs#13146
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
PR-URL: nodejs#13146
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
test-stream2-basic runs in a few seconds. It can be moved to parallel.

PR-URL: nodejs#13146
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Use the same dtrace command on ARM as on i386. Patch obtained from
FreeBSD PR 218081 [1].

1. https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=218081

PR-URL: nodejs#12193
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#13215
Refs: nodejs#12586
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Use `common.mustNotCall()` in test-stream2-objects.js to confirm that
noop function is never invoked.

PR-URL: nodejs#13249
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Use `common.mustCall()` to make sure noop function is called as
expected.

PR-URL: nodejs#13259
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
* Use common.mustNotCall() and common.mustCall() as appropriate
* Use block scoping
* Move assertions out of `exit` handler and into callbacks
* Order assert.strictEqual() args per docs
* Remove console.log() calls
* Move test from `parallel` to `sequential` so `common.PORT` can be used
  without conflicting with OS-provided ports in other `parallel` tests

PR-URL: nodejs#13273
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#13359
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Cai <[email protected]>
* Change common.noop to common.mustNotCall() to verify callback is not
  invoked.
* Add destructuring assignment for clarity. Yeah, clarity. That's why.

PR-URL: nodejs#13443
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Use `common.mustNotCall()` to confirm that callback is not invoked when
`dns.lookup()` throws.

PR-URL: nodejs#13456
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
* Check that noop callback is or isn't invoked as appropriate using
  common.mustCall() and common.mustNotCall()
* Fix typo in array literal

PR-URL: nodejs#13480
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
lenakaplan and others added 14 commits August 14, 2017 16:16
Remove unnecessary imports from:

* test/parallel/test-buffer-nopendingdep-map.js
* test/parallel/test-buffer-pending-deprecation.js
* test/parallel/test-buffer-sharedarraybuffer.js
* test/parallel/test-buffer-slow.js
* test/parallel/test-buffer-tojson.js
* test/parallel/test-buffer-zero-fill.js

Refs: nodejs#13836
PR-URL: nodejs#13851
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
We don't use the global Buffer throughout the lib/ to avoid circular
dependency issues in core, but we actually don't need to require it on
test files. So remove them on:

+ test/parallel/test-stream-uint8array.js
+ test/parallel/test-stream2-finish-pipe.js
+ test/parallel/test-tls-session-cache.js
+ test/parallel/test-vm-cached-data.js

Refs: nodejs#13836
PR-URL: nodejs#13844
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Currently when configuring the project using --openssl-fips a gyp
include file name config_fips.gypi will be created. If the project is
later configured but without the --openssl-fips flag an error will
occur. For example:

  $ ./configure --openssl-fips=bogus
  $ ./configure && make -j8
  ...
  /node/deps/openssl/fips/fipsld:
  line 8: /bin/fipsld: No such file or directory
  Error 127

This commit suggests removing the generate config_fips.gypi when the
--openssl-fips flag is not give on the command line.

PR-URL: nodejs#13837
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Make test-http(s)-set-timeout-server tests more similar and resolve the
following issues:

* `test-https-set-timeout-server.js` was missing some `assert`
   statements, including with `http` module

* Both files were missing some calls to `common.mustCall()`

* Both files were calling `createServer()` in different ways

PR-URL: nodejs#13822
Refs: nodejs#13588
Refs: nodejs#13625
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Reviewed-By: James M Snell <[email protected]>
* Use `common.mustCall()` to guarantee callback invocations
* Order modules according to test writing guide

PR-URL: nodejs#13886
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
* use common.mustCall()/common.mustNotCall() as appropriate
* reorder require() statements per test writing guide
* add comment

PR-URL: nodejs#13893
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
The motivation for this commit is that we are building Node with
--shared-openssl and in our case the system OpenSSL version
supports FIPS.

The tests in test-crypto-fips that toggle fips mode on/off using the
config file option might succeed and return 1 instead of an error
being thrown from OpenSSL (which is what happens for a default build
but the error is not processed/displayed in any way at the moment):
openssl config failed: error:060B10A7:digital envelope
routines:ALG_MODULE_INIT:fips mode not supported

Note that this only concerns the test that use the configuration file
option which is different from when calling the fips setter as
the handling of the configuration file is handled by OpenSSL, so it
is not possible for us to try to call the fips setter as that would
throw an error ("Error: Cannot set FIPS mode in a non-FIPS build.").

The suggestion is to skips these tests when --shared-openssl is used.

PR-URL: nodejs#13786
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Removed some bad escape characters

PR-URL: nodejs#13808
Fixes: nodejs#13805
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
These methods are Javascript-accessible so they get an implicit
HandleScope. The extra scope is unneeded and can be dropped.

PR-URL: nodejs#13859
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Signed-off-by: Ezequiel Garcia <[email protected]>
The docs implied that the parameters `from` and `to` are invalid only
if neither of them is a string; in fact, they are invalid as soon as one
of them is not a string.

PR-URL: nodejs#13912
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: David Cai <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
PR-URL: nodejs#13967
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
PR-URL: nodejs#13952
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Update ESLint to 4.1.0. This fixes a bug that previously prevented us
from using the new and stricter indentation checking.

Refs: eslint/eslint#8721
PR-URL: nodejs#13895
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Provide a bash script for updating ESLint in the project.

PR-URL: nodejs#13895
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@nodejs-github-bot nodejs-github-bot added tools Issues and PRs related to the tools directory. v6.x labels Aug 14, 2017
@Trott Trott mentioned this pull request Aug 14, 2017
2 tasks
@Trott
Copy link
Member Author

Trott commented Aug 14, 2017

@Trott Trott mentioned this pull request Aug 14, 2017
2 tasks
@MylesBorins MylesBorins force-pushed the v6.x-staging branch 3 times, most recently from cebce8b to 5a93c16 Compare August 16, 2017 03:58
@MylesBorins
Copy link
Contributor

landed in ae3e7be...35131c3

MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
Update ESLint to 4.1.0. This fixes a bug that previously prevented us
from using the new and stricter indentation checking.

Refs: eslint/eslint#8721
Backport-PR-URL: #14830
PR-URL: #13895
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
Provide a bash script for updating ESLint in the project.

Backport-PR-URL: #14830
PR-URL: #13895
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 5, 2017
Update ESLint to 4.1.0. This fixes a bug that previously prevented us
from using the new and stricter indentation checking.

Refs: eslint/eslint#8721
Backport-PR-URL: #14830
PR-URL: #13895
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 5, 2017
Provide a bash script for updating ESLint in the project.

Backport-PR-URL: #14830
PR-URL: #13895
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@Trott Trott deleted the backport-13895 branch January 13, 2022 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.