lib: string indexOf to use primordials for http2/compat.js by rchougule · Pull Request #36679 · nodejs/node

rchougule · 2020-12-29T18:59:51Z

StringPrototypeIncludes primordial in place of <string>.indexOf for truthy validation in if expression.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

yashLadha · 2020-12-30T00:41:32Z

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

aduh95 · 2020-12-30T09:00:20Z

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.

aduh95 · 2020-12-30T09:21:57Z

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

aduh95 · 2020-12-31T08:53:55Z

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

yashLadha · 2021-01-01T07:50:27Z

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

Trott · 2021-01-02T18:03:24Z

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

rchougule · 2021-01-03T10:01:08Z

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? 😅

aduh95 · 2021-01-03T10:09:34Z

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

See #36746

nodejs-github-bot · 2021-01-05T07:41:09Z

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

nodejs-github-bot · 2021-01-06T10:29:37Z

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

rchougule · 2021-01-06T14:07:26Z

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

aduh95 · 2021-01-06T16:47:43Z

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.

nodejs-github-bot · 2021-01-06T16:48:05Z

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

nodejs-github-bot · 2021-01-06T17:43:28Z

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

rchougule · 2021-01-07T19:07:22Z

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

nodejs-github-bot · 2021-01-08T15:38:17Z

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

PR-URL: #36679 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Pooja D P <Pooja.D.P@ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

yashLadha · 2021-01-09T10:20:08Z

Landed in 3af175f

yashLadha · 2021-01-10T13:44:17Z

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

Trott · 2021-01-10T13:58:48Z

@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).

yashLadha · 2021-01-11T04:10:18Z

LGTM @Trott

Trott · 2021-01-11T10:16:23Z

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

PR-URL: #36679 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Zeyu Yang <himself65@outlook.com> Reviewed-By: Pooja D P <Pooja.D.P@ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

nodejs-github-bot added the http2 Issues or PRs related to the http2 subsystem. label Dec 29, 2020

aduh95 approved these changes Dec 29, 2020
View reviewed changes

Comment threadlib/internal/http2/compat.js Outdated

Lxxyx approved these changes Dec 30, 2020
View reviewed changes

himself65 approved these changes Dec 30, 2020
View reviewed changes

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 30, 2020

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 30, 2020

PoojaDurgad approved these changes Dec 31, 2020
View reviewed changes

aduh95 mentioned this pull request Dec 31, 2020
Cannot start CI jobs nodejs/build#2506
Closed

rchougule force-pushed the primordials_lib_http2 branch from 4bb6fab to 9d927f3Compare December 31, 2020 11:56

yashLadha added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 31, 2020

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 31, 2020

jasnell reviewed Dec 31, 2020
View reviewed changes

Comment threadlib/internal/http2/compat.js Outdated

jasnell approved these changes Dec 31, 2020
View reviewed changes

http2: refactor to use primordials instead of <string>.indexOf
12c78aa

rchougule force-pushed the primordials_lib_http2 branch from fcc389b to 12c78aaCompare December 31, 2020 18:19

yashLadha added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 1, 2021

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 1, 2021

rchougule mentioned this pull request Jan 1, 2021
zlib: refactor to use primordial instead of <string>.startsWith #36718
Closed
2 tasks

Trott approved these changes Jan 2, 2021
View reviewed changes

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 3, 2021

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 3, 2021

This comment has been minimized.
Sign in to view

yashLadha added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 8, 2021

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 8, 2021

yashLadha closed this Jan 9, 2021

This comment has been minimized.
Sign in to view

Trott mentioned this pull request Jan 11, 2021
benchmark: fix http2 benchmarks #36871
Merged

danielleadams mentioned this pull request Jan 12, 2021
v15.6.0 proposal #36889
Merged

targos added the backport-blocked-v14.x label May 25, 2021

Uh oh!

Uh oh!

Conversation

rchougule commented Dec 29, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

yashLadha commented Dec 30, 2020

Uh oh!

aduh95 commented Dec 30, 2020

Uh oh!

aduh95 commented Dec 30, 2020

Uh oh!

aduh95 commented Dec 31, 2020

Uh oh!

Uh oh!

yashLadha commented Jan 1, 2021

Uh oh!

Trott commented Jan 2, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rchougule commented Jan 3, 2021

Uh oh!

aduh95 commented Jan 3, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Jan 5, 2021

Uh oh!

nodejs-github-bot commented Jan 6, 2021

Uh oh!

rchougule commented Jan 6, 2021

Uh oh!

aduh95 commented Jan 6, 2021

Uh oh!

nodejs-github-bot commented Jan 6, 2021

Uh oh!

nodejs-github-bot commented Jan 6, 2021

Uh oh!

rchougule commented Jan 7, 2021

Uh oh!

nodejs-github-bot commented Jan 8, 2021

Uh oh!

yashLadha commented Jan 9, 2021

Uh oh!

yashLadha commented Jan 10, 2021

Uh oh!

Trott commented Jan 10, 2021

Uh oh!

This comment has been minimized.

yashLadha commented Jan 11, 2021

Uh oh!

Trott commented Jan 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

rchougule commented Dec 29, 2020•
edited
Loading

Trott commented Jan 2, 2021•
edited
Loading