test, fs: introduce helper function to test fs.* methods as fsPromises.* methods #20439
Conversation
Trott
added
fs
|
+1 on the approach. |
|
Great! 👍 Note that fs/promises has two alternative methods to be used - a method on the object and a "global" one accepting the object as the first argument. So that should be three checks in total? |
Are you talking about, for example: |
test/common/README.md
Outdated
| * return [<boolean>] | ||
|
|
||
| Checks if `pathname` exists | ||
|
|
||
| ### fsTest(method, args, setup) |
There was a problem hiding this comment.
Last argument should be options right?
test/common/README.md
Outdated
| ### fsTest(method, args, setup) | ||
| * `method` [<string>] | ||
| * `args` [<Array>] | ||
| * `options` [<Function>] |
test/parallel/test-fs-link.js
Outdated
| function callback(err) { | ||
| assert.ifError(err); | ||
| const dstContent = fs.readFileSync(dstPath, 'utf8'); | ||
| assert.strictEqual('hello world', dstContent); |
| @@ -120,11 +120,28 @@ an expected warning does not have a code then `common.noWarnCode` can be used | |||
| to indicate this. | |||
|
|
|||
| ### fileExists(pathname) | |||
| * pathname [<string>] | |||
| * `pathname` [<string>] | |||
| * return [<boolean>] | |||
|
|
|||
| Checks if `pathname` exists | |||
There was a problem hiding this comment.
Should here be a period? (and in some other places as well.)
There was a problem hiding this comment.
If so, that would be a large diff that I would prefer to save for another PR. Personally, I'd prefer no period at the end of lines like this, but as always, consistency would trump my preference. :-D
test/common/README.md
Outdated
| into one that takes a callback. The last value in `args` must be a callback that | ||
| is expected to run once for each of the two invocations. | ||
|
|
||
| The options object may contain a `setup` property that is a function that is run |
There was a problem hiding this comment.
options object -> `options` object?
test/common/README.md
Outdated
|
|
||
| The options object may contain a `setup` property that is a function that is run | ||
| before each test. This function might refresh the temporary directory if both | ||
| tests need to use it, for example. The options object may also contain a |
There was a problem hiding this comment.
options object -> `options` object?
|
@Trott Looks like this function tests only methods that don't expect Everything with the I guess it could be reversed the other way around — if the first argument is integer — construct About modifying (e.g. for (const type of ['callback', 'promise', 'class']) {
…bootstrap…
fsTest[type]('unlink', ...args);
…cleanup…
}Perhaps even the whole fs tests file could be just wrapped in that — then we mostly wouldn't need to use a function that tests all three variants at the same time, mostly wouldn't care if the first argument (previously returned by the previous Thoughts? |
Trott
force-pushed
the
fs-callbackify
branch
2 times, most recently
from
4921ce2
to
d4cd54e
Compare
|
@ChALkeR Modifying functions can be made to test properly using the The |
Trott
force-pushed
the
fs-callbackify
branch
2 times, most recently
from
f5faa2f
to
9bcc7f4
Compare
|
@ChALkeR Here's my first pass at solving the problems you flagged. https://github.com/Trott/io.js/blob/9bcc7f474f006266919eabec89c70fa6791ce149/test/parallel/test-fs-append-file.js#L114-L150 I introduced an option called |
|
Making a checklist of tests that need to be looked at and (if necessary) updated to test Promise-based APIs too.
|
|
@Trott I'm thinking — how hard would it be to mock callback-based That should minimize the number of changes inside the tests themselves. |
|
I think it is great that we have this PR as we likely end up more often in this situation if we plan on adding native promise support to other APIs as well. I am thinking about a maybe more controversial approach that would not need any changes to the existing test cases and tests future test cases as well: if we monkey patch all callback That approach should also be possible for functions where we have slightly different inputs in case we are able to unify inputs before passing them through. We could also provide helpers to e.g. add a file after deleting it so we can check for that as well. I am not sure if I miss something but I doubt that this would have negative effects, since we control everything involved. Other opinions? |
@ChALkeR I'm not sure I understand the suggestion. Can you explain it a little more? |
|
@BridgeAR Not necessarily a deal-breaker for it, but one thing about the approach you suggest is that it probably breaks down for tests with nested callbacks. For example, if In the approach in this PR, that same issue comes up. The solution I've used so far will work (and is hopefully sufficient for initial landing) but I'm open to the idea that it can be improved for sure. |
|
@Trott quick and dirty PoC: const fs = require('fs');
const fsPromises = require('fs/promises')
const assert = require('assert');
const util = require('util');
function createFsMock(classes = false) {
const methods = Object.keys(fs);
const fsMock = {};
for (const key of methods) {
if (
key.endsWith('Sync') || key.endsWith('Stream') || /^[A-Z_]/.test(key) ||
key.includes('watch') || key === 'exists' ||
typeof fs[key] !== 'function'
) {
assert.strictEqual(fsPromises[key], undefined);
fsMock[key] = fs[key];
continue;
}
const head = fs[key].toString().replace(/\n[\s\S]*/, '').replace(/\s/g, '');
const fd = head.includes('(fd,');
if (classes && fd || key === 'close' && !fsPromises.close) {
fsMock[key] = async (h, ...args) => h[key.replace(/^f/, '')](...args);
} else {
fsMock[key] = fsPromises[key];
}
assert.ok(fsMock[key], key);
fsMock[key] = util.callbackify(fsMock[key]);
}
return fsMock;
}
const fsImpls = [fs, createFsMock(false), createFsMock(true)];
module.exports = fsImpls;Example usage: for (const fs of fsImpls) {
const name = `tmp.fstest.${Math.random().toString(36).slice(2)}`;
fs.open(name, 'w', (err, res) => {
assert.ok(!err && res);
const fd = res;
fs.write(fd, 'boo', (err, res) => {
assert.ok(!err && res);
fs.write(fd, 'foo', (err, res) => {
assert.ok(!err && res);
fs.close(fd, err => {
assert.ok(!err);
fs.readFile(name, 'utf-8', (err, res) => {
assert.ok(!err);
assert.strictEqual(res, 'boofoo');
fs.unlink(name, err => {
assert.ok(!err);
fs.access(name, (err, res) => {
assert.ok(err && !res);
});
});
});
});
});
});
});
} |
Add helper function to the test `common` module that simplifies converting tests for fs.* methods to be for both fs.* and fsPromises.*.
Make test-fs-link test both fs.link() and fsPromises.link().
Make test-fs-access test both fs.access() and fsPromises.access().
Make test-fs-unlink-type-check test both fs.access() and fsPromises.access().
Make test-fs-append-file.js test both fs.appendFile() and fsPromises.appendFile().
Where relevant, apply tests in test-fs-assert-encoding-error.js to equivalent functions in fsPromises.
Make test-fs-readfile-tostring-fail test both fs.readfile() and fsPromises.readfile().
Make test-fs-append-file test both fs.* methods and fsPromises.* methods.
Make test-fs-chmod test both fs and fsPromises methods.
Make test-fs-chown-type-check test both fs.chown() and fsPromises.chown().
ChALkeR
added
experimental
|
Ill be doing this one: test/parallel/test-fs-write-stream-end.js |
|
@liron-navon I'm not sure modeling on this is the way to go. There seems to be some question as to whether it is the right approach and I've started picking things off independently more like in #20739 and #20667. |
|
Definitely needs a rebase. |
Introduce
common.fsTest()helper function which (hopefully) makes it easy to repurpose alltest-fs-*tests to also test equivalent functionality infsPromises.*methods. As a proof-of-concept, use the helper function in test-fs-access and test-fs-link. If people think this is a reasonable approach, the remaining 131 test-fs-* files can probably be done relatively quickly.@ChALkeR @jasnell @nodejs/testing @nodejs/fs
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes