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

Use real filesystem for unit testing #12172

Merged
merged 13 commits into from
Oct 15, 2024
Merged

Use real filesystem for unit testing #12172

merged 13 commits into from
Oct 15, 2024

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Oct 9, 2024

Changes

Use the real filesystem for unit testing instead of mocking fs with memfs. This unblocks #12165. Benefits:

  1. We can view the actual files that are being run.
  2. We rely on the real filesystem, ensuring path handling or any OS specific behaviours are tested.
  3. We no longer have to change our implementation to allows passing in a fs param.

The PR uses fs-fixture (with a patch that I'll comment below) that allows us to create files and directories as fixtures to test on. It's merging to main instead of next so that if anyone adds a new unit test in the future in main, they can use this pattern. Otherwise we have to manually migrate whenever merging main to next.

This means after the PR is merged (I hope!), we can start removing our fs abstraction that was introduced only for tests, which should simply things more.

Testing

Existing test should pass, especially unit tests

Docs

n/a

Copy link

changeset-bot bot commented Oct 9, 2024

⚠️ No Changeset found

Latest commit: f159c8a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Oct 9, 2024
@bluwy
Copy link
Member Author

bluwy commented Oct 9, 2024

I need to fix the test fail, but thanks for the very quick review 😅

@github-actions github-actions bot added the 🚨 action Modifies GitHub Actions label Oct 10, 2024
@github-actions github-actions bot removed the 🚨 action Modifies GitHub Actions label Oct 14, 2024
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Happy to merge!

@bluwy
Copy link
Member Author

bluwy commented Oct 15, 2024

Thanks for the reviews. I had trouble making restart.test.js working as there's issues with chokidar detecting changes. I resorted to the mock event emitting trick like before to make it happy for now, but it's something I want to take a better look in the future. I'll merge this for now so we can unblock the other PRs.

@bluwy bluwy merged commit 64bb796 into main Oct 15, 2024
14 checks passed
@bluwy bluwy deleted the real-fs branch October 15, 2024 07:34
@bluwy bluwy mentioned this pull request Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants