test: use common.fixturesDir almost everywhere #6997

bengl · 2016-05-26T17:02:19Z

Checklist

tests and code linting passes
a test and/or benchmark is included
the commit message follows commit guidelines

Affected core subsystem(s)

test

Description of change

Updating tests to use common.fixturesDir whenever possible/reasonable.
Left out things like tests for path and require.resolve.

cjihrig · 2016-05-26T17:10:48Z

test/parallel/test-global.js

@@ -13,7 +14,7 @@ assert.equal('bar',
             baseBar, // eslint-disable-line no-undef
             'global.x -> x in base level not working');

-var module = require('../fixtures/global/plain');
+var module = require(path.join(common.fixturesDir, 'global/plain'));


Can you do this: path.join(common.fixturesDir, 'global', 'plain')

jasnell · 2016-05-26T18:40:57Z

LGTM with @cjihrig's nits addressed.

bengl · 2016-05-26T22:02:06Z

@cjihrig Ok, path.resolve is now gone from all of those, and split it up where there were slashes.

Also, CI: https://ci.nodejs.org/job/node-test-pull-request/2814/

Fishrock123 · 2016-05-27T18:35:42Z

CI doesn't seem happy. I see errors besides the regular address failures in there.

Concept seems fine though.

Trott · 2016-05-27T20:41:03Z

test/parallel/test-child-process-detached.js

@@ -1,10 +1,10 @@
 'use strict';
-require('../common');
+var common = require('../common');


Nit: const

It's a nit so you can ignore it, of course, but if you do want to const-ify common here, you can also do it in the other files too. (Looks like you used var where the file was doing that for other modules and const where it was already using that. Which is also just fine. Like I said: nit.)

Right, I kept with whatever was being used in the file, for least-confusion/impact.

I think const-ifying tests makes more sense in separate PR(s)/commit(s). SGTY?

Trott · 2016-05-27T20:45:37Z

At least some of the CI problems look like things that might be addressed by a9492f5 which isn't in this branch (and the CI wasn't run to rebase against master--aside: should we change that to be the default in node-test-pull-request?).

In short, rebase against master might help with the CI results a bit.

EDIT: Whoops, used the wrong commit hash, updated it. But if you clicked on it in your email and got a puzzling result, that's why.

cjihrig · 2016-05-27T20:59:23Z

test/parallel/test-util-internal.js

@@ -43,7 +44,7 @@ assert.strictEqual(internalUtil.getHiddenValue(obj, 'foo'), 'bar');
 let arrowMessage;

 try {
-  require('../fixtures/syntax/bad_syntax');
+  require(path.join(common.fixturesDir, 'syntax/bad_syntax'));


Can you make 'syntax' and 'bad_syntax' separate arguments.

cjihrig · 2016-05-27T21:01:36Z

LGTM pending nits and the CI.

Updating tests to use `common.fixturesDir` whenever possible/reasonable. Left out things like tests for `path` and `require.resolve`.

bengl · 2016-05-27T21:09:50Z

Alright, nits addressed and new CI: https://ci.nodejs.org/job/node-test-pull-request/2828/

Trott · 2016-05-27T22:09:28Z

Looks like FreeBSD on CI might have a stray process hogging common.PORT. Just got on IRC and jbergstroem took care of it so that should fix that at least...

One more CI run because the last two have been Not Good: https://ci.nodejs.org/job/node-test-pull-request/2829/

cjihrig · 2016-06-01T19:54:31Z

One more time. CI: https://ci.nodejs.org/job/node-test-pull-request/2896/

cjihrig · 2016-06-02T16:29:41Z

@bengl the CI is green. Feel free to move forward with this.

Updating tests to use `common.fixturesDir` whenever possible/reasonable. Left out things like tests for `path` and `require.resolve`. PR-URL: #6997 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

bengl · 2016-06-06T22:30:53Z

Landed in cc2a88a

Updating tests to use `common.fixturesDir` whenever possible/reasonable. Left out things like tests for `path` and `require.resolve`. PR-URL: #6997 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins · 2016-07-11T22:50:22Z

@bengl would you be willing to backport to v4.x?

MylesBorins · 2016-08-30T18:45:42Z

ping @bengl

MylesBorins · 2016-10-10T16:31:05Z

I'm going to label this don't land. @bengl please feel free to backport

Updating tests to use `common.fixturesDir` whenever possible/reasonable. Left out things like tests for `path` and `require.resolve`. PR-URL: #6997 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label May 26, 2016

cjihrig reviewed May 26, 2016
View reviewed changes

bengl force-pushed the usefixturesdir branch from a4f097c to 81ad802 Compare May 26, 2016 21:57

Trott reviewed May 27, 2016
View reviewed changes

bengl force-pushed the usefixturesdir branch from 81ad802 to 498d992 Compare May 27, 2016 20:58

cjihrig reviewed May 27, 2016
View reviewed changes

test: use common.fixturesDir almost everywhere

39812b5

Updating tests to use `common.fixturesDir` whenever possible/reasonable. Left out things like tests for `path` and `require.resolve`.

bengl force-pushed the usefixturesdir branch from 498d992 to 39812b5 Compare May 27, 2016 21:04

bengl closed this Jun 6, 2016

evanlucas mentioned this pull request Jun 16, 2016

v6.2.2 proposal #7323

Merged

MylesBorins mentioned this pull request Jul 11, 2016

Audit commits not found on v4.x-staging #7661

Closed

MylesBorins added the lts-watch-v4.x label Jul 11, 2016

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Oct 10, 2016

targos removed the dont-land-on-v4.x label Dec 28, 2016

MylesBorins added the dont-land-on-v4.x label Jan 16, 2017

targos added land-on-v4.x and removed dont-land-on-v4.x labels Jan 23, 2017

MylesBorins mentioned this pull request Jan 24, 2017

v4.8.0 proposal #10973

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: use common.fixturesDir almost everywhere #6997

test: use common.fixturesDir almost everywhere #6997

bengl commented May 26, 2016

cjihrig May 26, 2016

jasnell commented May 26, 2016

bengl commented May 26, 2016

Fishrock123 commented May 27, 2016

Trott May 27, 2016

Trott May 27, 2016

bengl May 27, 2016

Trott commented May 27, 2016 •

edited

Loading

cjihrig May 27, 2016

cjihrig commented May 27, 2016

bengl commented May 27, 2016

Trott commented May 27, 2016

cjihrig commented Jun 1, 2016

cjihrig commented Jun 2, 2016

bengl commented Jun 6, 2016

MylesBorins commented Jul 11, 2016

MylesBorins commented Aug 30, 2016

MylesBorins commented Oct 10, 2016

test: use common.fixturesDir almost everywhere #6997

test: use common.fixturesDir almost everywhere #6997

Conversation

bengl commented May 26, 2016

Checklist

Affected core subsystem(s)

Description of change

cjihrig May 26, 2016

Choose a reason for hiding this comment

jasnell commented May 26, 2016

bengl commented May 26, 2016

Fishrock123 commented May 27, 2016

Trott May 27, 2016

Choose a reason for hiding this comment

Trott May 27, 2016

Choose a reason for hiding this comment

bengl May 27, 2016

Choose a reason for hiding this comment

Trott commented May 27, 2016 • edited Loading

cjihrig May 27, 2016

Choose a reason for hiding this comment

cjihrig commented May 27, 2016

bengl commented May 27, 2016

Trott commented May 27, 2016

cjihrig commented Jun 1, 2016

cjihrig commented Jun 2, 2016

bengl commented Jun 6, 2016

MylesBorins commented Jul 11, 2016

MylesBorins commented Aug 30, 2016

MylesBorins commented Oct 10, 2016

Trott commented May 27, 2016 •

edited

Loading