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

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

Closed
wants to merge 3 commits into from

Conversation

tniessen
Copy link
Member

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

@tniessen tniessen added refactor to ES6+ test Issues and PRs related to the tests. labels Nov 10, 2017
@refack
Copy link
Contributor

refack commented Nov 10, 2017

AFAICT this can be backported to v6.x

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

nice!

Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

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

One small comment, nice cleanup!!


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

Copy link
Contributor

Choose a reason for hiding this comment

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

While here, may as well remove this blank line?

@MylesBorins
Copy link
Contributor

MylesBorins commented Nov 10, 2017

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

@tniessen
Copy link
Member Author

tniessen commented Nov 11, 2017

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.

this._buffers.push(data);
return done(null);
}
}

Copy link
Member

Choose a reason for hiding this comment

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

(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?

Copy link
Member

Choose a reason for hiding this comment

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

I mean enclosing

util.inherits(Agent, http.Agent);

Agent.prototype.createConnection = function() {
const self = this;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


function listener() {}
function listener2() {}
class TestStream { constructor() { } }
util.inherits(TestStream, events.EventEmitter);
class TestStream extends events.EventEmitter {}

Copy link
Member

Choose a reason for hiding this comment

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

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

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 if CI is green

@jasnell
Copy link
Member

jasnell commented Nov 12, 2017

CI failure is unrelated

@jasnell
Copy link
Member

jasnell commented Nov 12, 2017

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

jasnell pushed a commit that referenced this pull request Nov 12, 2017
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>
@jasnell
Copy link
Member

jasnell commented Nov 12, 2017

Landed in 3fe165a

@jasnell jasnell closed this Nov 12, 2017
@tniessen tniessen mentioned this pull request Nov 12, 2017
2 tasks
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
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>
@evanlucas evanlucas mentioned this pull request Nov 13, 2017
tniessen added a commit to tniessen/node that referenced this pull request Nov 14, 2017
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>
gibfahn pushed a commit that referenced this pull request Nov 14, 2017
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>
tniessen added a commit to tniessen/node that referenced this pull request Nov 16, 2017
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>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
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>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
@gibfahn gibfahn mentioned this pull request Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
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>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
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>
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.

9 participants