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

benchmark: fix misc/startup failure #42442

Closed

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Mar 23, 2022

Fixes: #42437

It's not the ideal fix as it's duplicating a list/Set that already exists in the internals but 🤷‍♂️

@nodejs-github-bot nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Mar 23, 2022
@mscdex
Copy link
Contributor Author

mscdex commented Mar 23, 2022

@nodejs-github-bot
Copy link
Collaborator

@Trott Trott requested a review from cjihrig March 23, 2022 04:22
@cjihrig
Copy link
Contributor

cjihrig commented Mar 23, 2022

I would hold off on landing this because whether or not we are keeping node:test is under discussion in #42430.

@mscdex
Copy link
Contributor Author

mscdex commented Apr 12, 2022

@cjihrig Since the discussion in #42430 seems to be (presumably) taking longer than expected, can we merge this in the meantime? It's no longer just affecting my daily benchmark runs, but now also PRs that need to run the benchmark.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 12, 2022

Yea, I think it's probably fine to merge. Sorry for the hold up.

for (const key of canBeRequired) {
for (let key of canBeRequired) {
if (modulesNeedingPrefix.includes(key))
key = `node:${key}`;
require(key);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to use the prefix for all modules?

Suggested change
require(key);
require(`node:${key}`);

@aduh95
Copy link
Contributor

aduh95 commented Apr 17, 2022

Closing because #42746 has landed, and fixes the issue. Please re-open if I'm mistaken.

@aduh95 aduh95 closed this Apr 17, 2022
@mscdex mscdex deleted the benchmark-fix-misc-startup-failure branch April 17, 2022 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

misc/startup benchmark broken as of 3c4ee52
6 participants