test: begin normalizing fixtures use #14332

jasnell · 2017-07-17T22:37:03Z

Adds a new ../common/fixtures' module to begin normalizing test/fixturesuse. Our test code is a bit inconsistent with regards to use of the fixtures directory. Some code usespath.join()`, some code uses string concats, some other
code uses template strings, etc. In mnay cases, significant
duplication of code is seen when accessing fixture files, etc.

This updates many (but by no means all) of the tests in the
test suite to use the new consistent API. There are still
many more to update, which would make an excelent Code-n-Learn
exercise.

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)

test

mscdex · 2017-07-17T22:47:50Z

Commit message has a single quote instead of a backtick FWIW

addaleax

LGTM fort the test/common changes, somewhat rubber-stamp-y LGTM for the rest

vsemozhetbyt · 2017-07-18T00:15:07Z

test/common/README.md

+
+* [&lt;String>]
+
+The absolute path to the `test/fixtures/ directory.


vsemozhetbyt · 2017-07-18T00:17:51Z

test/common/README.md

+
+* `...args` [&lt;String>]
+
+Returns the result of `path.join(fixturesDir, ...args);


vsemozhetbyt · 2017-07-18T00:18:19Z

test/common/README.md

+
+* `args` [&lt;String|Array>]
+
+Returns the result of `fs.readFileSync(path.join(fixturesDir, ...args), 'enc');


vsemozhetbyt · 2017-07-18T00:18:50Z

test/common/README.md

+
+* `arg` [&lt;String>]
+
+Returns the result of `fs.readFileSync(path.join(fixturesDir, 'keys', arg), 'enc');


vsemozhetbyt · 2017-07-18T00:23:25Z

test/common/README.md

+
+### fixtures.readFixtureSync(args[, enc])
+
+* `args` [&lt;String|Array>]


[<String>] | [<Array>]?

Trott · 2017-07-18T07:37:18Z

I'm way too tired right now to review this, but not too tired to express enthusiasm for more cohesive common modules like this for tests! 🎉

gibfahn

Some minor suggestions, and one question I think we should discuss now.

+1 on this otherwise, seems like a really set of helpers for common.

gibfahn · 2017-07-18T08:47:39Z

test/common/fixtures.js

+}
+
+function readFixtureKey(name, enc) {
+  return fs.readFileSync(fixturesPath('keys', name), enc);


Unless I'm misreading this (quite possible), readFixtureKey('agent1-key.pem') == readFixtureSync('keys', 'agent1-key.pem'),. If so I'd rather we just have the latter, having an extra method adds unnecessary complexity for no particular gain IMO.

Take another look at the signatures. readFixtureSync(...args) takes an arbitrary number of args that are concatenated using path.join(...args), whereas readFixtureKey(name, enc) takes a single path segment and an optional encoding. Yes, readFixtureKey() uses readFixtureSync() but they are not equivalent to one.

I'm not saying they're identical, I'm saying that fixtures.readKey() is a specialised version of fixtures.readSync() (the one where the first arg is keys and you can omit the encoding).

You're right, I should have said:

fixtures.readKey('agent1-key.pem') == fixtures.readSync(['keys', 'agent1-key.pem'], 'utf8')

For me the latter is clearer (which is why I don't understand the need for readFixtureKey())

Generally because I find the necessity of using the [...] syntax a bit ugly and unfriendly.

Fair enough, I have a mild preference the other way, but not strongly enough to block this.

gibfahn · 2017-07-18T08:49:53Z

test/parallel/test-async-wrap-getasyncid.js

-  const key = fs.readFileSync(common.fixturesDir + '/test_key.pem', 'ascii');
+  const ca = fs.readFileSync(fixturesPath('test_ca.pem'), 'ascii');
+  const cert = fs.readFileSync(fixturesPath('test_cert.pem'), 'ascii');
+  const key = fs.readFileSync(fixturesPath('test_key.pem'), 'ascii');


Shouldn't these be const key = readFixtureSync('test_key.pem', 'ascii');?

no... see above. I left these because the keys being read require the additional encoding arg and but the paths do not include the keys directory.

AFAICT:

(function readFixtureSync(args, enc) { if (Array.isArray(args)) return fs.readFileSync(fixturesPath(...args), enc); return fs.readFileSync(fixturesPath(args), enc); })('test_key.pem', 'ascii') // => if (Array.isArray('test_key.pem')) return fs.readFileSync(fixturesPath(...'test_key.pem'), 'ascii'); return fs.readFileSync(fixturesPath('test_key.pem'), 'ascii'); // => return fs.readFileSync(fixturesPath('test_key.pem'), 'ascii');

Q.E.D.

gibfahn · 2017-07-18T09:18:04Z

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


-const fixture = path.join(common.fixturesDir, 'exit.js');
+const fixture = fixturesPath('exit.js');


I don't like losing the common. prefix, I think the const { fixturesPath } is a really nice syntax, but when you're going through tests it's really helpful to know which variables are common. Assuming the movement towards breaking up common continues, we'll have more and more variables that aren't obviously from common.

I think the options are:

What this PR currently does, import the functions you want to use directly from fixtures, so you use with fixturesPath().

Don't use the { fixturesPath } require syntax, so you do const fixtures = require('../common/fixtures), and use with fixtures.fixturesPath() (or maybe just fixtures.path()).

Import and re-export each function/object from the common module, so you do common.fixturesPath().

Import and re-export fixtures from common, so you do common.fixtures.fixturesPath() (or common.fixtures.path()).

I think I'd prefer 4., interested to see what other people think.

-1 ... I personally prefer the more compact syntax.

I like option 2. It makes it obvious where the function came from without having to find the declaration and it allows us the option to continue to make common less monolithic.

That said, I'm actually OK with anything. Just, I like option 2 the best.

cc/ @nodejs/collaborators, I'd like to know what others think.

I'd like to use option 2 too

Option 2 is fine.

@jasnell taking this test as an example, going forward when looking at this test one has to remember that fixturesPath is from common, and fixture is a local variable defined in this test. Multiply that by 1500 tests and five common modules and things get complex.

More compact syntax seems to me to be favouring writability over readability, I'd rather do the opposite.

I'm fine with 2., it's slightly more mental load, but not much, and fixtures.path() is no less compact than fixturesPath() 😁

gibfahn · 2017-07-18T09:23:52Z

test/common/README.md

+
+* `...args` [&lt;String>]
+
+Returns the result of `path.join(fixturesDir, ...args)`.


Maybe add a sentence for each of these, e.g.:

Returns the absolute path to the specified fixture, i.e. the result of `path.join(fixturesDir, ...args)`.

Not sure if that's much better.

@jasnell do you think it's worse? Open to alternatives.

I think the docs should add something to the code, if the docs just repeat the code then what's the point?

I just don't see the suggested bits as adding any significant value. The short bit that is there highlights the code that is being replaced by the common utility, the fact that it's an absolute path is largely irrelevant.

gibfahn · 2017-07-18T09:25:01Z

test/common/README.md

+
+* `args` [&lt;String>] | [&lt;Array>]
+
+Returns the result of `fs.readFileSync(path.join(fixturesDir, ...args), 'enc')`.


Maybe

Takes the relative path to the fixture, and returns the contents of the file, ie. the result of `fs.readFileSync(path.join(fixturesDir, ...args), 'enc')`.

jasnell · 2017-07-24T16:49:44Z

Rebased...

jasnell · 2017-07-24T16:52:53Z

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

jasnell · 2017-07-25T05:03:56Z

Updated to use fixtures.path, fixtures.readSync and fixtures.readKey

gibfahn · 2017-07-26T11:11:42Z

test/common/fixtures.js

+}
+
+function readFixtureKey(name, enc) {
+  return fs.readFileSync(fixturesPath('keys', name), enc);


I'm not saying they're identical, I'm saying that fixtures.readKey() is a specialised version of fixtures.readSync() (the one where the first arg is keys and you can omit the encoding).

You're right, I should have said:

fixtures.readKey('agent1-key.pem') == fixtures.readSync(['keys', 'agent1-key.pem'], 'utf8')

For me the latter is clearer (which is why I don't understand the need for readFixtureKey())

gibfahn · 2017-07-26T11:18:18Z

test/parallel/test-async-wrap-getasyncid.js

-  const key = fs.readFileSync(common.fixturesDir + '/test_key.pem', 'ascii');
+  const ca = fs.readFileSync(fixtures.path('test_ca.pem'), 'ascii');
+  const cert = fs.readFileSync(fixtures.path('test_cert.pem'), 'ascii');
+  const key = fs.readFileSync(fixtures.path('test_key.pem'), 'ascii');


@jasnell re/ #14332 (comment)

I don't understand, the test passes for me with this change:

- const ca = fs.readFileSync(common.fixturesDir + '/test_ca.pem', 'ascii'); - const cert = fs.readFileSync(common.fixturesDir + '/test_cert.pem', 'ascii'); - const key = fs.readFileSync(common.fixturesDir + '/test_key.pem', 'ascii'); + const ca = fixtures.readSync('test_ca.pem', 'ascii'); + const cert = fixtures.readSync('test_cert.pem', 'ascii'); + const key = fixtures.readSync('test_key.pem', 'ascii');

heh... I misread your original comment on this :-). You're right :-)

jasnell · 2017-08-03T18:51:52Z

@gibfahn ... fixed the one issue and provided an explanation for the other. Can you please take another look when you get a moment. Thank you!

jasnell · 2017-08-04T14:31:43Z

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

gibfahn

Looks great, thanks @jasnell

gibfahn · 2017-08-04T15:29:35Z

test/parallel/test-zlib-from-gzip.js

-const fixture = path.resolve(common.fixturesDir, 'person.jpg.gz');
-const unzippedFixture = path.resolve(common.fixturesDir, 'person.jpg');
+const fixture = fixtures.path('person.jpg.gz');
+const unzippedFixture = fixtures.path('person.jpg');
 const outputFile = path.resolve(common.tmpDir, 'person.jpg');
 const expect = fs.readFileSync(unzippedFixture);


Doesn't really need to be part of this PR, but this could now be:

const expect = fixtures.readFile('person.jpg');

right?

gibfahn · 2017-08-04T15:31:04Z

test/common/fixtures.js

+}
+
+function readFixtureKey(name, enc) {
+  return fs.readFileSync(fixturesPath('keys', name), enc);


Fair enough, I have a mild preference the other way, but not strongly enough to block this.

jasnell · 2017-08-08T00:19:12Z

Final CI before landing: https://ci.nodejs.org/job/node-test-pull-request/9530/

Adds a new `../common/fixtures' module to begin normalizing `test/fixtures` use. Our test code is a bit inconsistent with regards to use of the fixtures directory. Some code uses `path.join()`, some code uses string concats, some other code uses template strings, etc. In mnay cases, significant duplication of code is seen when accessing fixture files, etc. This updates many (but by no means all) of the tests in the test suite to use the new consistent API. There are still many more to update, which would make an excelent Code-n-Learn exercise.

Refs: nodejs#14332

PR-URL: #14716 Refs: #14332 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>

PR-URL: nodejs/node#14716 Refs: nodejs/node#14332 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>

Adds a new `../common/fixtures' module to begin normalizing `test/fixtures` use. Our test code is a bit inconsistent with regards to use of the fixtures directory. Some code uses `path.join()`, some code uses string concats, some other code uses template strings, etc. In mnay cases, significant duplication of code is seen when accessing fixture files, etc. This updates many (but by no means all) of the tests in the test suite to use the new consistent API. There are still many more to update, which would make an excelent Code-n-Learn exercise. PR-URL: #14332 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

PR-URL: #14716 Refs: #14332 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]>

MylesBorins · 2017-09-20T15:07:08Z

This should likely be backported to v6.x-staging as it introduces a decent sized delta.

@jasnell would you be able to backport?

jasnell · 2017-09-20T22:11:40Z

Not anytime within the next couple of weeks.

refack · 2017-09-20T23:14:04Z

I'm give it a go.

MylesBorins · 2017-10-17T20:17:52Z

ping @refack this blocking landing a whole bunch of the code and learn patches to v6.x

refack · 2017-10-17T21:21:50Z

Oh it now.

Adds a new `../common/fixtures' module to begin normalizing `test/fixtures` use. Our test code is a bit inconsistent with regards to use of the fixtures directory. Some code uses `path.join()`, some code uses string concats, some other code uses template strings, etc. In mnay cases, significant duplication of code is seen when accessing fixture files, etc. This updates many (but by no means all) of the tests in the test suite to use the new consistent API. There are still many more to update, which would make an excelent Code-n-Learn exercise. PR-URL: nodejs#14332 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

refack · 2017-10-17T22:06:21Z

Backport PR: #16265

Adds a new `../common/fixtures' module to begin normalizing `test/fixtures` use. Our test code is a bit inconsistent with regards to use of the fixtures directory. Some code uses `path.join()`, some code uses string concats, some other code uses template strings, etc. In mnay cases, significant duplication of code is seen when accessing fixture files, etc. This updates many (but by no means all) of the tests in the test suite to use the new consistent API. There are still many more to update, which would make an excelent Code-n-Learn exercise. Backport-PR-URL: #16265 PR-URL: #14332 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 17, 2017

addaleax approved these changes Jul 17, 2017

View reviewed changes

vsemozhetbyt reviewed Jul 18, 2017

View reviewed changes

gibfahn suggested changes Jul 18, 2017

View reviewed changes

cjihrig approved these changes Jul 18, 2017

View reviewed changes

jasnell force-pushed the test-common-fixture branch from 7883d07 to fc179c7 Compare July 24, 2017 16:49

jasnell force-pushed the test-common-fixture branch from d3ff72f to 5d870ac Compare July 25, 2017 05:06

gibfahn suggested changes Jul 26, 2017

View reviewed changes

tniessen approved these changes Jul 26, 2017

View reviewed changes

jasnell force-pushed the test-common-fixture branch 2 times, most recently from 530950e to 1205a5d Compare August 3, 2017 18:51

targos approved these changes Aug 3, 2017

View reviewed changes

jasnell force-pushed the test-common-fixture branch from 1205a5d to fbb654e Compare August 4, 2017 00:14

gibfahn approved these changes Aug 5, 2017

View reviewed changes

jasnell force-pushed the test-common-fixture branch 2 times, most recently from 8ca4083 to 78e5757 Compare August 8, 2017 00:15

jasnell force-pushed the test-common-fixture branch from 78e5757 to 81e35b4 Compare August 8, 2017 00:33

maasencioh added a commit to maasencioh/node that referenced this pull request Aug 24, 2017

test: continue normalizing fixtures use

5786956

Refs: nodejs#14332

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins added the backport-requested-v6.x label Sep 20, 2017

refack self-assigned this Sep 20, 2017

refack mentioned this pull request Oct 17, 2017

[backport to v6] test: begin normalizing fixtures use #16265

Closed

4 tasks

MylesBorins added backported-to-v6.x and removed backport-requested-v6.x labels Oct 19, 2017

MylesBorins mentioned this pull request Oct 19, 2017

v6.12.0 proposal #16263

Merged

MylesBorins mentioned this pull request Nov 3, 2017

v4.8.6 proposal #16500

Merged

sercanyersen mentioned this pull request Nov 6, 2017

test: use fixtures module for path resolve #16842

Closed

2 tasks

refack removed their assignment Oct 12, 2018


		* [<String>]

		The absolute path to the `test/fixtures/ directory.


		* `...args` [<String>]

		Returns the result of `path.join(fixturesDir, ...args);


		* `args` [<String\|Array>]

		Returns the result of `fs.readFileSync(path.join(fixturesDir, ...args), 'enc');


		* `arg` [<String>]

		Returns the result of `fs.readFileSync(path.join(fixturesDir, 'keys', arg), 'enc');


		### fixtures.readFixtureSync(args[, enc])

		* `args` [<String\|Array>]


		const fixture = path.join(common.fixturesDir, 'exit.js');
		const fixture = fixturesPath('exit.js');


		* `args` [<String>] \| [<Array>]

		Returns the result of `fs.readFileSync(path.join(fixturesDir, ...args), 'enc')`.

test: begin normalizing fixtures use #14332

test: begin normalizing fixtures use #14332

Conversation

jasnell commented Jul 17, 2017

Checklist

Affected core subsystem(s)

mscdex commented Jul 17, 2017

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Jul 18, 2017 • edited Loading

gibfahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Jul 24, 2017

jasnell commented Jul 24, 2017

jasnell commented Jul 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Aug 3, 2017

jasnell commented Aug 4, 2017

gibfahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Aug 8, 2017

MylesBorins commented Sep 20, 2017

jasnell commented Sep 20, 2017

refack commented Sep 20, 2017

MylesBorins commented Oct 17, 2017

refack commented Oct 17, 2017

refack commented Oct 17, 2017

Trott commented Jul 18, 2017 •

edited

Loading

gibfahn Jul 25, 2017 •

edited

Loading

gibfahn Jul 25, 2017 •

edited

Loading