test: use ES6 classes instead of util.inherits by tniessen · Pull Request #16938 · nodejs/node · GitHub

As part of the ongoing refactoring to ES6, I replaced many occurrences of util.inherits within our unit tests with ES6 classes, plus some other minor changes to make the code more readable.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test

AFAICT this can be backported to v6.x

nice!

One small comment, nice cleanup!!

While here, may as well remove this blank line?

I would like to see a backport for 8.x and 6.x opened before this lands

edit: not going to block on it, but would be nice

CI: https://ci.nodejs.org/job/node-test-pull-request/11351/

@MylesBorins I opened a backport PR for 8.x (#16946). It would be nice to land #16947 before backporting this to 6.x.

(while we are here) how about eclosing the class definition inside the common.hasFipsCrypto block as it is used only if the conditional is met?

I mean enclosing

how about eliminating the need for self by converting the only consuming function to an arrow function?

I did that in a couple of other places, seems like I missed it here.

Ahh I remember why I did not do it, because it is simpler to remove the listener this way. Can still refactor it, but it's not pretty.

same comment as above - confining the scope into the block where it is used.

LGTM if CI is green

CI failure is unrelated

Given that this is a test cleanup, it has passed CI, and has plenty of signoff, I'm going to go ahead and land.

PR-URL: #16938 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Landed in 3fe165a

PR-URL: #16938 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

PR-URL: nodejs#16938 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

PR-URL: #16938 Backport-PR-URL: #16946 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

PR-URL: nodejs#16938 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

Backport-PR-URL: #17068 PR-URL: #16938 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

test: use ES6 classes instead of util.inherits

c85977c

tniessen added refactor to ES6+ test Issues and PRs related to the tests. labels Nov 10, 2017

refack approved these changes Nov 10, 2017

View reviewed changes

addaleax approved these changes Nov 10, 2017

View reviewed changes

cjihrig approved these changes Nov 10, 2017

View reviewed changes

maclover7 reviewed Nov 10, 2017

View reviewed changes

[fixup] remove empty line

8c35ace

This was referenced Nov 11, 2017

(v8.x backport) test: use ES6 classes instead of util.inherits #16946

Closed

(v6.x backport) test: refactor test-readline-keys #16947

Closed

gireeshpunathil reviewed Nov 11, 2017

View reviewed changes

[fixup] address comments

0fea678

jasnell approved these changes Nov 12, 2017

View reviewed changes

gireeshpunathil approved these changes Nov 12, 2017

View reviewed changes

jasnell closed this Nov 12, 2017

tniessen mentioned this pull request Nov 12, 2017

test: remove unused parameter #16935

Closed

2 tasks

evanlucas mentioned this pull request Nov 13, 2017

v9.2.0 proposal #16992

Merged

tniessen mentioned this pull request Nov 14, 2017

(v6.x backport) test: remove node-tap lookalike #17024

Closed

gibfahn added the backported-to-v8.x label Nov 14, 2017

tniessen mentioned this pull request Nov 16, 2017

(v6.x backport) test: use ES6 classes instead of util.inherits #17068

Closed

MylesBorins added the backported-to-v6.x label Nov 16, 2017

MylesBorins mentioned this pull request Nov 21, 2017

v6.12.1 proposal #17180

Merged

gibfahn mentioned this pull request Nov 21, 2017

v8.9.2 proposal #17204

Merged

This was referenced Mar 9, 2020

[Snyk] Security upgrade gatsby from 2.13.73 to 2.13.75 kissofire/node#4

Open

[Snyk] Security upgrade gatsby from 2.13.73 to 2.13.75 meghasfdc/node#5

Open

snyk-bot mentioned this pull request Jul 15, 2022

[Snyk] Security upgrade gatsby from 2.13.73 to 2.13.75 kissofire/node#72

Open

+                  return done(null);
+                }
+              }

-              util.inherits(Agent, http.Agent);
-              Agent.prototype.createConnection = function() {
-                const self = this;

-              class TestStream { constructor() { } }
-              util.inherits(TestStream, events.EventEmitter);
+              class TestStream extends events.EventEmitter {}


		_read(n) {
		setTimeout(() => {

test: use ES6 classes instead of util.inherits by tniessen · Pull Request #16938 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: use ES6 classes instead of util.inherits - #16938

test: use ES6 classes instead of util.inherits#16938tniessen wants to merge 3 commits intonodejs:masternodejs/node:masterfrom tniessen:use-more-classestniessen/node:use-more-classesCopy head branch name to clipboard

Conversation

tniessen commented Nov 10, 2017

Checklist

Affected core subsystem(s)

Uh oh!

refack commented Nov 10, 2017

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

maclover7 left a comment

Choose a reason for hiding this comment

Uh oh!

maclover7 Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Nov 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tniessen commented Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gireeshpunathil Nov 11, 2017

Choose a reason for hiding this comment

Uh oh!

gireeshpunathil Nov 11, 2017

Choose a reason for hiding this comment

Uh oh!

gireeshpunathil Nov 11, 2017

Choose a reason for hiding this comment

Uh oh!

tniessen Nov 11, 2017

Choose a reason for hiding this comment

Uh oh!

tniessen Nov 11, 2017

Choose a reason for hiding this comment

Uh oh!

gireeshpunathil Nov 11, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Nov 12, 2017

Uh oh!

jasnell commented Nov 12, 2017

Uh oh!

jasnell commented Nov 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

test: use ES6 classes instead of util.inherits#16938
tniessen wants to merge 3 commits into
nodejs:masterfrom
tniessen:use-more-classes

MylesBorins commented Nov 10, 2017 •
edited
Loading

tniessen commented Nov 11, 2017 •
edited
Loading