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: enable trace-events tests for workers #23698

Closed
wants to merge 3 commits into from

Conversation

richardlau
Copy link
Member

@richardlau richardlau commented Oct 16, 2018

Use the cwd option for child_process instead of process.chdir()
to enable the trace events tests to run on workers.

Refs: #23674 (comment)

I've split one of the tests (test-trace-events-api.js) into its own commit
because it crashes when run under workers. Is this indicative of a real
bug?

Edit: test-trace-events-api.js updated to be skipped for workers due
to #22767

cc @nodejs/trace-events @nodejs/workers

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

Use the `cwd` option for `child_process` instead of `process.chdir()`
to enable the trace events tests to run on workers.
Currently crashes when run with workers. Real bug?
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 16, 2018
@richardlau richardlau changed the title Worker traceevents test: enable trace-events tests for workers Oct 16, 2018
@richardlau

This comment has been minimized.

@richardlau richardlau added trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. worker Issues and PRs related to Worker support. labels Oct 16, 2018
@richardlau

This comment has been minimized.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM once that CI failure is looked at.

@richardlau

This comment has been minimized.

@jasnell
Copy link
Member

jasnell commented Oct 16, 2018

/cc @ofrobots

@richardlau
Copy link
Member Author

Ah looks like parallel/test-trace-events-api is failing due to #22767 and #22513. I'll restore the skip but leave a comment pointing to the issue as is currently done in

common.skipIfWorker(); // https://github.com/nodejs/node/issues/22767

@richardlau
Copy link
Member Author

richardlau commented Oct 16, 2018

@richardlau
Copy link
Member Author

image 😁

@richardlau richardlau added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 17, 2018
@danbev
Copy link
Contributor

danbev commented Oct 19, 2018

Landed in 23f8b7d.

@danbev danbev closed this Oct 19, 2018
danbev pushed a commit that referenced this pull request Oct 19, 2018
Use the `cwd` option for `child_process` instead of `process.chdir()`
to enable the trace events tests to run on workers.

PR-URL: #23698
Refs: #23674 (comment)
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@addaleax
Copy link
Member

Should this be backported to v10.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

jasnell pushed a commit that referenced this pull request Oct 21, 2018
Use the `cwd` option for `child_process` instead of `process.chdir()`
to enable the trace events tests to run on workers.

PR-URL: #23698
Refs: #23674 (comment)
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@richardlau
Copy link
Member Author

Backport PR: #23882

richardlau added a commit to richardlau/node-1 that referenced this pull request Nov 2, 2018
Use the `cwd` option for `child_process` instead of `process.chdir()`
to enable the trace events tests to run on workers.

Backport-PR-URL: nodejs#23882
PR-URL: nodejs#23698
Refs: nodejs#23674 (comment)
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>

Conflicts:
	test/parallel/test-trace-events-binding.js
	test/parallel/test-trace-events-category-used.js
MylesBorins pushed a commit that referenced this pull request Nov 4, 2018
Use the `cwd` option for `child_process` instead of `process.chdir()`
to enable the trace events tests to run on workers.

Conflicts:
	test/parallel/test-trace-events-binding.js
	test/parallel/test-trace-events-category-used.js

Backport-PR-URL: #23882
PR-URL: #23698
Refs: #23674 (comment)
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg pushed a commit that referenced this pull request Nov 28, 2018
Use the `cwd` option for `child_process` instead of `process.chdir()`
to enable the trace events tests to run on workers.

Conflicts:
	test/parallel/test-trace-events-binding.js
	test/parallel/test-trace-events-category-used.js

Backport-PR-URL: #23882
PR-URL: #23698
Refs: #23674 (comment)
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
Use the `cwd` option for `child_process` instead of `process.chdir()`
to enable the trace events tests to run on workers.

Conflicts:
	test/parallel/test-trace-events-binding.js
	test/parallel/test-trace-events-category-used.js

Backport-PR-URL: #23882
PR-URL: #23698
Refs: #23674 (comment)
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
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. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants