src: merge `RunInThisContext()` with `RunInContext()` #43225

daeyeon · 2022-05-28T03:54:51Z

This commit resolves TODO in RunInThisContext() by merging
RunInThisContext() with RunInContext().

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

daeyeon · 2022-05-31T00:15:14Z

@addaleax Could you take a look when you have time? I tried to resolve the todo you wrote.

src/node_contextify.cc

RaisinTen · 2022-06-03T04:31:15Z

cc @nodejs/vm

test/parallel/test-trace-events-vm.js

src/node_contextify.cc

daeyeon · 2022-06-10T00:46:33Z

The test failure looks irrelevant to this change. Can anyone pls help me re-run the CI?

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

daeyeon · 2022-07-08T18:08:10Z

Thanks for keeping retry the CI test. I've rebased this PR again because of the same reason above.

aduh95 · 2022-07-09T11:55:34Z

Thanks for keeping retry the CI test. I've rebased this PR again because of the same reason above.

IMO you'll get better result by not rebasing the CI, every time we push new commits we have to re-run all the jobs including the one that were already green. FYI no need to rebase it yourself, by re-adding the request-ci label, it instructs the CI to rebase the PR on top of main and run the tests on all platforms. In fact it's better if you don't rebase but simply add a comment requesting a clean CI as we don't need to re-review the changes you just pushed.

nodejs-github-bot · 2022-07-09T11:57:23Z

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

daeyeon · 2022-07-09T12:26:09Z

@aduh95 A long-living curiosity I've had is cleared through your suggestion. Thanks!

nodejs-github-bot · 2022-07-09T20:07:51Z

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

nodejs-github-bot · 2022-07-10T07:52:02Z

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

aduh95 · 2022-07-10T08:56:02Z

@daeyeon I'd suggest to not add the label, resuming CI is much more efficient than re-requesting CI as it can skip already passing jobs.

daeyeon · 2022-07-11T02:17:41Z

@aduh95 Regarding "resuming CI", after reading the collaborator guide, I guessed it maybe mean "Resume Build" in CI.

If there are Jenkins CI failures unrelated to the change in the pull request, try "Resume Build". It is in the left navigation of the relevant node-test-pull-request job.
https://github.com/nodejs/node/blob/main/doc/contributing/collaborator-guide.md#testing-and-ci

It says to try "Resume Build" in the left navigation, but I didn't find it there. Instead, I found a "Resume Build" string below "Test Result" in this page that looks disabled. Could you please check that's what you recommended? (I've recently joined the triagers team. So maybe I have no permission yet.)

aduh95 · 2022-07-11T07:59:22Z

Yeah, triaggers don't have permission for that, adding the possibility for them to do it has been discussed in #40817 but it's not as trivial you'd think to implement.

aduh95

RSLGTM

nodejs-github-bot · 2022-07-11T08:08:43Z

Landed in 7f29993

daeyeon · 2022-07-11T09:12:02Z

Thank you all!

This commit resolves a TODO in `RunInThisContext()` by merging `RunInThisContext()` with `RunInContext()`. Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com PR-URL: #43225 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

This commit resolves a TODO in `RunInThisContext()` by merging `RunInThisContext()` with `RunInContext()`. Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com PR-URL: nodejs/node#43225 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem. labels May 28, 2022

addaleax approved these changes May 31, 2022

View reviewed changes

RaisinTen reviewed May 31, 2022

View reviewed changes

src/node_contextify.cc Show resolved Hide resolved

daeyeon force-pushed the master.todo.contextify-220528.Sat.e2de branch 4 times, most recently from ca2b036 to fe9eab5 Compare June 1, 2022 02:09

RaisinTen approved these changes Jun 3, 2022

View reviewed changes

RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 3, 2022

This comment was marked as outdated.

Sign in to view

github-actions bot mentioned this pull request Jun 4, 2022

CI Reliability 2022-06-04 nodejs/reliability#298

Open

21 tasks

This comment was marked as outdated.

Sign in to view

tniessen reviewed Jun 6, 2022

View reviewed changes

test/parallel/test-trace-events-vm.js Show resolved Hide resolved

legendecas reviewed Jun 6, 2022

View reviewed changes

src/node_contextify.cc Outdated Show resolved Hide resolved

RaisinTen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 6, 2022

JungMinu approved these changes Jun 6, 2022

View reviewed changes

legendecas approved these changes Jun 6, 2022

View reviewed changes

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 6, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 6, 2022

This comment was marked as outdated.

Sign in to view

RaisinTen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jun 6, 2022

tniessen approved these changes Jun 6, 2022

View reviewed changes

This comment was marked as outdated.

Sign in to view

fixup: merge eval_env with env

06a97fd

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

daeyeon force-pushed the master.todo.contextify-220528.Sat.e2de branch from 6d86ee6 to 06a97fd Compare July 8, 2022 18:05

github-actions bot mentioned this pull request Jul 9, 2022

CI Reliability 2022-07-09 nodejs/reliability#319

Open

21 tasks

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 9, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 9, 2022

github-actions bot mentioned this pull request Jul 10, 2022

CI Reliability 2022-07-10 nodejs/reliability#320

Open

20 tasks

daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 10, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 10, 2022

aduh95 approved these changes Jul 11, 2022

View reviewed changes

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 11, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 11, 2022

nodejs-github-bot merged commit 7f29993 into nodejs:main Jul 11, 2022

daeyeon deleted the master.todo.contextify-220528.Sat.e2de branch July 11, 2022 09:12

targos mentioned this pull request Jul 12, 2022

v18.6.0 release proposal #43789

Merged

targos mentioned this pull request Aug 3, 2022

v16.17.0 release proposal #44098

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: merge `RunInThisContext()` with `RunInContext()` #43225

src: merge `RunInThisContext()` with `RunInContext()` #43225

daeyeon commented May 28, 2022

daeyeon commented May 31, 2022

RaisinTen commented Jun 3, 2022

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

daeyeon commented Jun 10, 2022

This comment was marked as outdated.

daeyeon commented Jul 8, 2022

aduh95 commented Jul 9, 2022 •

edited

Loading

nodejs-github-bot commented Jul 9, 2022

daeyeon commented Jul 9, 2022

nodejs-github-bot commented Jul 9, 2022

nodejs-github-bot commented Jul 10, 2022

aduh95 commented Jul 10, 2022

daeyeon commented Jul 11, 2022

aduh95 commented Jul 11, 2022

aduh95 left a comment

nodejs-github-bot commented Jul 11, 2022

daeyeon commented Jul 11, 2022

src: merge RunInThisContext() with RunInContext() #43225

src: merge RunInThisContext() with RunInContext() #43225

Conversation

daeyeon commented May 28, 2022

daeyeon commented May 31, 2022

RaisinTen commented Jun 3, 2022

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

daeyeon commented Jun 10, 2022

This comment was marked as outdated.

daeyeon commented Jul 8, 2022

aduh95 commented Jul 9, 2022 • edited Loading

nodejs-github-bot commented Jul 9, 2022

daeyeon commented Jul 9, 2022

nodejs-github-bot commented Jul 9, 2022

nodejs-github-bot commented Jul 10, 2022

aduh95 commented Jul 10, 2022

daeyeon commented Jul 11, 2022

aduh95 commented Jul 11, 2022

aduh95 left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jul 11, 2022

daeyeon commented Jul 11, 2022

src: merge `RunInThisContext()` with `RunInContext()` #43225

src: merge `RunInThisContext()` with `RunInContext()` #43225

aduh95 commented Jul 9, 2022 •

edited

Loading