-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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: correct isReused parameter in test-http-parser #23412
Conversation
Pass true for isReused in the two reinitialize calls to better reflect the actual usage here.
@nodejs/http @nodejs/testing Can someone review this? |
Resume Build CI again: https://ci.nodejs.org/job/node-test-pull-request/18623/ |
Resume Build: https://ci.nodejs.org/job/node-test-pull-request/18633/ |
This seems pretty much good to go besides a rebase. @basti1302 I am very sorry that it took so long to look at this again. With all the PRs incoming it is some times difficult to keep track. |
ping @basti1302 - can you please rebase this? |
I actually think this has become obsolete with 8876ac5#diff-d4784c96a833a5b56576449c93b866fc / #25094 But I did not have time yet to dig into this again to check if it actually has become obsolete. I don't even fully remember in detail what this was about :) Honestly though, folks: You have let this sit well over a year but ping me after just a few days of me not responding? I'll take care of it in the next few days or weeks, when I find some time. |
@basti1302 - thanks no worries. and apologies for the double ping; did not see the previous one! |
Sorry, took me a while to follow up on this. This PR is definitely obsolete now. First, ece5073 removed the Closing. |
While backporting #23272 to v10.x and v8.x I noticed that the
isReused
parameters used in the tworeinitialize
calls in this test should actually betrue
instead offalse
to better reflect the actual usage here (the parser is being reused).This does not have any effect on the test result here (since we do not test async_hooks calls here) but for the sake of consistency the test should use the API correctly.
Refs: #23272
Checklist
make -j4 test
passes