FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: add fixtures module by matthewmeyer · Pull Request #15984 · nodejs/node · GitHub

/ node Public

test: add fixtures module - #15984

Closed
matthewmeyer wants to merge 3 commits into
nodejs:masterfrom
matthewmeyer:fixCommonsTestFixture
ConversationCommits (3)ChecksFiles changed
Closed

test: add fixtures module#15984
matthewmeyer wants to merge 3 commits into
nodejs:masterfrom
matthewmeyer:fixCommonsTestFixture

Conversation

matthewmeyer commented Oct 6, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Replaced common.fixturesDir with usage of the common.fixtures module

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows [commit guidelines]
Affected core subsystem(s)

test

test: add fixtures module
00215ea 
Updated parallel/test-require-resolve.js to use the common.fixtures module
nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017

tniessen commented Oct 6, 2017

Copy link
Copy Markdown
Member

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

tniessen commented Oct 6, 2017

Copy link
Copy Markdown
Member

New linter CI: https://ci.nodejs.org/job/node-test-linter/12257/

mscdex added the module Issues and PRs related to the module subsystem. label Oct 6, 2017
Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 6, 2017
tniessen self-assigned this Oct 13, 2017
tniessen pushed a commit that referenced this pull request Oct 13, 2017
test: use fixtures.path instead of fixturesDir
b1506f7 
Updated parallel/test-require-resolve.js to use the fixtures module.

PR-URL: #15984
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

tniessen commented Oct 13, 2017

Copy link
Copy Markdown
Member

Landed in b1506f7, thank you for your contribution! 🎉

tniessen closed this Oct 13, 2017

eugeneo commented Oct 13, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

b1506f7 fails on my local Ubuntu setup:

node/test/parallel/test-require-resolve.js
module.js:507
    throw err;
    ^

Error: Cannot find module '(absolute path to my local code)/node/test/fixtures/a'

I verified locally that rolling it back fixes regression.

tniessen commented Oct 13, 2017

Copy link
Copy Markdown
Member

@eugeneo Does this fix your problem?

index 27fb3f71f7..77f07b394f 100644
--- test/parallel/test-require-resolve.js
+++ test/parallel/test-require-resolve.js
@@ -26,7 +26,7 @@ const assert = require('assert');

 assert.strictEqual(
   fixtures.path('a.js').toLowerCase(),
-  require.resolve(fixtures.path('a').toLowerCase()));
+  require.resolve(fixtures.path('a')).toLowerCase());
 assert.strictEqual(
   fixtures.path('a.js').toLowerCase(),
   require.resolve(fixtures.path('a')).toLowerCase());

tniessen added a commit to tniessen/node that referenced this pull request Oct 13, 2017
test: fix regression in test-require-resolver.js
2f14f95 
Refs: nodejs#15984
tniessen mentioned this pull request Oct 13, 2017
Closed
2 tasks
tniessen added a commit that referenced this pull request Oct 14, 2017
test: fix regression in test-require-resolver.js
91a465c 
PR-URL: #16192
Refs: #15984
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
test: use fixtures.path instead of fixturesDir
e7898b7 
Updated parallel/test-require-resolve.js to use the fixtures module.

PR-URL: nodejs/node#15984
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
test: fix regression in test-require-resolver.js
0608636 
PR-URL: nodejs/node#16192
Refs: nodejs/node#15984
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
targos pushed a commit that referenced this pull request Oct 18, 2017
test: use fixtures.path instead of fixturesDir
b054a4e 
Updated parallel/test-require-resolve.js to use the fixtures module.

PR-URL: #15984
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
targos pushed a commit that referenced this pull request Oct 18, 2017
test: fix regression in test-require-resolver.js
816acc9 
PR-URL: #16192
Refs: #15984
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

bnoordhuis commented Oct 25, 2017

Copy link
Copy Markdown
Member

Seems there were not one but two regressions, see #16357 and #16486.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
lpinca lpinca approved these changes
tniessen tniessen approved these changes
BridgeAR BridgeAR approved these changes

Assignees

tniessen

Labels

code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. module Issues and PRs related to the module subsystem. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL