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:
benjamingr
force-pushed
the
remove-outdated-config
branch
from
ea862ff to
6ab5841
Compare
Thanks for following up. |
maclover7
added
the
author ready
|
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: ✔️) |
|
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 <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>
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>
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