Skip to content

test: replace util.inherits with es6 extends #6545

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.

Sign up for GitHub

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

Jump to bottom
Closed
bnoordhuis wants to merge 2 commits into from
Closed

test: replace util.inherits with es6 extends #6545

bnoordhuis wants to merge 2 commits into from

Conversation

@bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented May 3, 2016 •
edited
Loading

I wanted to see how hard it would be to eradicate util.inherits() from an existing code base and I figured, having done the work, I might as well PR it.

I did a quick and partial attempt at updating lib/ as well but it makes for huge diffs, plus it breaks code that calls constructors as functions (e.g. server = http.Server(cb).)

$ git stash show 
 lib/_debug_agent.js                             | 237 ++++++++++++++++++------------------
 lib/_debugger.js                                | 808 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------------------
 lib/_http_agent.js                              | 460 +++++++++++++++++++++++++++++++++++----------------------------------
 lib/_http_client.js                             | 590 ++++++++++++++++++++++++++++++++++++++++++++---------------------------------------------
 lib/_http_incoming.js                           | 275 +++++++++++++++++++++---------------------
 lib/http.js                                     |  85 ++++++-------
 lib/https.js                                    | 253 +++++++++++++++++++-------------------
 test/parallel/test-http-keepalive-client.js     |   4 +-
 test/parallel/test-http-keepalive-maxsockets.js |   4 +-
 test/parallel/test-http-keepalive-request.js    |   4 +-
 test/parallel/test-https-agent-servername.js    |   2 +-
 test/parallel/test-https-agent-sni.js           |   2 +-
 test/parallel/test-https-agent.js               |   2 +-
 13 files changed, 1347 insertions(+), 1379 deletions(-)

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

@bnoordhuis bnoordhuis added the test Issues and PRs related to the tests. label May 3, 2016
@eljefedelrodeodeljefe
Copy link
Contributor

eljefedelrodeodeljefe commented May 3, 2016

There is also one occurrence in test/common.js, which does not break.

@bnoordhuis
Copy link
Member Author

bnoordhuis commented May 3, 2016

I got that one in the first commit, didn't I? Or did I overlook something?

@eljefedelrodeodeljefe
Copy link
Contributor

eljefedelrodeodeljefe commented May 3, 2016

Ah sorry. Was just looking at your stash output. My bad.

@cjihrig
Copy link
Contributor

cjihrig commented May 3, 2016

LGTM. Any plans for a breaking change to lib?

@jasnell
Copy link
Member

jasnell commented May 3, 2016

Not a fan of changing the existing test cases to this for two reasons:

  1. It makes them significantly more difficult to backport to LTS if we need to
  2. It could potentially introduce subtle differences in the test that might impact the results of the test.

I don't mind the change in the benchmark or test/common.js tho

@Fishrock123
Copy link
Contributor

Fishrock123 commented May 3, 2016 •
edited
Loading

It makes them significantly more difficult to backport to LTS if we need to

Agreed, unless there are tangible positives to this I fail to see a good reason for now. (maybe in a year or something?)

@eljefedelrodeodeljefe eljefedelrodeodeljefe mentioned this pull request May 6, 2016
Closed
3 tasks
@bnoordhuis
Copy link
Member Author

bnoordhuis commented May 10, 2016

Guess I'll close this, then.

It makes them significantly more difficult to backport to LTS if we need to

Not if this PR got backported, right? Or am I misunderstanding you?

It could potentially introduce subtle differences in the test that might impact the results of the test.

Shrug. I don't think that's likely. I wouldn't have opened the PR if I did.

@bnoordhuis bnoordhuis closed this May 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@bnoordhuis @eljefedelrodeodeljefe @cjihrig @jasnell @Fishrock123