Skip to content
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

Closed

Conversation

basti1302
Copy link

@basti1302 basti1302 commented Oct 10, 2018

While backporting #23272 to v10.x and v8.x I noticed that the isReused parameters used in the two reinitialize calls in this test should actually be true instead of false 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

Pass true for isReused in the two reinitialize calls to better reflect
the actual usage here.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 10, 2018
@Trott
Copy link
Member

Trott commented Oct 10, 2018

@Trott
Copy link
Member

Trott commented Nov 14, 2018

@Trott
Copy link
Member

Trott commented Nov 14, 2018

@nodejs/http @nodejs/testing Can someone review this?

@Trott
Copy link
Member

Trott commented Nov 15, 2018

Resume Build CI again: https://ci.nodejs.org/job/node-test-pull-request/18623/

@Trott
Copy link
Member

Trott commented Nov 15, 2018

@BridgeAR
Copy link
Member

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.

@gireeshpunathil
Copy link
Member

ping @basti1302 - can you please rebase this?

@basti1302
Copy link
Author

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.

@gireeshpunathil
Copy link
Member

@basti1302 - thanks no worries. and apologies for the double ping; did not see the previous one!

@basti1302
Copy link
Author

basti1302 commented Mar 7, 2020

Sorry, took me a while to follow up on this. This PR is definitely obsolete now.

First, ece5073 removed the isReused flag from HTTPParser's Reinitialize/Initialize method. Later, the HTTP parser this PR refers to was even replaced by a completely different parser (llhttp) in aa943d0 and ac59dc4.

Closing.

@basti1302 basti1302 closed this Mar 7, 2020
@basti1302 basti1302 deleted the correct-is-reused-flag-in-tests branch March 7, 2020 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants