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

switch from jest to vitest #5022

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open

Conversation

with-heart
Copy link
Contributor

This PR switches our test setup from jest to vitest.

How it's configured

vitest.workspace.json defines our workspaces as ["packages/*"]. vitest looks for a vitest.config file in each workspace and uses it to run that workspace's tests. This allows each workspace to define the plugins (like vite-plugin-solid for @xstate/solid) and test configuration used to run that workspace's tests.

This allows us to run tests for specific workspaces using vitest --project <package name> (vitest --project @xstate/react).

Major changes

  • removed all jest dependencies (11❗), configs, scripts, etc.
  • replaced jsdom with happy-dom
  • replaced deprecated done callbacks with new Promise callbacks + resolve
  • added vite plugins to some packages for out-of-the-box support in tests
  • removed @xstate-repo/jest-utils package
    • replaced toMatchMockCallsInlineSnapshot with toMatchInlineSnapshot (vitest doesn't support wrapping snapshot functions the way we could with jest-snapshot)
    • replaced spyOnConsole with inline console spies for the tests that require it
    • replaced sleep with setTimeout from node:timers/promises
  • removed redundant patches

Copy link

changeset-bot bot commented Aug 7, 2024

⚠️ No Changeset found

Latest commit: 8cf9741

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

Copy link
Member

@Andarist Andarist left a comment

Choose a reason for hiding this comment

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

I'd like to block this on updating the VS Code launch.json script. It's essential to my workflow.

Extra questions:

  1. Does Vitest have an interactive watch mode in which I could filter tests using filenames or/and test names?
  2. If yes, does it have something akin to jest-watch-typehead?

@davidkpiano
Copy link
Member

I'd like to block this on updating the VS Code launch.json script. It's essential to my workflow.

Extra questions:

  1. Does Vitest have an interactive watch mode in which I could filter tests using filenames or/and test names?
  2. If yes, does it have something akin to jest-watch-typehead?

Yes; here is an example in @statelyai/agent:

CleanShot 2024-08-07 at 07 39 00

@with-heart
Copy link
Contributor Author

I'd like to block this on updating the VS Code launch.json script. It's essential to my workflow.

I've got you buddy 🤗

@with-heart
Copy link
Contributor Author

I'd like to block this on updating the VS Code launch.json script. It's essential to my workflow.

@Andarist Wanna test it locally and make sure it's working the way you expect?

@Andarist
Copy link
Member

Andarist commented Aug 8, 2024

I noticed it's fairly easy to lose those typeahead filters in watch mode (for example pressing Enter clears them). It's annoying but not a big deal.

When debugging with VS Code, I can't disable source mapped stepping - it still shows me a location in the source file (albeit a different/broken one). This is way more annoying, probably something I can live with but well. I just wish that console evaluation could use original references, is this too much to ask for? 😭 😭 😭 (to clarify, this is just something that current debuggers suck at cause they are not utilizing sourcemapped info to its full extent)

@with-heart
Copy link
Contributor Author

I noticed it's fairly easy to lose those typeahead filters in watch mode (for example pressing Enter clears them). It's annoying but not a big deal.

Yeah for some reason they chose to use r for re-running filtered tests. Quite an annoying change if you're a long-time jest user 🥲

@with-heart with-heart requested a review from Andarist August 9, 2024 15:31
@with-heart with-heart mentioned this pull request Aug 16, 2024
after: {
10: '#end'
it('should be able to transition with delay from nested initial state', () =>
new Promise<void>((resolve) => {
Copy link
Member

Choose a reason for hiding this comment

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

Those kind of changes could probably be replaced with async functions and waitFor. Some might not be but then I'd go with Promise.withResolvers (which might not be available in our testing envs, I'm not sure). This would allow u to create a deferred thing at the top of the async function, await it at the bottom before ur expect calls and .resolve/.reject it from callbacks. This would avoid adding extra indentation to the tests that got refactored by introducing new Promise

Copy link
Contributor Author

@with-heart with-heart Aug 16, 2024

Choose a reason for hiding this comment

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

imo the new Promise callback style works well for assertion-free tests (where the test is basically just that it doesn't time out (because resolve was called)). Plus it's the closest to the old done callback style (minus the extra indentation).

I think this example shows the pitfalls of waitFor/Promise.withResolvers with this style of test:

test('new Promise', () =>
  new Promise((resolve) => {
    const machine = createMachine({
      initial: 'waiting',
      states: {
        waiting: {
          after: {
            10: 'done'
          }
        },
        done: {
          entry: resolve
        }
      }
    });
    createActor(machine).start();

    // no assertions or other logic necessary. the test only passes if the
    // promise resolves; otherwise, it times out
  }));

test('waitFor', async () => {
  const machine = createMachine({
    initial: 'waiting',
    states: {
      waiting: {
        after: {
          10: 'done'
        }
      },
      done: {}
    }
  });
  const actor = createActor(machine);
  actor.start();

  // we need to wait for + assert the transition. if we forget to `await` or
  // forget `waitFor` entirely, the test passes even though we haven't actually
  // tested anything
  await waitFor(actor, (snapshot) => snapshot.matches('done'));
});

test('Promise.withResolvers', () => {
  const { promise, resolve } = Promise.withResolvers();

  const machine = createMachine({
    initial: 'waiting',
    states: {
      waiting: {
        after: {
          10: 'done'
        }
      },
      done: {
        entry: resolve
      }
    }
  });
  createActor(machine).start();

  // if we don't `await promise` or `return promise`, the test passes even
  // though `resolve` isn't called. would be an easy mistake to make and the
  // cost is that the test isn't actually testing anything even though it's
  // passing
  return promise;
});

Maybe not a big deal since it's something that could be caught in code review, but is it worth it just to avoid extra indentation?

Though actually now that I'm thinking about it, maybe we shouldn't have assertion-free tests to begin with 🙃

Definitely agree with waitFor/Promise.withResolvers for tests where we're actually making assertions and such though. I'll convert those over.

Copy link
Member

@Andarist Andarist Aug 17, 2024

Choose a reason for hiding this comment

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

Though actually now that I'm thinking about it, maybe we shouldn't have assertion-free tests to begin with 🙃

This. I think it's always best to assert something explicitly. That always gives some extra insight to what is actually being tested

@boneskull
Copy link
Contributor

Just wanted to plug Wallaby for running tests in "watch" mode (or even as you type). I've been a user for years and I haven't found anything better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants