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

esm: delete preload mock test #46402

Merged

Conversation

GeoffreyBooth
Copy link
Member

@GeoffreyBooth GeoffreyBooth commented Jan 29, 2023

Per #44710 (comment), this PR deletes a test that isn’t compatible with off-thread loaders; but more broadly, I think we’re likely to want to rethink the approach of globalPreload‘s sloppy scripts, rather than build on it. I’m not reverting the full PR that added this test, #39240, because we’re keeping parts of it. The off-thread PR will delete the undocumented setImportMetaCallback that that PR adds. More context: nodejs/loaders#124

cc @bmeck @nodejs/loaders

@GeoffreyBooth GeoffreyBooth added esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders labels Jan 29, 2023
@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 Jan 29, 2023
@bmeck
Copy link
Member

bmeck commented Jan 29, 2023 via email

@GeoffreyBooth
Copy link
Member Author

GeoffreyBooth commented Jan 29, 2023

This shouldn’t be deleted without a plan forward.

This was discussed in nodejs/loaders#124 and #46220. Basically, test mocking can be achieved with just the port option in globalPreload, as that’s what Quibble is doing; and the test in #46220 tests port. So the use case of mocking for tests is handled with existing tests.

This test has a grander vision of what mocking for tests should look like, including mocking import.meta objects, but we can’t get it to work with the off-thread PR. We removed setImportMetaCallback a month ago from the off-thread PR. Since it was undocumented, my feeling was that it could always be re-added after the off-thread PR landed. But more broadly, I’m leaning against using the sloppy mode scripts of globalPreload as a way to inject what are essentially more hooks. I think we can remove the sloppy mode scripts entirely, now that we have --import; no one in nodejs/loaders#124 disputed that. To do so, we would need to find a different way to provide access to port, and to an equivalent to setImportMetaCallback if mocking import.meta objects is something we want to provide an API for.

@bmeck
Copy link
Member

bmeck commented Jan 29, 2023 via email

@GeoffreyBooth
Copy link
Member Author

GeoffreyBooth commented Jan 29, 2023

I can port the test to use a leaky global as well then.

Can you do that as a commit on the branch for #44710? If you can refactor the test so that it passes on the off-thread branch, then sure, it can stay. At this point my top priority is landing that PR, as it’s holding up just about the entire Loaders API agenda, and I’m okay with removing things to make way for that PR landing (and those removed things can get added back later).

If you don’t have time soon to refactor the test on that branch, we can just land this removal PR and then a refactored version of the test can get added back in a later PR.

@bmeck
Copy link
Member

bmeck commented Jan 29, 2023 via email

@GeoffreyBooth
Copy link
Member Author

As such removal of this is unfortunate since there isn’t another mocking ref to ensure the feature in core is possible but acceptable.

Sure, it would be nice if there was a test in core to validate that the mocking use case is possible; but I think such a test could be more minimal than this one, which goes beyond what I would consider the minimum acceptable mocking (such as setting up a fake import.meta for the mocked module). @giltayar in nodejs/loaders#124 (comment) seems very confident that as long as globalPreload port remains after the move to off-thread, Quibble will continue to be able to work. So I think we should be good, and having the test in core is a nice-to-have that can happen anytime. I asked him to contribute such a test when he found the time and he seemed willing.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@GeoffreyBooth GeoffreyBooth added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Jan 30, 2023
@aduh95
Copy link
Contributor

aduh95 commented Jan 30, 2023

At this point my top priority is landing that PR, as it’s holding up just about the entire Loaders API agenda, and I’m okay with removing things to make way for that PR landing (and those removed things can get added back later).

Another possibility would be to move that test to test/known_issues in the off-thread branch, that would let the CI be green while keeping the test in the repo.

@GeoffreyBooth
Copy link
Member Author

GeoffreyBooth commented Jan 30, 2023

Another possibility would be to move that test to test/known_issues in the off-thread branch, that would let the CI be green while keeping the test in the repo.

It’s not really a known issue in that we’re intentionally removing setImportMetaCallback. The test would be written for an API that no longer exists.

I suppose we could move it into known_issues and explain all this in a comment at the top of the test file, but is that really better? If we’re looking to create a TODO, we could open an issue on GitHub that we want a test for the mocking use case. That feels more likely to get noticed. Such an issue could link to this PR, to provide the old test as reference.

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 31, 2023
@nodejs-github-bot nodejs-github-bot merged commit 938341a into nodejs:main Jan 31, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 938341a

@GeoffreyBooth GeoffreyBooth deleted the remove-preload-mock-test branch January 31, 2023 04:50
@GeoffreyBooth
Copy link
Member Author

@aduh95 I forgot to remove author ready/commit queue after your comment; should I have done so? Not sure if that was an outstanding question that wasn’t resolved.

@aduh95
Copy link
Contributor

aduh95 commented Jan 31, 2023

is that really better?

I might be wrong, but I think removing the file and adding it back later "reset" the git blame history, while renaming/moving the file would not. In that sense it would be better. Also if someone wants to work on the test, it's much easier if the code is still in the codebase rather than having to dig the PRs or the git history.

If the off-thread loader is not capable of mocking, that would be an issue, in that sense it's a known issue. setImportMetaCallback is an implementation detail.

I forgot to remove author ready/commit queue after your comment; should I have done so? Not sure if that was an outstanding question that wasn’t resolved.

I did not remove it myself, so no worries. I kinda regret not doing so, as now this test has been removed but the off-thread PR still has failing tests and still cannot land, so it could have waited longer, but anyway.

ruyadorno pushed a commit that referenced this pull request Feb 1, 2023
PR-URL: #46402
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Feb 1, 2023
@aduh95 aduh95 mentioned this pull request Feb 12, 2023
7 tasks
juanarbol pushed a commit that referenced this pull request Mar 3, 2023
PR-URL: #46402
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@juanarbol juanarbol mentioned this pull request Mar 3, 2023
juanarbol pushed a commit that referenced this pull request Mar 3, 2023
PR-URL: #46402
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
juanarbol pushed a commit that referenced this pull request Mar 5, 2023
PR-URL: #46402
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@aduh95 aduh95 mentioned this pull request May 30, 2023
aduh95 added a commit that referenced this pull request Jul 17, 2023
PR-URL: #48779
Fixes: #48778
Fixes: #48516
Refs: #46402
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
pluris pushed a commit to pluris/node that referenced this pull request Aug 6, 2023
pluris pushed a commit to pluris/node that referenced this pull request Aug 7, 2023
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
PR-URL: #48779
Fixes: #48778
Fixes: #48516
Refs: #46402
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Nov 11, 2023
targos pushed a commit that referenced this pull request Nov 23, 2023
PR-URL: #48779
Backport-PR-URL: #50669
Fixes: #48778
Fixes: #48516
Refs: #46402
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
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. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants