-
Notifications
You must be signed in to change notification settings - Fork 29k
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
fs: work with any TypedArrays instead of only Uint8Arrays #22150
Conversation
cc: @TimothyGu So, i'm not 100% sure what behaviour should be expected, when writing and reading to and from files, using TypedArrays. For instance, const input = Uint8Array.from(Buffer.from(s, 'utf8')); with any other |
a53af6e
to
5204651
Compare
We need tests to make sure it's handled correctly on the C++ side |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, reading this again tests are missing - if we officially support those types we need to make sure we have tests making sure we don't break that.
If you haven't wrote tests for PRs before and want help with writing the tests or aren't sure what/where to do let me know and I'll elaborate on how that works. |
@benjamingr I'm happy to write tests, but I'm not sure what the behaviour should be, when writing to files via TypedArrays or reading from them. Hence the hesitation. Maybe I could make copies of |
@SirR4T basically, to check that you can perform each FS operations that supports other types of arrays and that it returns the value you expect - |
5204651
to
ae2d513
Compare
That is because const a = new Uint8Array([1, 2, 3])
// In memory: 0x01 0x02 0x03
const b = Uint16Array.from(a);
// In memory 0x0001 0x0002 0x0003 However, if the test doesn't work with For testing, you could use |
Also, I'd be fine if you modify existing tests rather than creating new ones. |
ae2d513
to
bcaacaa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benjamingr : going ahead with @TimothyGu 's suggestion, modifying the existing test case.
for (const type of types) { | ||
const expect = convertToType(type, expectBuf); | ||
fs.writeFileSync(filename, expect); | ||
assert.strictEqual(convertToType(type, readInput(filename)), expect); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benjamingr this test is currently failing with
assert.js:84
throw new AssertionError(obj);
^
AssertionError [ERR_ASSERTION]: Expected inputs to be strictly equal:
+ actual - expected ... Lines skipped
Int8Array [
+ 45,
+ 50,
+ 55,
+ 44,
+ 45,
+ 49,
+ 49,
+ 53,
+ 44,
+ 45,
...
- -27,
- -115,
- -105,
- -24,
- -74,
- -118,
- -27,
- -101,
- -67,
- -26,
...
What am i doing wrong?
@TimothyGu makes sense, going ahead with the current test case. Will modify the test case name too. Currently, it is failing with
I notice that:
Can't shake the feeling i'm doing something terribly wrong here. |
bcaacaa
to
bc3b58e
Compare
@TimothyGu @benjamingr : The file contents are a string of numbers (positive and negative), concatenated by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hope this answers your question.
assert.strictEqual(fs.readFileSync(filename, 'utf8'), s); | ||
for (const expectView of common.getArrayBufferViews(inputBuffer)) { | ||
fs.writeFileSync(filename, expectView); | ||
assert.strictEqual(fs.readFileSync(filename, 'utf8'), expectView); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fs.readFileSync(..., 'utf8')
returns a string, so the assertion should compare with s.repeat(8)
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah! indeed. so that will work for reads that are string based, that is, for utf8
strings, Buffer
s, Uint8Array
s. How should I test for the other typed arrays and data views?
For instance, when encoding is not specified fs.readFileSync()
returns a Buffer. How do i match this with Int8Array
s or other typed arrays?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't. The test is testing if writeFile
supports non-Buffer
TypedArrays. It's not testing readFileSync
, and you should always compare the result of readFileSync
to the string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aha! things are much more clearer now, thanks for the explanation 😄
})); | ||
fs.readFile(filename, 'utf8', common.mustCall((err, data) => { | ||
assert.ifError(err); | ||
assert.strictEqual(data, expectView); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
@@ -17,13 +17,18 @@ const s = '南越国是前203年至前111年存在于岭南地区的一个国家 | |||
'历经五代君主。南越国是岭南地区的第一个有记载的政权国家,采用封建制和郡县制并存的制度,' + | |||
'它的建立保证了秦末乱世岭南地区社会秩序的稳定,有效的改善了岭南地区落后的政治、##济现状。\n'; | |||
|
|||
const input = Uint8Array.from(Buffer.from(s, 'utf8')); | |||
const inputBuffer = Buffer.from(s.repeat(8), 'utf8'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment explaining why .repeat(8)
is necessary? Specifically, that getArrayBufferViews
expects it.
Could you please rebase this branch on master? |
@benjamingr tests added, and now passing. @TimothyGu I can see that this diff of my feature branch against upstream/master is showing the correct files changed, but this PR is showing many more commits. How do I fix this? Should I close this and raise a different PR? This is after rebasing against latest upstream master. |
@SirR4T I see that your branch has this merge commit: SirR4T@590d218 You would need to get rid of that merge commit for things to work. I think just doing a |
590d218
to
0b78626
Compare
lib/fs.js
Outdated
@@ -525,7 +524,7 @@ function write(fd, buffer, offset, length, position, callback) { | |||
const req = new FSReqCallback(); | |||
req.oncomplete = wrapper; | |||
|
|||
if (isUint8Array(buffer)) { | |||
if (ArrayBuffer.isView(buffer)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably use https://github.com/nodejs/node/blob/master/lib/internal/util/types.js#L22 here and later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, will do that.
fs.writeFileSync(filename, input); | ||
assert.strictEqual(fs.readFileSync(filename, 'utf8'), s); | ||
for (const expectView of common.getArrayBufferViews(inputBuffer)) { | ||
debuglog('Running test for: ', expectView[Symbol.toStringTag]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we just use console.log
in tests, don't we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh... I'm not sure what the current convention is, but following the discussion at #9314, I had assumed debug log was the consensus. Lemme know if I should change to console.log.
If I understand correctly, console.log statements are not printed when tests are run, but debuglog statements are printed atleast when tests fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once @lundibundi's comments are addressed.
@SirR4T seems like the CI is failing on the new test. Could you take a look? |
Should this be backported to |
The conflict is trivial. I have a good version of this change locally. Currently compiling/running tests. |
PR-URL: #22150 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Using the same filename for different async tests could lead to race conditions. Example failure: https://travis-ci.com/nodejs/node/jobs/143351655 Refs: nodejs#22150
Using the same filename for different async tests could lead to race conditions. Example failure: https://travis-ci.com/nodejs/node/jobs/143351655 Refs: #22150 PR-URL: #22659 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
PR-URL: #22150 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Using the same filename for different async tests could lead to race conditions. Example failure: https://travis-ci.com/nodejs/node/jobs/143351655 Refs: #22150 PR-URL: #22659 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Notable changes: * child_process: * `TypedArray` and `DataView` values are now accepted as input by `execFileSync` and `spawnSync`. #22409 * coverage: * Native V8 code coverage information can now be output to disk by setting the environment variable `NODE_V8_COVERAGE` to a directory. #22527 * deps: * The bundled npm was upgraded to version 6.4.1. #22591 * Changelogs: [6.3.0-next.0](https://github.com/npm/cli/releases/tag/v6.3.0-next.0) [6.3.0](https://github.com/npm/cli/releases/tag/v6.3.0) [6.4.0](https://github.com/npm/cli/releases/tag/v6.4.0) [6.4.1](https://github.com/npm/cli/releases/tag/v6.4.1) * fs: * The methods `fs.read`, `fs.readSync`, `fs.write`, `fs.writeSync`, `fs.writeFile` and `fs.writeFileSync` now all accept `TypedArray` and `DataView` objects. #22150 * A new boolean option, `withFileTypes`, can be passed to to `fs.readdir` and `fs.readdirSync`. If set to true, the methods return an array of directory entries. These are objects that can be used to determine the type of each entry and filter them based on that without calling `fs.stat`. #22020 * http2: * The `http2` module is no longer experimental. #22466 * os: * Added two new methods: `os.getPriority` and `os.setPriority`, allowing to manipulate the scheduling priority of processes. #22394 * process: * Added `process.allowedNodeEnvironmentFlags`. This object can be used to programmatically validate and list flags that are allowed in the `NODE_OPTIONS` environment variable. #19335 * src: * Deprecated option variables in public C++ API. #22392 * Refactored options parsing. #22392 * vm: * Added `vm.compileFunction`, a method to create new JavaScript functions from a source body, with options similar to those of the other `vm` methods. #21571 * Added new collaborators: * [lundibundi](https://github.com/lundibundi) - Denys Otrishko PR-URL: #22716
PR-URL: #22150 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Using the same filename for different async tests could lead to race conditions. Example failure: https://travis-ci.com/nodejs/node/jobs/143351655 Refs: #22150 PR-URL: #22659 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Notable changes: * child_process: * `TypedArray` and `DataView` values are now accepted as input by `execFileSync` and `spawnSync`. #22409 * coverage: * Native V8 code coverage information can now be output to disk by setting the environment variable `NODE_V8_COVERAGE` to a directory. #22527 * deps: * The bundled npm was upgraded to version 6.4.1. #22591 * Changelogs: [6.3.0-next.0](https://github.com/npm/cli/releases/tag/v6.3.0-next.0) [6.3.0](https://github.com/npm/cli/releases/tag/v6.3.0) [6.4.0](https://github.com/npm/cli/releases/tag/v6.4.0) [6.4.1](https://github.com/npm/cli/releases/tag/v6.4.1) * fs: * The methods `fs.read`, `fs.readSync`, `fs.write`, `fs.writeSync`, `fs.writeFile` and `fs.writeFileSync` now all accept `TypedArray` and `DataView` objects. #22150 * A new boolean option, `withFileTypes`, can be passed to to `fs.readdir` and `fs.readdirSync`. If set to true, the methods return an array of directory entries. These are objects that can be used to determine the type of each entry and filter them based on that without calling `fs.stat`. #22020 * http2: * The `http2` module is no longer experimental. #22466 * os: * Added two new methods: `os.getPriority` and `os.setPriority`, allowing to manipulate the scheduling priority of processes. #22407 * process: * Added `process.allowedNodeEnvironmentFlags`. This object can be used to programmatically validate and list flags that are allowed in the `NODE_OPTIONS` environment variable. #19335 * src: * Deprecated option variables in public C++ API. #22515 * Refactored options parsing. #22392 * vm: * Added `vm.compileFunction`, a method to create new JavaScript functions from a source body, with options similar to those of the other `vm` methods. #21571 * Added new collaborators: * [lundibundi](https://github.com/lundibundi) - Denys Otrishko PR-URL: #22716
Notable changes: * child_process: * `TypedArray` and `DataView` values are now accepted as input by `execFileSync` and `spawnSync`. #22409 * coverage: * Native V8 code coverage information can now be output to disk by setting the environment variable `NODE_V8_COVERAGE` to a directory. #22527 * deps: * The bundled npm was upgraded to version 6.4.1. #22591 * Changelogs: [6.3.0-next.0](https://github.com/npm/cli/releases/tag/v6.3.0-next.0) [6.3.0](https://github.com/npm/cli/releases/tag/v6.3.0) [6.4.0](https://github.com/npm/cli/releases/tag/v6.4.0) [6.4.1](https://github.com/npm/cli/releases/tag/v6.4.1) * fs: * The methods `fs.read`, `fs.readSync`, `fs.write`, `fs.writeSync`, `fs.writeFile` and `fs.writeFileSync` now all accept `TypedArray` and `DataView` objects. #22150 * A new boolean option, `withFileTypes`, can be passed to to `fs.readdir` and `fs.readdirSync`. If set to true, the methods return an array of directory entries. These are objects that can be used to determine the type of each entry and filter them based on that without calling `fs.stat`. #22020 * http2: * The `http2` module is no longer experimental. #22466 * os: * Added two new methods: `os.getPriority` and `os.setPriority`, allowing to manipulate the scheduling priority of processes. #22407 * process: * Added `process.allowedNodeEnvironmentFlags`. This object can be used to programmatically validate and list flags that are allowed in the `NODE_OPTIONS` environment variable. #19335 * src: * Deprecated option variables in public C++ API. #22515 * Refactored options parsing. #22392 * vm: * Added `vm.compileFunction`, a method to create new JavaScript functions from a source body, with options similar to those of the other `vm` methods. #21571 * Added new collaborators: * [lundibundi](https://github.com/lundibundi) - Denys Otrishko PR-URL: #22716
Notable changes: * child_process: * `TypedArray` and `DataView` values are now accepted as input by `execFileSync` and `spawnSync`. #22409 * coverage: * Native V8 code coverage information can now be output to disk by setting the environment variable `NODE_V8_COVERAGE` to a directory. #22527 * deps: * The bundled npm was upgraded to version 6.4.1. #22591 * Changelogs: [6.3.0-next.0](https://github.com/npm/cli/releases/tag/v6.3.0-next.0) [6.3.0](https://github.com/npm/cli/releases/tag/v6.3.0) [6.4.0](https://github.com/npm/cli/releases/tag/v6.4.0) [6.4.1](https://github.com/npm/cli/releases/tag/v6.4.1) * fs: * The methods `fs.read`, `fs.readSync`, `fs.write`, `fs.writeSync`, `fs.writeFile` and `fs.writeFileSync` now all accept `TypedArray` and `DataView` objects. #22150 * A new boolean option, `withFileTypes`, can be passed to to `fs.readdir` and `fs.readdirSync`. If set to true, the methods return an array of directory entries. These are objects that can be used to determine the type of each entry and filter them based on that without calling `fs.stat`. #22020 * http2: * The `http2` module is no longer experimental. #22466 * os: * Added two new methods: `os.getPriority` and `os.setPriority`, allowing to manipulate the scheduling priority of processes. #22407 * process: * Added `process.allowedNodeEnvironmentFlags`. This object can be used to programmatically validate and list flags that are allowed in the `NODE_OPTIONS` environment variable. #19335 * src: * Deprecated option variables in public C++ API. #22515 * Refactored options parsing. #22392 * vm: * Added `vm.compileFunction`, a method to create new JavaScript functions from a source body, with options similar to those of the other `vm` methods. #21571 * Added new collaborators: * [lundibundi](https://github.com/lundibundi) - Denys Otrishko PR-URL: #22716
Does this also affect |
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesPartially fixing #1826
Haven't added tests yet, since I'm not sure what behaviour to expect. Current tests are not breaking, though, so yay!Tests have been added since.