Skip to content

test: redirect stderr EnvironmentWithNoESMLoader#36548

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.

Sign up for GitHub

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

Jump to bottom
Merged
Trott merged 1 commit into from
Dec 24, 2020
Merged

test: redirect stderr EnvironmentWithNoESMLoader #36548

Trott merged 1 commit into from
Dec 24, 2020

Conversation

@danbev
Copy link
Contributor

@danbevdanbev commented Dec 17, 2020•
edited
Loading

This commit adds a suggestion to redirect stderr for
EnvironmentTest.EnvironmentWithNoESMLoader.

The motivation for this is that currently this tests prints the
following error (which is expected):

vm:module(0):1globalThis.importResult = import("")^Error: Not supported at vm:module(0):1:1 at SourceTextModule.evaluate (node:internal/vm/module:229:23) at node:embedder_main_12:1:328 at processTicksAndRejections (node:internal/process/task_queues:93:5)

It might not be obvious which test caused this error just by looking at
the output above and it would be nice if it was not displayed.

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

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Dec 17, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 17, 2020

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

@Trott

This comment has been minimized.

Sign in to view
@danbevdanbevforce-pushed the test_enviroment_stderr_redirect branch from 6f32a6f to 42fd093CompareDecember 17, 2020 14:06
@Trott
Copy link
Member

Trott commented Dec 17, 2020

This writes the output file to the current directory, right? I guess the cctest has no notion of the temp directory like the test.py does? IIRC, trying to use system tmp directory has been problematic in a few scenarios on CI, so I guess this is OK. But ideally, tests wouldn't write files where code lives.

@Trott
Copy link
Member

Trott commented Dec 17, 2020•
edited
Loading

This writes the output file to the current directory, right? I guess the cctest has no notion of the temp directory like the test.py does?

gtest-all.cc has a TempDir() function. Can that be used or re-used somehow? (EDIT: Although I guess we'll have to see if it works for everything in CI? These are comments/questions, but not blockers. I'm OK with this landing as it is now.)

@danbev
Copy link
ContributorAuthor

danbev commented Dec 17, 2020

This writes the output file to the current directory, right?

It does, but it also removes the file it writes in the destructor. The assumption here is that one would not be interested in the stderr output, but could just comment out the line if wanting to see the output while developing (or tail the file redirected to while debugging would also work).

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 17, 2020

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

@danbevdanbevforce-pushed the test_enviroment_stderr_redirect branch from 42fd093 to 4214584CompareDecember 17, 2020 20:17
joyeecheung
joyeecheung approved these changes Dec 18, 2020
View reviewed changes
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 22, 2020

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 22, 2020

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

@danbev
Copy link
ContributorAuthor

danbev commented Dec 23, 2020

Re-run of failing node-test-commit-aix ✔️

@danbevdanbev added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 23, 2020
@danbev@Trott
test: redirect stderr EnvironmentWithNoESMLoader
3b0ecfc
This commit adds a suggestion to redirect stderr for EnvironmentTest.EnvironmentWithNoESMLoader. The motivation for this is that currently this tests prints the following error (which is expected): vm:module(0):1 globalThis.importResult = import("") ^ Error: Not supported at vm:module(0):1:1 at SourceTextModule.evaluate (node:internal/vm/module:229:23) at node:embedder_main_12:1:328 at processTicksAndRejections (node:internal/process/task_queues:93:5) It might not be obvious which test caused this error just by looking at the output above and it would be nice if it was not displayed. PR-URL: nodejs#36548 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
@TrottTrottforce-pushed the test_enviroment_stderr_redirect branch from cc74364 to 3b0ecfcCompareDecember 24, 2020 15:18
@TrottTrott merged commit 3b0ecfc into nodejs:masterDec 24, 2020
@Trott
Copy link
Member

Trott commented Dec 24, 2020

Landed in 3b0ecfc

danielleadams pushed a commit that referenced this pull request Jan 12, 2021
@danbev@danielleadams
test: redirect stderr EnvironmentWithNoESMLoader
863bfc4
This commit adds a suggestion to redirect stderr for EnvironmentTest.EnvironmentWithNoESMLoader. The motivation for this is that currently this tests prints the following error (which is expected): vm:module(0):1 globalThis.importResult = import("") ^ Error: Not supported at vm:module(0):1:1 at SourceTextModule.evaluate (node:internal/vm/module:229:23) at node:embedder_main_12:1:328 at processTicksAndRejections (node:internal/process/task_queues:93:5) It might not be obvious which test caused this error just by looking at the output above and it would be nice if it was not displayed. PR-URL: #36548 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
@danielleadamsdanielleadams mentioned this pull request Jan 12, 2021
Merged
targos pushed a commit that referenced this pull request May 1, 2021
@danbev@targos
test: redirect stderr EnvironmentWithNoESMLoader
064144d
This commit adds a suggestion to redirect stderr for EnvironmentTest.EnvironmentWithNoESMLoader. The motivation for this is that currently this tests prints the following error (which is expected): vm:module(0):1 globalThis.importResult = import("") ^ Error: Not supported at vm:module(0):1:1 at SourceTextModule.evaluate (node:internal/vm/module:229:23) at node:embedder_main_12:1:328 at processTicksAndRejections (node:internal/process/task_queues:93:5) It might not be obvious which test caused this error just by looking at the output above and it would be nice if it was not displayed. PR-URL: #36548 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
@danielleadamsdanielleadams mentioned this pull request May 3, 2021
Merged
@snyk-botsnyk-bot mentioned this pull request Sep 13, 2022
Open
@ron-a20ron-a20 mentioned this pull request Sep 13, 2022
Open
This was referenced Sep 13, 2022
Open
Open
@wedataintelligencewedataintelligence mentioned this pull request Sep 13, 2022
Open
@snyk-botsnyk-bot mentioned this pull request Sep 13, 2022
Open
@molllyn1molllyn1 mentioned this pull request Sep 14, 2022
Open
@137717unity137717unity mentioned this pull request Sep 14, 2022
Open
@snyk-botsnyk-bot mentioned this pull request Sep 25, 2022
Open
@wedataintelligencewedataintelligence mentioned this pull request Sep 26, 2022
Open
@137717unity137717unity mentioned this pull request Sep 26, 2022
Open
@snyk-botsnyk-bot mentioned this pull request Sep 27, 2022
Open
@wedataintelligencewedataintelligence mentioned this pull request Sep 29, 2022
Open
This was referenced Sep 29, 2022
Open
Open
This was referenced Oct 5, 2022
Open
Open
@snyk-botsnyk-bot mentioned this pull request Oct 6, 2022
Open
@137717unity137717unity mentioned this pull request Oct 7, 2022
Open
@wedataintelligencewedataintelligence mentioned this pull request Oct 7, 2022
Open
This was referenced Oct 8, 2022
Open
Open
@137717unity137717unity mentioned this pull request Oct 9, 2022
Open
@snyk-botsnyk-bot mentioned this pull request Oct 9, 2022
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott approved these changes

@joyeecheungjoyeecheungjoyeecheung approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@danbev@nodejs-github-bot@Trott@joyeecheung