Skip to content

Fix: properly destroy components after test case #304

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

royalrex
Copy link

@royalrex royalrex commented Dec 29, 2022

This PR fixes issues around the use of svelte stores.

When components tested are controlled by svelte stores instead of props, they can end up in a long lived state (memory leak actually) where the components are still live in the DOM, and because of the way the cleanup has been done up until now by simply removing the DOM elements, they would still react to any svelte store changes happening in later tests. It further depended on what type of DOM manipulation the component was doing, if this was a problem or not.

The error being thrown is this:

Cannot read properties of null (reading 'insertBefore')

[svelte-app/node_modules/svelte/internal/index.mjs:363:1](http://localhost:8080/__/#)

mjs
  361 | }
  362 | function insert(target, node, anchor) {
> 363 |     target.insertBefore(node, anchor || null);
      | ^
  364 | }
  365 | function insert_hydration(target, node, anchor) {
  366 |     if (is_hydrating && !anchor) {

A repro case with instructions, can be found here: https://github.com/royalrex/svelte-unit-test-bug

The solution was inspired by how Cypress v10.x, that includes built-in support for svelte component testing does their cleanup.
https://github.com/cypress-io/cypress/blob/develop/npm/svelte/src/mount.ts
Lines 25-29 + 74

Specifically was has been done is:

  • Add global reference to latest component instantiated by mount()
  • Add $destroy call before replacing current component

Alternative solution
While testing this solution with styleOptions.html I found that it would be enough to just default to a standard '

" template, as that somehow removes the svelte component more thoroughly. I would still suggest to include this fix as well, as it's more clean and also tests the cleanup function executed in onDestroy(() => {});

See other PR: Chore: Improve html template check (#305)

@royalrex
Copy link
Author

I'm not sure why the tests are failing, maybe they just need a rerun? They run fine locally.

This is the error from circleci:

  1) RxJS users
       fetches users:
     AjaxError: The following error originated from your application code, not from Cypress.

  > ajax error 403

Btw. A suggestion would be to also upgrade circleci to a newer base image than node 12. Maybe bump to v16 or v18: https://endoflife.date/nodejs

I'd be happy to help upgrade the CI pipeline, just let me know.

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.

1 participant