GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: redirect stderr EnvironmentWithNoESMLoader by danbev · Pull Request #36548 · nodejs/node

Conversation

@danbev

@danbev danbev commented Dec 17, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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):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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to Node.js core tests and test infrastructure. label Dec 17, 2020
@nodejs-github-bot

nodejs-github-bot commented Dec 17, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

This comment has been minimized.

Sign in to view
@danbev
danbev force-pushed the test_enviroment_stderr_redirect branch from 6f32a6f to 42fd093 Compare December 17, 2020 14:06
@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? 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Dec 17, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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.)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott
Trott approved these changes Dec 17, 2020
View reviewed changes
@danbev

danbev commented Dec 17, 2020

Copy link
Copy Markdown
Contributor Author

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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Dec 17, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@danbev
danbev force-pushed the test_enviroment_stderr_redirect branch from 42fd093 to 4214584 Compare December 17, 2020 20:17
joyeecheung
joyeecheung approved these changes Dec 18, 2020
View reviewed changes
Comment thread test/cctest/test_environment.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Dec 22, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Dec 22, 2020

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@danbev

danbev commented Dec 23, 2020

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@danbev danbev added the author ready PRs with CI started, the required approvals, and no outstanding review comments. 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]>
@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

Trott commented Dec 24, 2020

Copy link
Copy Markdown
Member

Landed in 3b0ecfc
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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]>
@danielleadams danielleadams mentioned this pull request Jan 12, 2021
v15.6.0 proposal #36889
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]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
v14.17.0 proposal #38507
Merged
@snyk-bot snyk-bot mentioned this pull request Sep 13, 2022
[Snyk] Security upgrade gatsby-plugin-sharp from 2.6.24 to 4.23.0 baby636/node#64
Open
@ron-a20 ron-a20 mentioned this pull request Sep 13, 2022
[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 4.23.0 ron-a20/node#73
Open
This was referenced Sep 13, 2022
[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 4.23.0 Rednickle/node-1#53
Open
[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 4.23.0 aliceUnhinged613/node#66
Open
@wedataintelligence wedataintelligence mentioned this pull request Sep 13, 2022
[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 4.23.0 wedataintelligence/node#40
Open
@snyk-bot snyk-bot mentioned this pull request Sep 13, 2022
[Snyk] Security upgrade gatsby-plugin-sharp from 2.2.13 to 4.23.0 kissofire/node#75
Open
@molllyn1 molllyn1 mentioned this pull request Sep 14, 2022
[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 4.23.0 molllyn1/node#55
Open
@137717unity 137717unity mentioned this pull request Sep 14, 2022
[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 4.23.0 137717unity/node#64
Open
@snyk-bot snyk-bot mentioned this pull request Sep 25, 2022
[Snyk] Fix for 22 vulnerabilities 137717unity/node#70
Open
@wedataintelligence wedataintelligence mentioned this pull request Sep 26, 2022
[Snyk] Fix for 22 vulnerabilities wedataintelligence/node#44
Open
@137717unity 137717unity mentioned this pull request Sep 26, 2022
[Snyk] Fix for 22 vulnerabilities 137717unity/node#73
Open
@snyk-bot snyk-bot mentioned this pull request Sep 27, 2022
[Snyk] Fix for 21 vulnerabilities Rednickle/node-1#56
Open
@wedataintelligence wedataintelligence mentioned this pull request Sep 29, 2022
[Snyk] Fix for 24 vulnerabilities wedataintelligence/node#50
Open
This was referenced Sep 29, 2022
[Snyk] Fix for 24 vulnerabilities 137717unity/node#75
Open
[Snyk] Fix for 23 vulnerabilities Rednickle/node-1#60
Open
This was referenced Oct 5, 2022
[Snyk] Fix for 24 vulnerabilities wedataintelligence/node#51
Open
[Snyk] Fix for 24 vulnerabilities wedataintelligence/node#52
Open
@snyk-bot snyk-bot mentioned this pull request Oct 6, 2022
[Snyk] Fix for 24 vulnerabilities wedataintelligence/node#53
Open
@137717unity 137717unity mentioned this pull request Oct 7, 2022
[Snyk] Fix for 24 vulnerabilities 137717unity/node#76
Open
@wedataintelligence wedataintelligence mentioned this pull request Oct 7, 2022
[Snyk] Fix for 24 vulnerabilities wedataintelligence/node#54
Open
This was referenced Oct 8, 2022
[Snyk] Fix for 24 vulnerabilities wedataintelligence/node#55
Open
[Snyk] Fix for 24 vulnerabilities 137717unity/node#77
Open
@137717unity 137717unity mentioned this pull request Oct 9, 2022
[Snyk] Fix for 24 vulnerabilities 137717unity/node#78
Open
@snyk-bot snyk-bot mentioned this pull request Oct 9, 2022
[Snyk] Fix for 23 vulnerabilities Rednickle/node-1#63
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott approved these changes
@joyeecheung joyeecheung joyeecheung approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@danbev @nodejs-github-bot @Trott @joyeecheung
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.