test: remove unused config#21985
test: remove unused config #21985
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added benchmark 
apapirovski left a comment
apapirovski left a comment There was a problem hiding this comment.
Thanks for cleaning this up!
refack left a comment
refack left a comment There was a problem hiding this comment.
Please restore test descriptions
There was a problem hiding this comment.
Could we keep the test description (these 3 lines)
There was a problem hiding this comment.
Sure, can revert this - though I didn't find it very informative if you do I'll revert.
There was a problem hiding this comment.
I'll revert except the third line
There was a problem hiding this comment.
Please keep (maybe move to L24 as per https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
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:
benjamingrforce-pushed the remove-outdated-config branch from ea862ff to 6ab5841Compare
refack commented Jul 26, 2018
Thanks for following up. |
maclover7 commented Jul 28, 2018
maclover7 added the author ready 
Trott commented Jul 28, 2018
Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/16050/ |
Resume Build: https://ci.nodejs.org/job/node-test-pull-request/16136/ (edit @maclover7: ✔️) |
maclover7 commented Aug 3, 2018
Landed in d68f946 |
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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jon Moss <[email protected]>
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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jon Moss <[email protected]>
14 participants
process.maxTickDepthwas 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
testorchoreorbenchmark- seemed very insignificant to think a lot about it for this tiny PR.make -j4 test(UNIX), orvcbuild test(Windows) passes