★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/21985Nouvelle recherche | Portail wanayoo
Skip to content

/node

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: remove unused config #21985

Closed
wants to merge 1 commit into from

Conversation

Reviewers

@apapirovski apapirovski

@refack refack

@jasnell jasnell

@addaleax addaleax

@lpinca lpinca

@cjihrig cjihrig

@sagirk sagirk

@maclover7 maclover7

@BridgeAR BridgeAR

@trivikr trivikr

@SLYJason SLYJason

Assignees
No one assigned
Labels
author ready benchmark process test
Projects
None yet
Milestone
No milestone
@benjamingr
Copy link
Member

benjamingr commented Jul 26, 2018

process.maxTickDepth was removed in v0.12 a whole while ago and was mostly removed from our code base. There are still some places it was left in old benchmarks and tests.

This PR removes those. Wasn't sure if to label it test or chore or benchmark - seemed very insignificant to think a lot about it for this tiny PR.

  • 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
@nodejs-github-bot

@nodejs-github-bot nodejs-github-bot added benchmark process test labels Jul 26, 2018

@apapirovski

apapirovski approved these changes Jul 26, 2018
View changes

Copy link
Member

apapirovski left a comment

Thanks for cleaning this up!
@refack

refack requested changes Jul 26, 2018
View changes

Copy link
Member

refack left a comment

Please restore test descriptions
test/parallel/test-next-tick-intentional-starvation.js
@@ -23,12 +23,6 @@
require('../common');
const assert = require('assert');

// this is the inverse of test-next-tick-starvation.

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Jul 26, 2018

Member

Could we keep the test description (these 3 lines)

This comment has been minimized.

Sign in to view
Copy link
@benjamingr

benjamingr Jul 26, 2018

Author Member

Sure, can revert this - though I didn't find it very informative if you do I'll revert.

This comment has been minimized.

Sign in to view
Copy link
@benjamingr

benjamingr Jul 26, 2018

Author Member

I'll revert except the third line
test/parallel/test-stream2-read-sync-stack.js
@@ -25,9 +25,6 @@ const Readable = require('stream').Readable;
const r = new Readable();
const N = 256 * 1024;

// Go ahead and allow the pathological case for this test.

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Jul 26, 2018

Member

Please keep (maybe move to L24 as per https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure)

This comment has been minimized.

Sign in to view
Copy link
@benjamingr

benjamingr Jul 26, 2018

Author Member

It no longer allows the pathological use case though since maxTickDepth was removed so the comment is incorrect (and has been since 0.12). Is there alternative phrasing you'd prefer?

This comment has been minimized.

Sign in to view
Copy link
@refack

refack Jul 26, 2018

Member

I did not associate the comment specifically with maxTickDepth. If you are sure it's related, then I would assume the whole test is obsolete, and should be removed.

This comment has been minimized.

Sign in to view
Copy link
@benjamingr

benjamingr Jul 26, 2018

Author Member

@refack the test verifies the behaviour of a synchronous read on the stream. I've added a comment I think makes the most sense. I think benjamingr@782149d explains why it was added
@benjamingr
test: remove unused config 
process.maxTickDepth was removed in v0.12 a whole while ago and was
mostly removed from our code base. There are still some places it was
left in old benchmarks and tests.

This PR removes setting the value in those tests and
benchmarks.

PR-URL:
Reviewed-By:
Loading status checks…
6ab5841

@benjamingr benjamingr force-pushed the benjamingr:remove-outdated-config branch from ea862ff to 6ab5841 Jul 26, 2018

@refack

refack approved these changes Jul 26, 2018
View changes

@refack

This comment has been minimized.

Sign in to view
Copy link
Member

refack commented Jul 26, 2018

refack the test verifies the behaviour of a synchronous read on the stream. I've added a comment I think makes the most sense

Thanks for following up.
@cjihrig

cjihrig approved these changes Jul 26, 2018
View changes

@lpinca

lpinca approved these changes Jul 26, 2018
View changes

@SLYJason

SLYJason approved these changes Jul 26, 2018
View changes

@addaleax

addaleax approved these changes Jul 26, 2018
View changes

@BridgeAR

BridgeAR approved these changes Jul 27, 2018
View changes

@trivikr

trivikr approved these changes Jul 27, 2018
View changes

@jasnell

jasnell approved these changes Jul 27, 2018
View changes

@sagirk

sagirk approved these changes Jul 27, 2018
View changes

@maclover7

maclover7 approved these changes Jul 28, 2018
View changes

@maclover7

This comment has been minimized.

Sign in to view
Copy link
Member

maclover7 commented Jul 28, 2018

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

@maclover7 maclover7 added the author ready label Jul 28, 2018

@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented Jul 28, 2018

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/16050/
@trivikr

trivikr approved these changes Aug 1, 2018
View changes

@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

Trott commented Aug 1, 2018 •
edited by maclover7

Resume Build: https://ci.nodejs.org/job/node-test-pull-request/16136/ (edit @maclover7: ✔️)
@maclover7

This comment has been minimized.

Sign in to view
Copy link
Member

maclover7 commented Aug 3, 2018

Landed in d68f946

@maclover7 maclover7 closed this Aug 3, 2018

maclover7 added a commit that referenced this pull request Aug 3, 2018

@benjamingr @maclover7
test: remove unused config 
process.maxTickDepth was removed in v0.12 a whole while ago and was
mostly removed from our code base. There are still some places it was
left in old benchmarks and tests.

This PR removes setting the value in those tests and
benchmarks.

PR-URL: #21985
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Loading status checks…
d68f946

targos added a commit that referenced this pull request Aug 4, 2018

@benjamingr @targos
test: remove unused config 
process.maxTickDepth was removed in v0.12 a whole while ago and was
mostly removed from our code base. There are still some places it was
left in old benchmarks and tests.

This PR removes setting the value in those tests and
benchmarks.

PR-URL: #21985
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
dafaff3

@rvagg rvagg referenced this pull request Aug 13, 2018

Merged

Release proposal: v10.9.0 #22295

@beevelop beevelop referenced this pull request Aug 16, 2018

Closed

2018-08-15 Version 10.9.0 (Current) @rvagg #230

This was referenced Aug 16, 2018

Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.