Skip to content
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

test: add test for Module._stat #44713

Merged
merged 6 commits into from
Oct 25, 2022

Conversation

RaisinTen
Copy link
Contributor

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

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

@nodejs-github-bot 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
@nodejs-github-bot

This comment was marked as outdated.

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@RaisinTen
Copy link
Contributor Author

@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.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

test/parallel/test-vfs.js Outdated Show resolved Hide resolved
Comment on lines +60 to +75
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;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

ljharb commented Sep 19, 2022

cc @nodejs/modules

@GeoffreyBooth
Copy link
Member

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
Copy link
Contributor Author

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?

Copy link
Contributor

@arcanis arcanis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

(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.

@RaisinTen
Copy link
Contributor Author

@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 Outdated Show resolved Hide resolved
test/parallel/test-vfs.js Outdated Show resolved Hide resolved
test/parallel/test-vfs.js Outdated Show resolved Hide resolved
@aduh95 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 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 github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 22, 2022
@nodejs-github-bot

This comment was marked as outdated.

@GeoffreyBooth 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 RaisinTen added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 23, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 23, 2022
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@RaisinTen RaisinTen added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 25, 2022
@nodejs-github-bot 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 nodejs-github-bot merged commit 37b8a60 into nodejs:main Oct 25, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 37b8a60

@RaisinTen RaisinTen deleted the add-tests-for-Module._stat branch October 25, 2022 11:51
RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
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>
@RafaelGSS RafaelGSS mentioned this pull request Nov 1, 2022
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
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>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
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>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
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>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants