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: add fixtures module #15984

Closed

Conversation

matthewmeyer
Copy link
Contributor

@matthewmeyer matthewmeyer commented Oct 6, 2017

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

Updated parallel/test-require-resolve.js to use the common.fixtures module
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
@tniessen
Copy link
Member

tniessen commented Oct 6, 2017

@tniessen
Copy link
Member

tniessen commented Oct 6, 2017

@mscdex mscdex added the module Issues and PRs related to the module subsystem. label Oct 6, 2017
@Trott 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 tniessen self-assigned this Oct 13, 2017
tniessen pushed a commit that referenced this pull request Oct 13, 2017
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
Copy link
Member

Landed in b1506f7, thank you for your contribution! 🎉

@tniessen tniessen closed this Oct 13, 2017
@eugeneo
Copy link
Contributor

eugeneo commented Oct 13, 2017

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
Copy link
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
tniessen added a commit that referenced this pull request Oct 14, 2017
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
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
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
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
PR-URL: #16192
Refs: #15984
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@bnoordhuis
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.