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

test: redirect stderr EnvironmentWithNoESMLoader #36548

Merged
merged 1 commit into from
Dec 24, 2020

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Dec 17, 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.

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

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

@Trott

This comment has been minimized.

@danbev danbev force-pushed the test_enviroment_stderr_redirect branch from 6f32a6f to 42fd093 Compare December 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

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
Contributor Author

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

@danbev danbev force-pushed the test_enviroment_stderr_redirect branch from 42fd093 to 4214584 Compare December 17, 2020 20:17
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@danbev
Copy link
Contributor Author

danbev commented Dec 23, 2020

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

@danbev danbev 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 Trott force-pushed the test_enviroment_stderr_redirect branch from cc74364 to 3b0ecfc Compare December 24, 2020 15:18
@Trott Trott merged commit 3b0ecfc into nodejs:master Dec 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
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>
@danielleadams danielleadams 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>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants