fs: promises fixes - #20407
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Is it possible to add a test for this?
Sorry, something went wrong.
There was a problem hiding this comment.
@mscdex In general, fs/promises has poor coverage and this is not the only method that lacks it completely. In the current state, I view that as a separate task. I mentioned that in the PR.
This is not some edge case — the whole public method (among with several others) completely lacks tests and they need to be written.
Ref: https://coverage.nodejs.org/coverage-b55a11d1b17b3e4b/root/fs/promises.js.html
Sorry, something went wrong.
davisjam
left a comment
There was a problem hiding this comment.
Nice catch on fchmod vs fchown.
I think it's appropriate to improve things as you go along, so I think adding a TC for lchown would be good as part of this PR.
Sorry, something went wrong.
I don't think it's fair to require this here (requesting is fine though) as this PR is an improvement and a bug fix as is. Plus tests for it could be great for a code & learn. |
Sorry, something went wrong.
|
@davisjam @mscdex I opened issue #20435 for tests. If there were tests present and this fixed a corner case, I would have included a test for that corner case, but there are just no tests for those functions at all currently (and testing just the patched one won't even improve the situation much). E.g. #20435 (comment) could be a good approach, but again — out of scope here. |
Sorry, something went wrong.
|
@ChALkeR Clearly I overestimated the state of tests. I retract my objection. |
Sorry, something went wrong.
|
I don't understand why tests have to be added separately? I was under the impression that bug fixes should generally come with test(s) to ensure the bug doesn't happen again? To be clear, I wasn't asking for tests for the entire set of fs/promise APIs, just the one being affected in this PR. |
Sorry, something went wrong.
|
@mscdex @devsnek Because in the current state, that's a separate problem, meaning that it requires more work and discussion than a mere fix here. E.g. #20439 is a good way to address it. |
Sorry, something went wrong.
|
I personally think it would still be best to land it with tests. I know we work on getting something else going but until then, the extra test would help. Nevertheless, I am not going to block this without a test either. So if it is only about me: go ahead and land it. |
Sorry, something went wrong.
|
I am going to merge this as it is tomorrow if there would be no explicit concerns. I still strongly believe that lack of tests for fs/promises should be adressed separately and attemptiing to adress that issue here would just provide a defective result. |
Sorry, something went wrong.
Sorry, something went wrong.
|
stringbytes-external-exceed-max failures on osx look unrelated. |
Sorry, something went wrong.
That code expects the last argument to be a callback. When it's not a callback, it shifts arguments, defaulting encoding to 'utf-8', which is clearly broken. Old signature: (fd, string[, position[, encoding]], callback) New signature: (fd, string[, position[, encoding]]) PR-URL: nodejs#20407 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jamie Davis <davisjam@vt.edu> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
This was a clear error. chown should do chown, not chmod. PR-URL: nodejs#20407 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jamie Davis <davisjam@vt.edu> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
|
Landed in dd03709...15f7431. |
Sorry, something went wrong.
That code expects the last argument to be a callback. When it's not a callback, it shifts arguments, defaulting encoding to 'utf-8', which is clearly broken. Old signature: (fd, string[, position[, encoding]], callback) New signature: (fd, string[, position[, encoding]]) PR-URL: #20407 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jamie Davis <davisjam@vt.edu> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
This was a clear error. chown should do chown, not chmod. PR-URL: #20407 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jamie Davis <davisjam@vt.edu> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
That code expects the last argument to be a callback. When it's not a callback, it shifts arguments, defaulting encoding to 'utf-8', which is clearly broken. Old signature: (fd, string[, position[, encoding]], callback) New signature: (fd, string[, position[, encoding]]) PR-URL: #20407 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jamie Davis <davisjam@vt.edu> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
This was a clear error. chown should do chown, not chmod. PR-URL: #20407 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jamie Davis <davisjam@vt.edu> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
That code expects the last argument to be a callback. When it's not a callback, it shifts arguments, defaulting encoding to 'utf-8', which is clearly broken. Old signature: (fd, string[, position[, encoding]], callback) New signature: (fd, string[, position[, encoding]]) PR-URL: #20407 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jamie Davis <davisjam@vt.edu> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
This was a clear error. chown should do chown, not chmod. PR-URL: #20407 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jamie Davis <davisjam@vt.edu> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Checklist
This fixes two errors in fs/promises:
Sorry, this doesn't include tests — I would appreciate some help there. In general, fs/promises test coverage is poor, and this PR isn't aimed at that (yet). Also, #19811 aims to increase the test covereage for fs/promises.
Perhaps #20406 need to be resolved first — the first patch fixes technically undocumented behavior and I am not sure we should be testing it or removing it.