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: update fixturesDir import #15887

Closed
wants to merge 2 commits into from
Closed

test: update fixturesDir import #15887

wants to merge 2 commits into from

Conversation

seabrookmx
Copy link
Contributor

Import fixturesDir path from common/fixturesDir
module rather than from common.

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

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 6, 2017
@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. 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
@Trott
Copy link
Member

Trott commented Oct 8, 2017

Hi, @seabrookmx! Welcome and thanks for this! Can you update this to use fixtures.path()? It's documented at https://github.com/nodejs/node/tree/master/test/common#fixtures-module

const assert = require('assert');

const fs = require('fs');
const FSReadable = fs.ReadStream;

const path = require('path');
const file = path.resolve(common.fixturesDir, 'x1024.txt');
const file = path.resolve(fixturesDir, 'x1024.txt');
Copy link
Member

Choose a reason for hiding this comment

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

There is a dedicated fixtures.path function that could be used here.

@BridgeAR
Copy link
Member

BridgeAR commented Oct 9, 2017

Ah, sorry @Trott I did not read your comment before writing mine.

Import fixturesDir path from common/fixturesDir
module rather than from common.
@seabrookmx
Copy link
Contributor Author

@Trott Thanks for the link. Commit updated 🙇‍♂️

@gireeshpunathil
Copy link
Member

@seabrookmx - thanks, can you also apply @BridgeAR's recommendation to improve the code further?

const assert = require('assert');

const fs = require('fs');
const FSReadable = fs.ReadStream;

const path = require('path');
const file = path.resolve(common.fixturesDir, 'x1024.txt');
const file = path.resolve(fixtures.path(), 'x1024.txt');
Copy link
Member

@lpinca lpinca Oct 13, 2017

Choose a reason for hiding this comment

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

This could be just fixtures.path('x1024.txt').

@jasnell
Copy link
Member

jasnell commented Oct 13, 2017

@joyeecheung
Copy link
Member

Landed in bb56fbb, thank you for the contribution!

joyeecheung pushed a commit that referenced this pull request Oct 14, 2017
Import fixturesDir path from common/fixturesDir
module rather than from common.

PR-URL: #15887
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
Import fixturesDir path from common/fixturesDir
module rather than from common.

PR-URL: nodejs/node#15887
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this pull request Oct 18, 2017
Import fixturesDir path from common/fixturesDir
module rather than from common.

PR-URL: #15887
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2017
Import fixturesDir path from common/fixturesDir
module rather than from common.

PR-URL: #15887
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Nov 21, 2017
MylesBorins pushed a commit that referenced this pull request Nov 21, 2017
Import fixturesDir path from common/fixturesDir
module rather than from common.

PR-URL: #15887
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 28, 2017
Import fixturesDir path from common/fixturesDir
module rather than from common.

PR-URL: #15887
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
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. fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants