Failed tests don't remove element from DOM

[chrisolsen]

Because my svelte component is being compiled to a web component I need to do some additional validation on passed in attributes within the onMount function. If I throw an exception for an invalid attribute type the element isn't removed from the DOM and all later tests then fail due to the findByRole method then finding more than one element.

  onMount(() => {
    if (!isButtonType(type)) {
      throw "Invalid button type";
    }
    if (!isSize(size)) {
      throw "Invalid button size";
    }
    if (!isVariant(variant)) {
      throw "Invalid button variant";
    }
  })

  describe("size", () => {
    ["compact", "foo"].forEach(size => {
      it(`should render ${size} size`, async () => {
        const { findByRole } = render(GoAButton, { size });
        const button = await findByRole("button");

        expect(button).toBeTruthy();
        expect(button.className).toContain(size);
      });
    });
  });

Errors raised

I have tried calling the cleanup method in an afterEach, but that didn't change anything.

[ObieMunoz]

Hi,
I had a similar issue previously and believe this will be addressed properly by utilizing screen, which is the recommended practice.

Import screen from @testing-library/svelte and update your test as follows:

  describe("size", () => {
    ["compact", "foo"].forEach(size => {
      it(`should render ${size} size`, () => {
        render(GoAButton, { size });
        const button = screen.findByRole("button");

        expect(button).toBeTruthy();
        expect(button.className).toContain(size);
      });
    });
  });

Pulling queries directly out of the render should also be avoided. There are certain circumstances where you will retrieve a query function that is unusable.

[austinworks]

Hello, I am also seeing an issue with this. It appears to happen with async test cases where the test throws an exception. I set up a simple problem case to reproduce it with a standard build of sveltekit. Maybe someone can take a look?

https://github.com/austinworks/render-problem-example

[canastro]

We're you able to find any workaround for your issue @austinworks ?

[austinworks]

Seems like a legit screen pollution issue, even happens when using the { container } or { getByRole } returned from the render within the test.

best I've been able to figure out is wrapping the target version of the render in an identifier and then using queries within the test to scope it. I added a branch to demo this: https://github.com/austinworks/render-problem-example/blob/selector_workaround/src/example/CompTest.test.ts

[mcous]

@yanick I dug into this and can confirm that this is, indeed, a cleanup bug in svelte-testing-library. It is difficult to reproduce in our own test suite due to #222, but can be done with some of the trickery mentioned in that thread.

1. By default, render will add a div to the body element:

svelte-testing-library/src/pure.js

Line 26 in c0ff791

target = target || container.appendChild(document.createElement('div'))

2. Then, it will create the Svelte component:

svelte-testing-library/src/pure.js

Lines 57 to 60 in c0ff791

	let component = new ComponentConstructor({
	target,
	...checkProps(options)
	})

3. Then, it will add the component and <div> to its internal cache:

svelte-testing-library/src/pure.js

Line 62 in c0ff791

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

4. Finally, it will read from the cache to cleanup

svelte-testing-library/src/pure.js

Lines 108 to 110 in c0ff791

	const cleanup = () => {
	Array.from(containerCache.keys()).forEach(cleanupAtContainer)
	}

If the component throws in step (2) (or internals throw between steps (1) and (2)), the target will never be added to the container cache and thus, it will never get cleaned up.

For fixing the issues, there's a lot of suspect-looking stuff going on in the code in question. I think this might be a case of "simplify to fix"

• appendChild side-effect in the target initializer
• No way to differentiate between a managed div or an explicitly provided target, which may remove a passed in target from the DOM during cleanup
• Multiple caches with components going into both caches
• Repetition between render and rerender

[austinworks]

Thanks for looking into this.

[yanick]

Repetition between render and rerender

I think that will be alleviated when we merge #210, as rerender will no longer destroy the component, and will be simplified by a lot.

No way to differentiate between a managed div or an explicitly provided target

I have to think a little more before I say anything silly, but perhaps we might do something as simple as add a __stl-wrapper class to the divs we are created, to provide an easy to recognize (and querySelector) them.

[mcous]

I think that will be alleviated when we merge #210, as rerender will no longer destroy the component

Nice, yeah that should help!

perhaps we might do something as simple as add a __stl-wrapper class to the divs

Yeah, something like this feels pretty resilient. I was planning to peep the React and Vue libs to see what they do and check if there's an alignment opportunity

[github-actions]

🎉 This issue has been resolved in version 4.2.0 🎉

The release is available on:

• npm package (@next dist-tag)
• GitHub release

Your semantic-release bot 📦🚀