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.

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

test: permit test-signalwrap to work without test runner #28306

Merged
merged 0 commits into from
Sep 8, 2019
Merged

test: permit test-signalwrap to work without test runner #28306

merged 0 commits into from
Sep 8, 2019

Conversation

Trott
Copy link
Member

@Trott Trott commented Jun 19, 2019

test/async-hooks/test-signalwrap.js passes with the test.py test runner
but fails if run directly with the node executable. Modify the test so
it passes in both cases.

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

@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. test Issues and PRs related to the tests. labels Jun 19, 2019
@addaleax addaleax mentioned this pull request Jun 19, 2019
Merged
2 tasks
@nodejs-github-bot
Copy link
Collaborator

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

bnoordhuis
bnoordhuis approved these changes Jun 20, 2019
View reviewed changes
test/async-hooks/test-signalwrap.js Outdated
// TEST_THREAD_ID is set by tools/test.py. Adjust test results depending
// on whether the test was invoked via test.py or from the shell
// directly.
const expectedLength = process.env.TEST_THREAD_ID ? 2 : 3;
Copy link
Member

Choose a reason for hiding this comment

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

Can I suggest updating the comment to explain that the third signal event listener is the SIGWINCH handler that node installs when it thinks process.stdout is a tty?

It's still kind of fragile because there might be a second SIGWINCH handler for process.stderr, i.e., a total of 4.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bnoordhuis Would the test be more robust by checking process.stdout.isTTY and process.stderr.isTTY rather than process.env.TEST_THREAD_ID?

Copy link
Member

Choose a reason for hiding this comment

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

@Trott I think that’s better, yes. There is only one async hook per active signal (which should probably be considered a bug), so currently the number is 2 + (process.stdout.isTTY || process.stderr.isTTY), I think.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 20, 2019
addaleax
addaleax approved these changes Jun 25, 2019
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM with Ben’s suggestion

@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 25, 2019
@BridgeAR BridgeAR added the wip Issues and PRs that are still a work in progress. label Jul 4, 2019
@Trott Trott force-pushed the denver-2 branch 2 times, most recently from 37f498c to 07c0428 Compare September 8, 2019 03:53
@Trott Trott removed the wip Issues and PRs that are still a work in progress. label Sep 8, 2019
@Trott
Copy link
Member Author

Trott commented Sep 8, 2019

OK, all fixed up!

@nodejs-github-bot
Copy link
Collaborator

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

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 8, 2019
@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member Author

Trott commented Sep 8, 2019

Landed in 2d1b512

@Trott Trott closed this Sep 8, 2019
@Trott Trott merged commit 2d1b512 into nodejs:master Sep 8, 2019
targos pushed a commit that referenced this pull request Sep 20, 2019
@Trott @targos
test: permit test-signalwrap to work without test runner
4bb00a5 
test/async-hooks/test-signalwrap.js passes with the test.py test runner
but fails if run directly with the `node` executable. Modify the test so
it passes in both cases.

PR-URL: #28306
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BridgeAR BridgeAR mentioned this pull request Sep 24, 2019
Merged
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
@Trott @BridgeAR
test: permit test-signalwrap to work without test runner
561d504 
test/async-hooks/test-signalwrap.js passes with the test.py test runner
but fails if run directly with the `node` executable. Modify the test so
it passes in both cases.

PR-URL: #28306
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
This was referenced Sep 25, 2019
Closed
Closed
Closed
Closed
Closed
Closed
@Trott Trott deleted the denver-2 branch January 13, 2022 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
async_hooks Issues and PRs related to the async hooks subsystem. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Trott @nodejs-github-bot @bnoordhuis @addaleax @BridgeAR