microsoft / FluidFramework

Library for building distributed, real-time collaborative web applications
https://fluidframework.com
MIT License
4.72k stars 532 forks source link

test(build-tools): Upgrade oclif/test and replace mocha with vitest #21920

Open tylerbutler opened 3 months ago

tylerbutler commented 3 months ago

Update September 3, 2024: While I opened this originally just to get feedback, I have found myself wishing for it to be merged when working on other changes, because the current infra just isn't great. I don't know if vitest is the best for the whole repo, but it seems like a good choice for build-tools. At the very least, this lets us upgrade the oclif/test version which is sorely needed.


Updates the build-tools projects to use vitest as a test runner instead of mocha.

What motivated this originally is that oclif/test has changed quite a bit in the latest major, so all the command tests need to be updated anyway. I did try to do this using mocha, but it wasn't straightforward. I also dislike how you need chai for some things, chai-arrays for others, etc. One advantage of vitest is that it's sort of "batteries included".

Major differences/known gaps:

  1. Tests no longer need to output anything and are always transpiled. This may not be good for the repo at large, but build-tools has generally always transpiled tests even with mocha, so the change isn't making anything worse.
  2. There are some speed improvements over mocha. Haven't quantified them fully yet, and they could also be from the oclif/test upgrade.
  3. Checking array contents is not built-in as far as I can tell, or if it is I can't figure out how to use it. So while there are more batteries included, we may still need a few more. An example from the current tests, using chai-arrays:

    https://github.com/microsoft/FluidFramework/blob/2b0199dae3d73cc7d4fab0f4848614b42e212220/build-tools/packages/build-cli/test/filter.test.ts#L153-L158

    Vitest has expect.tocontain and expect.arrayContaining but they're not 100% equivalent to chai-arrays AFAICT. And in fact I think some of the current tests may be wrong because they don't check that there aren't other items in the resulting array. Still, it's easy enough to create small util functions that do some of this stuff when needed.

changeset-bot[bot] commented 2 months ago

⚠️ No Changeset found

Latest commit: c5462c8beb4ddff6bdccb7c37d429dea8fe5b63b

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.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

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

msfluid-bot commented 2 months ago
@fluid-example/bundle-size-tests: +245 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 457.3 KB 457.33 KB +35 Bytes
azureClient.js 554.61 KB 554.66 KB +49 Bytes
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 258.11 KB 258.12 KB +14 Bytes
fluidFramework.js 406.45 KB 406.47 KB +14 Bytes
loader.js 134.1 KB 134.11 KB +14 Bytes
map.js 42.13 KB 42.14 KB +7 Bytes
matrix.js 146.3 KB 146.31 KB +7 Bytes
odspClient.js 522.76 KB 522.8 KB +49 Bytes
odspDriver.js 97.55 KB 97.57 KB +21 Bytes
odspPrefetchSnapshot.js 42.61 KB 42.62 KB +14 Bytes
sharedString.js 163 KB 163.01 KB +7 Bytes
sharedTree.js 396.97 KB 396.97 KB +7 Bytes
Total Size 3.3 MB 3.3 MB +245 Bytes

Baseline commit: cbba69554fc5026f562f44683a902474fabd6e81

Generated by :no_entry_sign: dangerJS against 99bab25d41ba1e8a879b35515bb87b02e6986a8f

Josmithr commented 2 months ago

Tests no longer need to output anything and are always transpiled - is this a good thing?

I like it, personally. I still think test code should be built in our standard build step for verification. But it's nice to not have to re-run a build after making a quick test change just to run the test(s).

Checking array contents is not built-in as far as I can tell, or if it is I can't figure out how to use it. So while there are more batteries included, we may still need a few more.

Can you elaborate on this? Do you mean checking deep equality of arrays?

tylerbutler commented 2 months ago

Checking array contents is not built-in as far as I can tell, or if it is I can't figure out how to use it. So while there are more batteries included, we may still need a few more.

Can you elaborate on this? Do you mean checking deep equality of arrays?

That and checking array membership without always caring about order. An example from the current tests, using chai-arrays:

https://github.com/microsoft/FluidFramework/blob/2b0199dae3d73cc7d4fab0f4848614b42e212220/build-tools/packages/build-cli/test/filter.test.ts#L153-L158

Vitest has expect.tocontain and expect.arrayContaining but they're not 100% equivalent to chai-arrays AFAICT. And in fact I think some of the current tests may be wrong because they don't check that there aren't other items in the resulting array. Still, it's easy enough to create small util functions that do some of this stuff.

tylerbutler commented 2 months ago

But it's nice to not have to re-run a build after making a quick test change just to run the test(s).

vitest's default mode is a watch mode that reruns tests as code changes. It's pretty slick. And vitest is fast enough that it works pretty well.

jason-ha commented 2 months ago

But it's nice to not have to re-run a build after making a quick test change just to run the test(s).

vitest's default mode is a watch mode that reruns tests as code changes. It's pretty slick. And vitest is fast enough that it works pretty well.

jest has that well-supported too. I would think someone has built the support for mocha tests. My personal history is that some integration is nice, but if that integration is not running our normal build on our behalf, then I expect trouble. It is okay to have an approximation VS Code integrated test run, but actual test runs must use javascript from regular build. Before a suite change for main products the suite will need to handle testing both ESM and CJS using our normal build output.

Josmithr commented 2 months ago

That and checking array membership without always caring about order.

Ah, I see. In theory, dumping the arrays into Sets, and running deep equality checks on those would be more semantically correct. But most tools don't diff sets well (chai, for example, just outputs that the sets are different, without giving any item-by-item details). I'd be curious to see how vitest does with them.

But just doing the same with arrays (and not using Sets) is simpler / valuable.