Skip to content

test: redirect stderr EnvironmentWithNoESMLoader - #36548

Merged
Trott merged 1 commit into
nodejs:masterfrom
danbev:test_enviroment_stderr_redirect
Dec 24, 2020
Merged

test: redirect stderr EnvironmentWithNoESMLoader#36548
Trott merged 1 commit into
nodejs:masterfrom
danbev:test_enviroment_stderr_redirect

Conversation

@danbev

@danbevdanbev commented Dec 17, 2020

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Collaborator

@Trott

This comment has been minimized.

@danbev
danbevforce-pushed the test_enviroment_stderr_redirect branch from 6f32a6f to 42fd093CompareDecember 17, 2020 14:06
@Trott

Copy link
Copy Markdown
Member

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

Trott commented Dec 17, 2020

Copy link
Copy Markdown
Member

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
Copy Markdown
ContributorAuthor

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
Copy Markdown
Collaborator

@danbev
danbevforce-pushed the test_enviroment_stderr_redirect branch from 42fd093 to 4214584CompareDecember 17, 2020 20:17
Comment threadtest/cctest/test_environment.cc Outdated
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@danbev

Copy link
Copy Markdown
ContributorAuthor

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
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 <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@Trott
Trottforce-pushed the test_enviroment_stderr_redirect branch from cc74364 to 3b0ecfcCompareDecember 24, 2020 15:18
@Trott
Trott merged commit 3b0ecfc into nodejs:masterDec 24, 2020
@Trott

Copy link
Copy Markdown
Member

Landed in 3b0ecfc

danielleadams pushed a commit that referenced this pull request Jan 12, 2021
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 <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@danielleadamsdanielleadams mentioned this pull request Jan 12, 2021
targos pushed a commit that referenced this pull request May 1, 2021
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 <rtrott@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@danielleadamsdanielleadams mentioned this pull request May 3, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

4 participants

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