test: add test for `Module._stat` by RaisinTen · Pull Request #44713 · nodejs/node

RaisinTen · 2022-09-18T12:31:45Z

Module._stat landed in #44537 without a test, so this change adds one.

Signed-off-by: Darshan Sen raisinten@gmail.com

aduh95

Maybe add mustCalls to ensure it’s indeed being called?

RaisinTen · 2022-09-18T12:42:33Z

@aduh95 aren't mustCalls for making sure that a callback passed to a function gets called? This one's totally synchronous, that's why I tested this completely based on the change in return values.

test/parallel/test-vfs.js

aduh95 · 2022-09-19T07:18:06Z

test/parallel/test-vfs.js

+fs.readFileSync = function readFileSync(pathArgument, options) {
+  if (!pathArgument.startsWith(process.execPath)) {
+    return originalReadFileSync.apply(this, arguments);
+  }
+  if (pathArgument === vfsFile) {
+    return "module.exports = { x: 'y' };";
+  }
+  throw new Error();
+};
+
+fs.realpathSync = function realpathSync(pathArgument, options) {
+  return pathArgument;
+};


Wait, that's not a use case we want to support, mutating fs should not have any effect on Node.js internals, we should fix that.

This is actually how VFSs are implemented in the ecosystem (pkg, electron, etc.) currently and fixing that would break a lot of packages and I believe the intention behind exposing Module._stat is to allow this? I don't think there is any other use case behind Module._stat or Module._readPackage. cc @arcanis

FWIW, we are also trying to find better ways of doing this without monkey-patching in nodejs/single-executable#37.

It shows that we probably also need Module._realPath and Module._readFileSync – or rather, that we need the loader hook API to stabilize. Anyway, I don't know if we want this is our tests, I think we want to break this at some point.

@aduh95 if we start exposing Module._* functions for these, we would have to do so for a lot more functions. These are the ones that Electron overrides - https://github.com/electron/electron/blob/eebf34cc6c4691e2ddca9b5a0a97566aeabd9072/lib/asar/fs-wrapper.ts#L236-L854 (quite a lot!) and there are probably additional ones in yarn's fslib implementation - https://github.com/yarnpkg/berry/tree/76ccb18b3b8cc81e28dbef5f3f867395aa31d5fb/packages/yarnpkg-fslib/sources/patchFs.

Fwiw I personally have an expectation that Node.js should abide to its own fs API (which is part of why _stat and _readPackage were so problematic, being the two places not doing so purely for optimization purposes).

It's probably never been discussed formally before though, and perhaps doing so would be a good thing (if only to get this use case formally recognized, supported, and covered by tests).

I agree that the lack of consistency is quite bad. IMHO Node.js internals should not be affected by user-land actions, however I could see that we still want to support the use case of alternative fs implementation, which could be supplied by e.g. a CLI flag and would affect the whole process, not just the few files where we forgot to use destructuring.

ljharb · 2022-09-19T15:48:59Z

cc @nodejs/modules

GeoffreyBooth · 2022-09-19T16:28:48Z

we need the loader hook API to stabilize

It’s pretty close. Once #44710 and #43772 land and we allow some time for baking, that’s all that’s on our list before declaring the API stable: https://github.com/nodejs/loaders#status

RaisinTen · 2022-10-21T14:48:06Z

To be clear, the PR that exposed Module._stat has already landed and it has been released to v18 and v16, I'm not sure why a test for such a thing wouldn't be accepted. Do y'all have suggestions on what needs to be changed in the test for it to be accepted? I don't think it's a good idea to have untested features laying around. If y'all are not happy that the feature exists, would y'all be acceptive of a PR that reverts #44537?

arcanis

Fwiw I don't have real approval rights in this repo, but as far as I can tell the test looks fine to me 🙂

(As for reverting, I strongly object - I made the original PR for a reason, it shouldn't be reverted)

GeoffreyBooth · 2022-10-21T16:45:50Z

(As for reverting, I strongly object - I made the original PR for a reason, it shouldn't be reverted)

Where's the suggestion for reverting?

Even if we eventually revert (not that I'm suggesting we do) it would be preferable to revert both the feature and its test together, I think, so we have the history.

test/parallel/test-vfs.js

RaisinTen · 2022-10-22T13:38:16Z

@GeoffreyBooth

Where's the suggestion for reverting?

I posted the comment about reverting in #44713 (comment) in case we are not comfortable with exposing Module._stat.

Even if we eventually revert (not that I'm suggesting we do) it would be preferable to revert both the feature and its test together, I think, so we have the history.

Yes but for that we would need to land the test first. If you take a look at the contents of #44537 you would see that it landed without any tests, which is why I thought of sending a PR to add some.

test/parallel/test-vfs.js

Signed-off-by: Darshan Sen <raisinten@gmail.com>

nodejs-github-bot · 2022-10-25T09:10:06Z

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

nodejs-github-bot · 2022-10-25T11:48:21Z

Landed in 37b8a60

Module._stat landed in #44537 without a test, so this change adds one. Signed-off-by: Darshan Sen <raisinten@gmail.com> PR-URL: #44713 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Sep 18, 2022

RaisinTen mentioned this pull request Sep 18, 2022

module: open stat/readPackage to mutations #44537

Merged

RaisinTen force-pushed the add-tests-for-Module._stat branch from 18df730 to dfa0e90 Compare September 18, 2022 12:38

This comment was marked as outdated.

Sign in to view

aduh95 reviewed Sep 18, 2022

View reviewed changes

This comment was marked as outdated.

Sign in to view

RaisinTen force-pushed the add-tests-for-Module._stat branch from dfa0e90 to b814716 Compare September 19, 2022 06:24

This comment was marked as outdated.

Sign in to view

aduh95 reviewed Sep 19, 2022

View reviewed changes

RaisinTen mentioned this pull request Sep 19, 2022

Research the feasibility to providing a VFS extension mechanism to fs, require(), child_process, etc nodejs/single-executable#37

Closed

arcanis approved these changes Oct 21, 2022

View reviewed changes

GeoffreyBooth reviewed Oct 21, 2022

View reviewed changes

test/parallel/test-vfs.js Show resolved Hide resolved

aduh95 reviewed Oct 22, 2022

View reviewed changes

test/parallel/test-vfs.js Show resolved Hide resolved

RaisinTen commented Oct 22, 2022

View reviewed changes

test/parallel/test-vfs.js Show resolved Hide resolved

RaisinTen commented Oct 22, 2022

View reviewed changes

test/parallel/test-vfs.js Outdated Show resolved Hide resolved

RaisinTen commented Oct 22, 2022

View reviewed changes

test/parallel/test-vfs.js Show resolved Hide resolved

RaisinTen commented Oct 22, 2022

View reviewed changes

test/parallel/test-vfs.js Outdated Show resolved Hide resolved

aduh95 approved these changes Oct 22, 2022

View reviewed changes

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 22, 2022

RaisinTen added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Oct 22, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 22, 2022

This comment was marked as outdated.

Sign in to view

RaisinTen added 5 commits October 23, 2022 10:45

test: use common.expectWarning()

b135612

Signed-off-by: Darshan Sen <raisinten@gmail.com>

fixup! test: add comment

69a23d7

fixup! test: add comment

f40bbb2

fixup! test: add comment

1550841

fixup! test: add a more isolated Module._stat test

c19ddb9

Signed-off-by: Darshan Sen <raisinten@gmail.com>

RaisinTen force-pushed the add-tests-for-Module._stat branch from 9ec74c9 to c19ddb9 Compare October 23, 2022 05:29

GeoffreyBooth approved these changes Oct 23, 2022

View reviewed changes

GeoffreyBooth added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 23, 2022

RaisinTen added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 23, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 23, 2022

This comment was marked as outdated.

Sign in to view

This was referenced Oct 24, 2022

CI Reliability 2022-10-24 nodejs/reliability#409

Open

CI Reliability 2022-10-25 nodejs/reliability#410

Open

This comment was marked as outdated.

Sign in to view

RaisinTen added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 25, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 25, 2022

nodejs-github-bot merged commit 37b8a60 into nodejs:main Oct 25, 2022

RaisinTen deleted the add-tests-for-Module._stat branch October 25, 2022 11:51

This was referenced Oct 26, 2022

CI Reliability 2022-10-26 nodejs/reliability#411

Open

CI Reliability 2022-10-27 nodejs/reliability#412

Open

CI Reliability 2022-10-28 nodejs/reliability#413

Open

RafaelGSS mentioned this pull request Nov 1, 2022

v19.1.0 proposal #45269

Merged

danielleadams mentioned this pull request Dec 30, 2022

v18.13.0 release proposal #46025

Merged

Uh oh!

Conversation

RaisinTen commented Sep 18, 2022

Uh oh!

This comment was marked as outdated.

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

RaisinTen commented Sep 18, 2022

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

aduh95 Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

RaisinTen Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

aduh95 Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

RaisinTen Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

arcanis Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

aduh95 Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

ljharb commented Sep 19, 2022

Uh oh!

GeoffreyBooth commented Sep 19, 2022

Uh oh!

RaisinTen commented Oct 21, 2022

Uh oh!

arcanis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth commented Oct 21, 2022

Uh oh!

Uh oh!

RaisinTen commented Oct 22, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented Oct 25, 2022

Uh oh!

nodejs-github-bot commented Oct 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

arcanis left a comment •

edited

Loading