testing-library / svelte-testing-library

:chipmunk: Simple and complete Svelte DOM testing utilities that encourage good testing practices
https://testing-library.com/docs/svelte-testing-library/intro
MIT License
620 stars 33 forks source link

Svelte 5 feedback and support #284

Open stefanhoelzl opened 11 months ago

stefanhoelzl commented 11 months ago

Hello, @mcous has hijacked this post! We've launched ~experimental~ Svelte 5 support in @testing-library/svelte. ~See the Svelte 5 section in the README for setup instructions.~ No special setup is required to use Svelte 5 if you are using Vitest.

If you find bugs or have other Svelte 5 testing feedback, please add it to this thread! Svelte 5 is still changing rapidly at the time of writing, so feedback is appreciated!

Original post below


Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch @testing-library/svelte@4.0.5 for the project I'm working on to use svelte 5.

This patches some breaking changes but there might be more work to do for a proper PR (are tests still working? support svelte 4 and 5, maybe more)

But for everyone who likes to try out svelte 5 this patch should be a good starting point.

Here is the diff that solved my problem:

diff --git a/node_modules/@testing-library/svelte/src/pure.js b/node_modules/@testing-library/svelte/src/pure.js
index 04d3cb0..2f041e0 100644
--- a/node_modules/@testing-library/svelte/src/pure.js
+++ b/node_modules/@testing-library/svelte/src/pure.js
@@ -3,7 +3,7 @@ import {
   getQueriesForElement,
   prettyDOM
 } from '@testing-library/dom'
-import { tick } from 'svelte'
+import { tick, createRoot } from 'svelte'

 const containerCache = new Set()
 const componentCache = new Set()
@@ -54,18 +54,15 @@ const render = (
     return { props: options }
   }

-  let component = new ComponentConstructor({
+  let component = createRoot(ComponentConstructor, {
     target,
-    ...checkProps(options)
+    ...checkProps(options),
+    ondestroy: () => componentCache.delete(component)
   })

   containerCache.add({ container, target, component })
   componentCache.add(component)

-  component.$$.on_destroy.push(() => {
-    componentCache.delete(component)
-  })
-
   return {
     container,
     component,
@@ -73,18 +70,14 @@ const render = (
     rerender: (options) => {
       if (componentCache.has(component)) component.$destroy()

-      // eslint-disable-next-line no-new
-      component = new ComponentConstructor({
+      component = createRoot(ComponentConstructor, {
         target,
-        ...checkProps(options)
+        ...checkProps(options),
+        ondestroy: () => componentCache.delete(component)
       })

       containerCache.add({ container, target, component })
       componentCache.add(component)
-
-      component.$$.on_destroy.push(() => {
-        componentCache.delete(component)
-      })
     },
     unmount: () => {
       if (componentCache.has(component)) component.$destroy()

This issue body was partially generated by patch-package.

bradyisom commented 1 month ago

It says at the top of this issue to post problems or issues with Svelte 5 testing and feedback. So, here goes:

I am trying to write a test for my Svelte 5 component, but am struggling to figure out how to test a $bindable() prop on my component.

Here is a simple component:

<script lang="ts">
  import type { Snippet } from 'svelte';
  let {
    children,
    checked = $bindable(),
    ...restProps
  }: { children: Snippet; checked?: boolean; disabled?: boolean } = $props();
</script>

<label
  data-testid="checkbox"
  class="flex h-6 items-center text-sm leading-6 text-900"
>
  <input
    data-testid="checkbox-input"
    {...restProps}
    type="checkbox"
    class="h-4 w-4 rounded border border-300 checked:bg-secondary-600 checked:text-on-secondary-600 focus:ring-secondary-600 focus:outline-none focus:ring"
    bind:checked
  />
  <div data-testid="checkbox-label" class="ml-3">
    {@render children()}
  </div>
</label>

Here is a test that tries to test the bindable nature of the checked prop:

import { render, screen } from '@testing-library/svelte';
import { describe, expect, it } from 'vitest';
import userEvent from '@testing-library/user-event';
import { flush } from '$root/test-helpers';

import Checkbox from './Checkbox.svelte';
import { createRawSnippet } from 'svelte';

function getTestSnippet() {
  return createRawSnippet(() => {
    return {
      render: () => `My checkbox`,
    };
  });
}

describe('Checkbox', () => {
  it('should bind to checked', async () => {
    const user = userEvent.setup();
    const checked = $state(false);

    render(Checkbox, {
      props: {
        checked,
        children: getTestSnippet(),
      },
    });
    await flush();

    const input = screen.getByTestId('checkbox-input');
    await user.click(input);
    await flush();

    expect(input).toBeChecked();
    expect(checked).toBe(true);
  });
});

I am trying to simulate the bind:checked syntax in a parent component. I've tried setting the entire props to a $state(). I've also debugged down into the native Svelte mount() code, and it looks like it is expecting an object with a set() function on it to implement binding.

Any help or direction, or verification that there is a bug or issue here, would be appreciated.

mcous commented 1 month ago

@bradyisom in Svelte 4, binds were not directly testable, and as far as I know, this hasn't changed in Svelte 5. You will likely need a wrapper component - see the Svelte 4 example for testing bind: for inspiration. See earlier comments in this thread for reference: https://github.com/testing-library/svelte-testing-library/issues/284#issuecomment-2214079609, https://github.com/testing-library/svelte-testing-library/issues/284#issuecomment-2263489913

Opinionated advice that you didn't ask for: don't use bind:. It trades short term convenience for long-term scalability issues and bugginess. Use a callback prop to signal changes instead, which will be easier to write tests for and way less susceptible to weird race conditions as your application grows and evolves

bradyisom commented 1 month ago

@mcous, Thank you for your quick response. I did look at that Svelte 4 example several times, but somehow missed that it was creating a testing/wrapper component. I also got thrown off, because it was using a writable() store instead of a $state().

I will take your unsolicited advice under consideration.

bradyisom commented 1 month ago

I did refactor my code to use events, instead of binding values. However, I ran into a similar problem when working through dynamically mounting components. I was curious if there was a real solution, if you still want to use $bindable props.

I did a deeper dive into understanding universal reactivity and state. I found that I could actually "bind" the props in my test and test $bindable props by doing something like this:

let checked = $state(false);
render(Checkbox, {
      props: {
        get checked() {
          return checked;
        },
        set checked(value: boolean) {
          checked = value;
        },
        children: getTestSnippet(),
      },
    });

The key is adding the get and set properties on the props object. This explicitly makes the prop bindable. FWIW, in case someone else runs into this issue. It seems like, at the very least, we could add something to the docs to show this pattern for testing $bindable props.

KieranP commented 1 month ago

Just started looking at upgrading our app to Svelte5 now that it is released, and getting these errors in vitest using testing-library. App seems to run fine in the browser, so this has something to do with simulated dom/browser testing?

ReferenceError: ResizeObserver is not defined
 ❯ ResizeObserverSingleton.#getObserver node_modules/svelte/src/internal/client/dom/elements/bindings/size.js:54:26
     52|    (this.#observer = new ResizeObserver(
     53|     /** @param {any} entries */ (entries) => {
     54|      for (var entry of entries) {
       |                          ^
     55|       ResizeObserverSingleton.entries.set(entry.target, entry);
     56|       for (var listener of this.#listeners.get(entry.target) || []) {
 ❯ ResizeObserverSingleton.observe node_modules/svelte/src/internal/client/dom/elements/bindings/size.js:38:20
 ❯ Module.bind_element_size node_modules/svelte/src/internal/client/dom/elements/bindings/size.js:104:41
TypeError: element.animate is not a function
 ❯ animate node_modules/svelte/src/internal/client/dom/elements/transitions.js:377:26
    375|   // for bidirectional transitions, we start from the current position,
    376|   // rather than doing a full intro/outro
    377|   var t1 = counterpart?.t() ?? 1 - t2;
       |                          ^
    378|   counterpart?.abort();
    379|
 ❯ Object.in node_modules/svelte/src/internal/client/dom/elements/transitions.js:243:12
 ❯ node_modules/svelte/src/internal/client/dom/elements/transitions.js:298:54
 ❯ Module.untrack node_modules/svelte/src/internal/client/runtime.js:864:10
 ❯ node_modules/svelte/src/internal/client/dom/elements/transitions.js:298:27
 ❯ update_reaction node_modules/svelte/src/internal/client/runtime.js:327:56
 ❯ update_effect node_modules/svelte/src/internal/client/runtime.js:455:18
 ❯ flush_queued_effects node_modules/svelte/src/internal/client/runtime.js:545:4
 ❯ flush_queued_root_effects node_modules/svelte/src/internal/client/runtime.js:526:4
 ❯ process_deferred node_modules/svelte/src/internal/client/runtime.js:572:2
Svelte error: effect_update_depth_exceeded
Maximum update depth exceeded. This can happen when a reactive block or effect repeatedly sets a new value. Svelte limits the number of nested updates to prevent infinite loops

I'm able to reduce the ResizeObserver errors using this, but not sure if this is the right approach:

vi.stubGlobal(
  'ResizeObserver',
  vi.fn(() => ({
    observe: vi.fn(),
    unobserve: vi.fn(),
    disconnect: vi.fn()
  }))
)
webJose commented 1 month ago

@KieranP, Svelte v5 now uses ResizeObserver whenever you use bind:clientwidth or bind:clientheight. JsDom doesn't carry a mock for ResizeObserver.

mcous commented 1 month ago

@KieranP same story as ResizeObserver with Element.animate: Svelte 5 now requires Element.animate for transitions, and neither jsdom nor happy-dom provide it. You can try to knock it out with a mock, but comes with the usual downsides of faking out APIs you don't control at a layer you don't control. You can also consider:

Svelte error: effect_update_depth_exceeded
Maximum update depth exceeded...

Hard to say what would cause without seeing more of your code. If you can create a minimal reproduction of this particular error, feel free to throw a link in here or in discord if you need help debugging

KieranP commented 1 month ago

@mcous Thanks for the response. I'll see what I can do about mocking the APIs. Using browser mode for the basic tests I'm doing seems like overkill in our use case, so I want to try avoid it if possible.

As for effect_update_depth_exceeded, it seems that this is a possible undocumented breaking change; some state change that used to work in my Svelte 4 app but not causes a loop and throwing of effect_update_depth_exceeded error in Svelte 5. I've opened a ticket with the Svelte team.

bleucitron commented 3 weeks ago

Hi there ! I'm struggling with TypeError: animation.cancel is not a function, i suppose this is because Animation is not provided by jsdom either.

I haven't been able to mock it correctly so far...

bleucitron commented 3 weeks ago

Ok, i solved my issue.

I first badly mocked Element.prototype.animate as such:

 Element.prototype.animate = vi
    .fn()
    .mockImplementation(() => ({ finished: Promise.resolve() }));

without realising this actually then builds a malformed Animation when invoked, which causes the animation.cancel is not a function error.

Mocking this way solves this :

  Element.prototype.animate = vi
    .fn()
    .mockImplementation(() => ({ cancel: vi.fn(), finished: Promise.resolve() }));
cezaryszymanski commented 2 weeks ago

Hello

Not sure if that was mentioned before, but I cannot really see any way events can be tested with spy after upgrading. Before I used it like:

  const { container, component } = render(Search, { categories: [TestCategoryWithValues] });
  const onChange = vi.fn();
  component.$on("change", onChange);

But in svelte 5 the $on function is no longer present on the component. Is there any alternative way to do this now?

bleucitron commented 2 weeks ago

Not sure if that was mentioned before, but I cannot really see any way events can be tested with spy after upgrading.

I think the only way is to migrate your events to props.

mcous commented 2 weeks ago

But in svelte 5 the $on function is no longer present on the component. Is there any alternative way to do this now?

@cezaryszymanski I believe you can pass an events key to render. The Svelte docs are a little sparse on this option, but we pass it along to mount:

render(Component, {
  props: { /* ... */ },
  events: { /* ... */ },
})

Migrating to function props might be easier

cezaryszymanski commented 1 week ago

After the update my test where I do JSON file upload has 2 errors

TypeError: Cannot set property files of [object HTMLInputElement] which has only a getter

and

TypeError: Failed to set the 'files' property on 'HTMLInputElement': The provided value is not of type 'FileList'

I upload files like in the docs example https://testing-library.com/docs/user-event/utility/#upload

The test still ends with success but with this errors

mcous commented 1 week ago

@cezaryszymanski I'd be happy to help diagnose this issue if you could provide a minimal reproduction in a repository or Stackblitz. It sounds like an issue with @testing-library/user-event and/or JSDOM (or whichever library you're using) rather than @testing-library/svelte, which is only calling Svelte and doesn't affect the behaviors of the DOM elements that get rendered.

Did you update more than just Svelte and STL for your project?

cezaryszymanski commented 6 days ago

@mcous After some more debugging I thing that the second error I mentioned might be specific to my code, but I managed to reproduce the first one https://stackblitz.com/edit/vitejs-vite-ra2ipx?file=src%2Flib%2FFileInput.test.js

It happens when the input has binded $state property (see FileInput.svelte)

mcous commented 6 days ago

@cezaryszymanski I see! Yup, there seems to be a difference between how real browsers handle setting fileInput.files = undefined vs jsdom / happy-dom. A real browser will silently no-op, while jsdom will throw an error. Since this code is in Svelte internals, and doesn't appear to be a bug on Svelte's part, there's not much we can do here.

Since this issue is independent of Svelte and @testing-library/svelte, and is instead a difference between how real browsers behave vs jsdom / happy-dom, if you were so inclined, you could try raising issues with jsdom and happy-dom with minimal reproductions of setting fileInput.files to undefined or null. No idea if you'll get any traction there!

In the meantime, for your tests, I would recommend:

amosjyng commented 3 days ago

I have a local variable in my component that keeps track of whether a typewriter transition effect is playing or not. When testing, the onintrostart function gets called, then the transition tick function gets called with t = 0, then the animation somehow gets cancelled and neither the onintroend nor the tick functions get called one last time, leaving the tests to fail because the typewriter effect clears out the HTML textContent without ever getting to restore it. This does not happen in the browser. @bleucitron 's mock of element.animate works for tests on other components, but not this one with this specific effect.

Any ideas on how this might best be handled? Tests were passing in Svelte 4 but not now in Svelte 5.

mcous commented 3 days ago

Hey @amosjyng! You'd likely need a more complicated fake to simulate transitions - you could try reading through the Svelte internal transition source code to get an idea of what it uses from Element.animate.

That being said, I don't think trying to test animation logic in a fake DOM environment with a mocked out animate method is going to be a very fruitful path. It'll couple your component tests to Svelte internals and only be as good as your ability to simulate a browser in a mock object, assuming the mock doesn't mess up other things.

Instead, I recommend moving these tests to a real browser (via Vitest or Playright) and/or restructuring your code so that the interesting logic is wrapped up in some structure you can test easily. Then, add that unit to a component that simply does wire-up and doesn't have interesting logic nor direct tests of its own

amosjyng commented 2 days ago

Ah, good point, thanks @mcous! I will give browser tests a try.

amosjyng commented 2 days ago

Another issue I've noticed is that Vitest stack traces no longer have accurate line numbers for Svelte files.

For example, if you look at KieranP's stack trace above, the actual error is not from the output line for (var entry of entries), but probably from line 36 instead:

ReferenceError: ResizeObserver is not defined
 ❯ ResizeObserverSingleton.#getObserver node_modules/svelte/src/internal/client/dom/elements/bindings/size.js:54:26
     52|    (this.#observer = new ResizeObserver(
     53|     /** @param {any} entries */ (entries) => {
     54|      for (var entry of entries) {
       |                          ^
     55|       ResizeObserverSingleton.entries.set(entry.target, entry);
     56|       for (var listener of this.#listeners.get(entry.target) || []) {

Another example I have:

TypeError: Cannot read properties of undefined (reading 'hooks')
 ❯ handle_error ../node_modules/@sveltejs/kit/src/runtime/client/client.js:1623:7
    1621| function handle_error(error, event) {
    1622|  if (error instanceof HttpError) {
    1623|   return error.body;
       |       ^
    1624|  }
    1625|

or

Error: Textarea input not found
 ❯ src/lib/controls/SendInputForm.svelte:69:12
     68|   onsubmit={submitInput}
     69| >
     70|   <label for="message" class="accessibility-only">{acce…
       |          ^
     71|   <textarea
     72|     id="message"