test: use common.fixturesDir almost everywhere by bengl · Pull Request #6997 · nodejs/node

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

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

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

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

Comment threadtest/parallel/test-global.js Outdated
cjihrigMay 26, 2016
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you do this: path.join(common.fixturesDir, 'global', 'plain')

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

Trott reviewed May 27, 2016
View reviewed changes

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

cjihrig reviewed May 27, 2016
View reviewed changes

Comment threadtest/parallel/test-util-internal.js Outdated
cjihrigMay 27, 2016
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make 'syntax' and 'bad_syntax' separate arguments.

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 39812b5Compare 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

Uh oh!

Uh oh!

Conversation

bengl commented May 26, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

cjihrigMay 26, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented May 26, 2016

Uh oh!

bengl commented May 26, 2016

Uh oh!

Fishrock123 commented May 27, 2016

Uh oh!

TrottMay 27, 2016

Choose a reason for hiding this comment

Uh oh!

TrottMay 27, 2016

Choose a reason for hiding this comment

Uh oh!

benglMay 27, 2016

Choose a reason for hiding this comment

Uh oh!

Trott commented May 27, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrigMay 27, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrig commented May 27, 2016

Uh oh!

bengl commented May 27, 2016

Uh oh!

Trott commented May 27, 2016

Uh oh!

cjihrig commented Jun 1, 2016

Uh oh!

cjihrig commented Jun 2, 2016

Uh oh!

bengl commented Jun 6, 2016

Uh oh!

MylesBorins commented Jul 11, 2016

Uh oh!

MylesBorins commented Aug 30, 2016

Uh oh!

MylesBorins commented Oct 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Trott commented May 27, 2016•
edited
Loading