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

chore: replace fast-glob with tinyglobby #6274

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

SuperchupuDev
Copy link
Contributor

@SuperchupuDev SuperchupuDev commented Aug 4, 2024

Description

tinyglobby is meant to be a drop-in replacement to globby and fast-glob, and it only uses two subdependencies - fdir and picomatch. as discussed in the discord server, this PR switches to it

expandDirectories is explicitly disabled for better fast-glob compatibility

funny enough the vitest monorepo already uses it in some way, due to unocss and vite-plugin-pwa using it

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

  • Run the tests with pnpm test:ci.

Documentation

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

Copy link

netlify bot commented Aug 4, 2024

Deploy Preview for vitest-dev ready!

Name Link
🔨 Latest commit ab3aded
🔍 Latest deploy log https://app.netlify.com/sites/vitest-dev/deploys/66daf80c3975a8000868d95b
😎 Deploy Preview https://deploy-preview-6274--vitest-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@SuperchupuDev
Copy link
Contributor Author

the tests seem to fail some snapshots, but only because they have a different order. should i update the snapshots or do a manual .sort?

@sheremet-va
Copy link
Member

the tests seem to fail some snapshots, but only because they have a different order. should i update the snapshots or do a manual .sort?

Why is the order different?

@SuperchupuDev
Copy link
Contributor Author

SuperchupuDev commented Aug 4, 2024

unsure why until i debug a bit more, but it looks like it's alphabetically sorted now, maybe it's the opposite in fast-glob? ideally vitest should report errors in the same order regardless of the order the glob results came in. from earlier tests i did weeks ago tinyglobby's globs aren't guaranteed to be sorted. iirc globs in general follow a non-deterministic order even if fast-glob in particular doesn't

@sheremet-va
Copy link
Member

unsure why until i debug a bit more, but it looks like it's alphabetically sorted now, maybe it's the opposite in fast-glob? ideally vitest should report errors in the same order regardless of the order the glob results came in. from earlier tests i did weeks ago tinyglobby's globs aren't guaranteed to be sorted. iirc globs in general follow a non-deterministic order even if fast-glob in particular doesn't

Vitest has a built in sequencer that does sorting: https://vitest.dev/config/#sequence-sequencer

@SuperchupuDev
Copy link
Contributor Author

SuperchupuDev commented Aug 4, 2024

i'm having a bit of trouble debugging due to vitest running tests using itself

@sheremet-va
Copy link
Member

Hm, looks like we sort by names only when sharding. Normally, we use fs.stat and fallback to the existing sorting

@SuperchupuDev
Copy link
Contributor Author

SuperchupuDev commented Aug 5, 2024

okay, so after debugging i can confirm that on that particular test case, fast-glob returns the two results reverse sorted, while tinyglobby does sort them correctly. should i just change the snapshots?

@SuperchupuDev SuperchupuDev force-pushed the chore/tinyglobby branch 2 times, most recently from ce517aa to cfe4efc Compare August 5, 2024 18:50
@SuperchupuDev
Copy link
Contributor Author

pushed the snapshot sorting update, i don't see why it should keep the unsorted order it had, and if anything that can always be changed in a followup pr

@sheremet-va
Copy link
Member

sheremet-va commented Aug 5, 2024

okay, so after debugging i can confirm that on that particular test case, fast-glob returns the two results reverse sorted, while tinyglobby does sort them correctly. should i just change the snapshots?

What determines “correctly”? Why the previous sorting is incorrect and what are the consequences of that change? Is this flaky? Will test order be non-deterministic with tinyglobby?

@SuperchupuDev
Copy link
Contributor Author

SuperchupuDev commented Aug 5, 2024

from my testing, the array of two elements produced from globbing in that test is not alphabetically sorted, and it is in tinyglobby, every single time i've ran these tests. by correct i meant it being sorted at all. despite that, i don't have evidence that globbing is deterministic with tinyglobby. the proper fix would be to just let vitest sort the reports itself

@sheremet-va
Copy link
Member

by correct i meant it being sorted at all.

You said it returns them in reversed order, so that's sorted, no? Just in reverse.

i have no idea why the tests are failing now, the only thing changed after force pushing was that snapshot in particular.

Tests bail out early, so if there are more errors, you won't see them unless all tests have run

@sheremet-va
Copy link
Member

Sorry for being picky here. I just remember that the sorting is important for filtering for example and while I appreciate the smaller size and dependencies count (and I agree that we should keep making it smaller), we should keep it as compatible as possible.

@SuperchupuDev
Copy link
Contributor Author

that array of two elements in particular seems to be reverse sorted, not because fast-glob returns globs reverse sorted but because an array of two can only ever be either sorted or reverse sorted :P weird wording on my part

@SuperchupuDev
Copy link
Contributor Author

SuperchupuDev commented Aug 7, 2024

it seems like the config in that test in particular breaks tinyglobby, and it'd be complex and hacky enough to add. thankfully it's the only remaining failing test

https://github.com/vitest-dev/vitest/blob/eae3bf4a72c1ab0b0b8ea0e13a3ed8a415ebfc66/test/reporters/reportTest2/custom-reporter-path.vitest.config.ts

it looks like a very hacky config? if the root is __dirname, it makes no sense to include tests outside that root. do users actually do that instead of defining a broader root (like path.join(__dirname, '..'))?

@sheremet-va
Copy link
Member

sheremet-va commented Aug 12, 2024

it seems like the config in that test in particular breaks tinyglobby, and it'd be complex and hacky enough to add. thankfully it's the only remaining failing test

eae3bf4/test/reporters/reportTest2/custom-reporter-path.vitest.config.ts

it looks like a very hacky config? if the root is __dirname, it makes no sense to include tests outside that root. do users actually do that instead of defining a broader root (like path.join(__dirname, '..'))?

It does seem weird, but if it worked before then someone might've used this in the wild. But I haven't seen any usage of this pattern to be honest. We'll discuss with the team how we can proceed here in the next meeting. Thank you for the PR!

@sheremet-va sheremet-va added the p2-nice-to-have Not breaking anything but nice to have (priority) label Aug 12, 2024
@SuperchupuDev
Copy link
Contributor Author

SuperchupuDev commented Aug 12, 2024

after thinking a lot about the problem, it is something important to have regardless of its use in vitest. i have a not that hacky fix almost ready, but it makes using patterns with leading ../ slower in some cases (cases that can be made a lot faster using the ignore option). ill publish a new version with the fix very soon

@SuperchupuDev
Copy link
Contributor Author

opened a fix SuperchupuDev/tinyglobby#18, will wait until you have the meeting you mentioned though

@sheremet-va
Copy link
Member

The team thinks it's fine to drop support for ../

@SuperchupuDev
Copy link
Contributor Author

SuperchupuDev commented Aug 15, 2024

after refactoring out the ../ in that one test tests seem to pass now, except one in macos that's failing in other prs too

@SuperchupuDev
Copy link
Contributor Author

ended up adding ../ support in 0.2.3 anyways, some use cases from an unrelated package were breaking without it

@SuperchupuDev SuperchupuDev force-pushed the chore/tinyglobby branch 3 times, most recently from 0467c52 to 43c003d Compare August 24, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants