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

Closed
wants to merge 3 commits into from
Closed

test: enable trace-events tests for workers #23698

wants to merge 3 commits into from

Conversation

richardlau
Copy link
Member

@richardlau richardlau commented Oct 16, 2018 •
edited
Loading

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

@richardlau
test: enable trace-events tests for workers
d255466 
Use the `cwd` option for `child_process` instead of `process.chdir()`
to enable the trace events tests to run on workers.
@richardlau
squash: add test-trace-events-api.js
7658368 
Currently crashes when run with workers. Real bug?
@nodejs-github-bot
Copy link
Collaborator

@richardlau build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1263/pipeline

@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.

Sign in to view
@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.

Sign in to view
jasnell
jasnell approved these changes Oct 16, 2018
View reviewed changes
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.

Sign in to view
@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

node/test/parallel/test-trace-events-dynamic-enable.js

Line 6 in 25dc25b

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

@richardlau
Copy link
Member Author

richardlau commented Oct 16, 2018 •
edited
Loading

New CI: https://ci.nodejs.org/job/node-test-pull-request/17916/ ✔️

@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
@richardlau @danbev
test: enable trace-events tests for workers
23f8b7d 
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@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
@richardlau @jasnell
test: enable trace-events tests for workers
a3c6a8d 
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@richardlau richardlau mentioned this pull request Oct 25, 2018
Closed
3 tasks
@richardlau
Copy link
Member Author

Backport PR: #23882

richardlau added a commit to richardlau/node-1 that referenced this pull request Nov 2, 2018
@richardlau
test: enable trace-events tests for workers
9e39441 
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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
@richardlau @MylesBorins
test: enable trace-events tests for workers
8552bb4 
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@codebytere codebytere mentioned this pull request Nov 27, 2018
Closed
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@richardlau @rvagg
test: enable trace-events tests for workers
7c3848c 
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
@richardlau @MylesBorins
test: enable trace-events tests for workers
51ceaf5 
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@richardlau @nodejs-github-bot @jasnell @danbev @addaleax @cjihrig @BethGriggs