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: make .out checks embedder-friendly #35040

Merged
merged 1 commit into from
Sep 5, 2020

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Sep 3, 2020

Some of the color support tests in pseudo-tty and some esm tests in message were hardcoded to check the output messages against node - this causes failures in Electron's smoke tests against Node.js, since they'll output e.g:

(Use `electron --trace-warnings ...` to show where the warning was created)

in our case.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. tty Issues and PRs related to the tty subsystem. labels Sep 3, 2020
@codebytere codebytere force-pushed the embedder-agnostic-tests branch 2 times, most recently from 13b4d93 to 5f5d473 Compare September 3, 2020 21:07
@codebytere codebytere added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 4, 2020

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 5, 2020
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

It's too bad there's no way to do something like path.basename(process.execPath) within the out files. (Or is there?) LGTM.

@Trott
Copy link
Member

Trott commented Sep 5, 2020

Landed in 3268a9f

@Trott Trott merged commit 3268a9f into nodejs:master Sep 5, 2020
PR-URL: nodejs#35040
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@addaleax
Copy link
Member

addaleax commented Sep 5, 2020

It's too bad there's no way to do something like path.basename(process.execPath) within the out files. (Or is there?)

We could tell the test runner to replace a specific string, e.g. <execname>, in the expected output file with the value we want to see 🤷‍♀️

richardlau pushed a commit that referenced this pull request Sep 7, 2020
PR-URL: #35040
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@richardlau richardlau mentioned this pull request Sep 7, 2020
4 tasks
richardlau pushed a commit that referenced this pull request Sep 7, 2020
PR-URL: #35040
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere deleted the embedder-agnostic-tests branch September 20, 2020 02:56
codebytere added a commit to electron/electron that referenced this pull request Sep 21, 2020
codebytere added a commit to electron/electron that referenced this pull request Sep 21, 2020
codebytere added a commit to electron/electron that referenced this pull request Sep 21, 2020
codebytere added a commit to electron/electron that referenced this pull request Sep 21, 2020
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
PR-URL: nodejs#35040
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
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. tty Issues and PRs related to the tty subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants