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: fix single test runner regression #15329

Closed
wants to merge 1 commit into from
Closed

test: fix single test runner regression #15329

wants to merge 1 commit into from

Conversation

TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Sep 11, 2017 •
edited by gibfahn
Loading

Alternative to #15300.

/cc @apapirovski

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

test

Refs: #15300.
Refs: #14369

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Sep 11, 2017
@TimothyGu TimothyGu mentioned this pull request Sep 11, 2017
Closed
2 tasks
Trott
Trott approved these changes Sep 11, 2017
View reviewed changes
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.

LGTM if CI is green. I'd be +1 on landing this sooner than the usual 48/72 hours.

@TimothyGu
Copy link
Member Author

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

stress-single-test with five different flavors of single tests (.js test with .js, .js test w/o .js, .mjs test with .mjs, .mjs test w/o .mjs, directory): https://ci.nodejs.org/job/node-stress-single-test/1408/nodes=ubuntu1610-x64/

@apapirovski
Copy link
Member

Thanks for doing this. Wasn't familiar with this code, good to see there's a cleaner way.

@gibfahn gibfahn mentioned this pull request Sep 11, 2017
Merged
@MylesBorins
Copy link
Contributor

I think we should consider fast tracking this commit. If it lands before the EOD we can get it into the 8.5.0 release

MylesBorins
MylesBorins approved these changes Sep 11, 2017
View reviewed changes
Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LgTM

@gibfahn
Copy link
Member

gibfahn commented Sep 11, 2017

@TimothyGu's stress test seems to have worked (it ran all the tests) so I'd be good with landing this.

Quick ping to @nodejs/python @nodejs/testing

bnoordhuis
bnoordhuis approved these changes Sep 11, 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 but the commit log should go into more detail.

The regression was introduced in #14369, right?

@MylesBorins
Copy link
Contributor

landed in cb44cd4
I took the liberty of adding details to the commit

MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
@TimothyGu @MylesBorins
test: fix single test runner regression
2adabe6 
When ESM support was added it created a regression in the test runner
that broke the ability to run individual tests. This commit
re-introduces the use of `NormalizePath` which fixes the regression
in the test runner

Refs: #15300
PR-URL: #15329
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
@TimothyGu @MylesBorins
test: fix single test runner regression
cb44cd4 
When ESM support was added it created a regression in the test runner
that broke the ability to run individual tests. This commit
re-introduces the use of `NormalizePath` which fixes the regression
in the test runner

Refs: #15300
PR-URL: #15329
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
@TimothyGu @addaleax
test: fix single test runner regression
6d89469 
When ESM support was added it created a regression in the test runner
that broke the ability to run individual tests. This commit
re-introduces the use of `NormalizePath` which fixes the regression
in the test runner

Refs: nodejs#15300
PR-URL: nodejs#15329
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@TimothyGu TimothyGu deleted the test-single-test branch September 20, 2017 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@MylesBorins MylesBorins MylesBorins approved these changes

@Trott Trott Trott approved these changes

@tniessen tniessen tniessen approved these changes

@richardlau richardlau richardlau approved these changes

@gibfahn gibfahn gibfahn approved these changes

Assignees
No one assigned
Labels
test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@TimothyGu @apapirovski @MylesBorins @gibfahn @bnoordhuis @Trott @tniessen @richardlau @nodejs-github-bot