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

refactor away test/inspector #16197

Closed
wants to merge 2 commits into from
Closed

refactor away test/inspector #16197

wants to merge 2 commits into from

Conversation

maclover7
Copy link
Contributor

@maclover7 maclover7 commented Oct 14, 2017 •
edited
Loading

Refs: #13622, cc @Trott @refack

Tried to handle as much as I could in this PR, but there here are some things I'd like to handle in separate PRs:

  • Convert inspector-helper.js to a proper "common" module (write basic docs, etc.)
  • Make sure all mentions of test/inspector are gone
  • Evaluate if anymore of tests can/should be moved to test/parallel
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 14, 2017
@mscdex mscdex added inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests. labels Oct 14, 2017
@Trott
Copy link
Member

Trott commented Oct 14, 2017

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

@Trott
Copy link
Member

Trott commented Oct 14, 2017

@nodejs/v8-inspector

@Trott
Copy link
Member

Trott commented Oct 14, 2017

@maclover7 A third bullet point might be "evaluate if any of these tests can/should be moved to parallel".

bnoordhuis
bnoordhuis approved these changes Oct 15, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis 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 a request.

test/parallel/test-inspector-break-when-eval.js Outdated
const path = require('path');

const script = path.join(path.dirname(module.filename), 'global-function.js');
const script = `${common.fixturesDir}/inspector-global-function.js`;
Copy link
Member

Choose a reason for hiding this comment

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

You could use common.fixtures.path('inspector-global-function.js') here.

Makefile
@@ -401,9 +401,6 @@ test-pummel: all
test-internet: all
$(PYTHON) tools/test.py internet

test-inspector: all
$(PYTHON) tools/test.py inspector
Copy link
Member

Choose a reason for hiding this comment

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

Can you also remove it from vcbuild.bat? (Two mentions.)

refack
refack reviewed Oct 15, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Need the FLAKY flags moved as well

test/inspector/inspector.status
[true] # This section applies to all platforms

[$system==aix]
test-stop-profile-after-done: PASS, FLAKY
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't these be moved to test/parallel/parallel.status

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch -- thank you!

@refack refack self-assigned this Oct 16, 2017
@joyeecheung
Copy link
Member

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

@jasnell
Copy link
Member

jasnell commented Oct 16, 2017

Several related failures in CI, including linting issues.

@maclover7
Copy link
Contributor Author

@jasnell fixed linter issue and moved one of the tests to sequential, would you be willing to run CI again?

@refack
Copy link
Contributor

refack commented Oct 16, 2017 •
edited
Loading

Resume CI: https://ci.nodejs.org/job/node-test-commit/13201/
Windows-fanned: https://ci.nodejs.org/job/node-test-commit-windows-fanned/12564/
ARM-fanned: https://ci.nodejs.org/job/node-test-commit-arm-fanned/11754/

refack pushed a commit to refack/node that referenced this pull request Oct 17, 2017
@maclover7 @refack
test: move inspector tests to parallel/sequential
978629c 
* remove inspector directory artifacts

PR-URL: nodejs#16197
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@refack
Copy link
Contributor

refack commented Oct 17, 2017

Landed in 978629c

10x! 🎉

@refack refack closed this Oct 17, 2017
@maclover7 maclover7 deleted the jm-inspector-test branch October 18, 2017 00:17
@maclover7 maclover7 mentioned this pull request Oct 18, 2017
Closed
targos pushed a commit that referenced this pull request Oct 18, 2017
@maclover7 @targos
test: move inspector tests to parallel/sequential
1fe5308 
* remove inspector directory artifacts

PR-URL: #16197
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 18, 2017
@maclover7 @addaleax
test: move inspector tests to parallel/sequential
c76fdbb 
* remove inspector directory artifacts

PR-URL: nodejs/node#16197
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@refack
Copy link
Contributor

refack commented Oct 18, 2017

Cross-ref: #16281

@refack refack removed their assignment Oct 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@maclover7 @Trott @joyeecheung @jasnell @refack @bnoordhuis @cjihrig @mscdex @MylesBorins @nodejs-github-bot