GeezerVison

Can you please squash the fixup commit. Else LGTM 👍🏽

Note to whoever lands this: the first word of the commit message should be an imperative verb, I suggest something like http2: refactor to use primordials instead of <string>.indexOf.

Can you please squash the fixup commit. Else LGTM 👍🏽

@yashLadha This is usually done by the collaborator who lands the PR (git node land 36679 --fixupAll). They are also in charge of amending the commit message if there are typos or if it doesn't follow the commit guidelines.

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

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

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

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/856/ (queued, will 404 until it starts running)

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/856/ (queued, will 404 until it starts running)

Why does it say NA? or am I looking at the wrong place? 😅

Why does it say NA? or am I looking at the wrong place? 😅

See #36746

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

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

@aduh95 can you help me understand the further steps here? Just trying to understand what's failing here. Thanks!

can you help me understand the further steps here?

We need to have a green CI in order to land this; the failure on the CI are almost certainly unrelated to the changes introduced by this PR, I've just kicked off another one, hopefully it should be good now.

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

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

It seems the same checks failed 😓 ... @aduh95

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

Landed in 3af175f

@Trott It seems that multiple PRs are failing for the 3 jobs probably a flakiness issue.

@Trott It seems that multiple PRs are failing for the 3 jobs probably a flakiness issue.

Yeah, it can be frustrating when CI is broken. If a test is known to be faulty, then a good thing to do is open a fast-track PR to add a line to the .status file telling CI to ignore the test results for that test That's what eventually happened in this case, and that got us back to yellow (and we'll be back to green once nodejs/build#2521 lands).

LGTM @Trott

Subsequent benchmark of a revert on CI didn't show anything of interest, so this is good.