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

Tests are failing after updating to Vite 4.1.0 #2834

Open
6 tasks done
yuliankarapetkov opened this issue Feb 8, 2023 · 30 comments
Open
6 tasks done

Tests are failing after updating to Vite 4.1.0 #2834

yuliankarapetkov opened this issue Feb 8, 2023 · 30 comments
Labels
p2-edge-case Bug, but has workaround or limited in scope (priority)

Comments

@yuliankarapetkov
Copy link

yuliankarapetkov commented Feb 8, 2023

Describe the bug

I'm using SvelteKit with Vitest. I updated SvelteKit and Vitest to their latest versions - 1.5.0 and 0.28.4 respectfully. However, updating Vite from 4.0.4 to 4.1.0 caused 15% of my tests to fail (60/400).

Most of the errors seem unreasonable. For example, removing a DOM element throws a Failed to execute removeChild on Node error. Or Found multiple elements with the text: ... where there's just a single element containing that exact copy; Error: Unable to fire a "click" event - please provide a DOM element., Error: unable to find element with text, etc.

Reproduction

Here's a simple StackBlitz example.

  1. Open the link
  2. Wait for the installation to complete
  3. Stop the execution in the terminal (Cmd + C)
  4. Run npm run test
  5. The test fails

image

Then do the following:

  1. Open package.json
  2. Change the version of vite from 4.1.0 to 4.0.4
  3. Update the dependencies
  4. Open the terminal again and run npm run test
  5. The test succeeds

image

The example explained

It's a simple component with an if statement:

<script lang="ts">
  import { onMount } from "svelte"

  let showMessage = false

  onMount(() => {
    showMessage = true
  })
</script>

{#if showMessage}
  <div>Hello</div>
{/if}

Test file:

import { render } from "@testing-library/svelte"
import Hello from "../Hello.svelte"

describe("Hello component", () => {
  it("should show message", async () => {
    const { getByText } = render(Hello)
    expect(getByText("Hello")).toBeInTheDocument()
  })
})

Result with Vite 4.1.0:

TestingLibraryElementError: Unable to find an element with the text: Hello. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.

This is just a single example but similar issues occur where there's some sort of an asynchronous code used (or conditionals), like promises, setTimeout, useFakeTimers() and advanceTimersByTime(), TestingLIbrary's waitFor() function, etc.

More dependencies & config

See StackBlitz example for more info

package.json

"@sveltejs/kit": "^1.5.0",
"@testing-library/jest-dom": "^5.16.5",
"@testing-library/svelte": "^3.2.2",
"jsdom": "^21.1.0",

vite.config.ts

export default defineConfig({
  plugins: [sveltekit()],
  test: {
    include: ['src/**/*.{test,spec}.{js,ts}'],
    globals: true,
    environment: "jsdom",
    setupFiles: ["src/setupTest.js"],
  }
});

setupTest.js

import "@testing-library/jest-dom"

System Info

System:
    OS: macOS 13.1
    CPU: (16) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 1.84 GB / 16.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 19.2.0 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 8.13.1 
  Browsers:
    Brave Browser: 109.1.47.186
    Chrome: 109.0.5414.119
    Firefox: 105.0.1
    Safari: 16.2
  npmPackages:
    vite: 4.1.0 => 4.1.0 
    vitest: ^0.28.4 => 0.28.4

Used Package Manager

npm

Validations

@github-actions
Copy link

github-actions bot commented Feb 8, 2023

Hello @yuliankarapetkov. Please provide a minimal reproduction using a GitHub repository or StackBlitz. Issues marked with need reproduction will be closed if they have no activity within 3 days.

@yuliankarapetkov
Copy link
Author

Hey @sheremet-va, I just updated my comment with a StackBlitz example, along with some further explanation and config. Hope that gives a bit more context of the issue.

@AriPerkkio
Copy link
Member

Test passes with following:

<script lang="ts">
-	import { onMount } from "svelte"
+	import { onMount } from "svelte/internal"
 RERUN  src/lib/Hello.svelte x1

 ✓ src/lib/__test__/Hello.test.ts (1)

 Test Files  1 passed (1)

Maybe related to sveltejs/svelte#5534?
I'm not sure why this passes when vite version is lowered though.

@sheremet-va
Copy link
Member

Test passes with following:

<script lang="ts">

-	import { onMount } from "svelte"

+	import { onMount } from "svelte/internal"

 RERUN  src/lib/Hello.svelte x1



 ✓ src/lib/__test__/Hello.test.ts (1)



 Test Files  1 passed (1)

Maybe related to sveltejs/svelte#5534?

I'm not sure why this passes when vite version is lowered though.

Vitest should still work with import from svelte here. We need to figure out what breaks module resolution here

@yuliankarapetkov
Copy link
Author

@sheremet-va, nice catch! How did you find that?

I can confirm that this was the cause of my bug since after changing all onMount and onDestroy imports in the relevant files, all 60 tests that were failing succeed now.

@AriPerkkio
Copy link
Member

AriPerkkio commented Feb 10, 2023

Started to rollback local vite commit by commit. Test works after reverting vitejs/vite#11595 (:wave: @bluwy).

It seems that entrypoint for Svelte is resolved to svelte/ssr.mjs instead of svelte/index.mjs. In vite@4.0.4 the decision was made based on package.json's module field but in vite@4.1.0 it's done based on exports.

vite@4.0.4:
/Users/x/y/vitest-example-project/node_modules/.pnpm/svelte@3.55.1/node_modules/svelte/index.mjs

vite@4.1.0:
/Users/x/y/vitest-example-project/node_modules/.pnpm/svelte@3.55.1/node_modules/svelte/ssr.mjs

Parameters used here below: https://github.com/vitejs/vite/blob/d953536aae448e2bea0f3a7cb3c0062b16d45597/packages/vite/src/node/plugins/resolve.ts#L1108-L1112

{
  key: '.',
  browser: false,
  require: false,
  conditions: [ 'development', 'module', 'svelte', 'node' ]
}
-> Result = './ssr.mjs'

Svelte's entrypoints: https://github.com/sveltejs/svelte/blob/82d2982845df188631993db6b18c2842e3613acf/package.json#L23-L87

It does make sense that ssr.mjs is imported in Node. I guess the onMount intentionally does not run on SSR? Not sure what should be done here. Maybe vite-plugin-svelte should detect test environment and flip the import? Or if Svelte intentionally does not run onMount on SSR, maybe it should do it when it's run in tests. 😕

Another work-around for projects for now so that test code does not need changes:

import { defineConfig } from "vitest/config";

export default defineConfig({
  test: {
    alias: [
      {
        find: /svelte\/ssr.mjs/,
        replacement: "svelte/index.mjs",
      },
    ],

@sheremet-va
Copy link
Member

sheremet-va commented Feb 10, 2023

It seems that entrypoint for Svelte is resolved to svelte/ssr.mjs instead of svelte/index.mjs. In vite@4.0.4 the decision was made based on package.json's module field but in vite@4.1.0 it's done based on exports.

I think Svelte team wants Vitest to use browser condition (resolve.conditions in config) here. The problem is that other packages will not work with it, if we enable it by default (one of the most popular things to do with jest and jsdom right now is adding node condition overriding default browser one, because package authors don’t expect this code to run inside a node with emulated browser environment).

@TorstenDittmann
Copy link

Just wanna leave a hack here that worked for us, without having to change all the imports.

Create or add this to your test setup file

import * as svelteinternal from 'svelte/internal';
import { vi } from 'vitest';

beforeAll(() => {
    vi.mock('svelte', () => svelteinternal);
});

@AriPerkkio
Copy link
Member

I think Svelte team wants Vitest to use browser condition (resolve.conditions in config) here.

Using resolve.conditions seems to work. Maybe Vitest should expose conditions in configuration API, so that setting that one would not affect production builds? Similar as test.alias is done.

The problem is that other packages will not work with it, if we enable it by default

I guess the conditions cannot be set for specific packages only. One option would be to enable browser condition when Vitest detects test environment to be jsdom or happydom. That would at least limit the damage.

@bluwy
Copy link
Contributor

bluwy commented Feb 10, 2023

Yeah the Vite PR change is intentional as it was previously incorrect (though many tooling relied on it). If setting browser condition doesn't cause any other issues, I think it's the best way as long as you have jsdom/happydom to shim the environment.

(Though usually I prefer testing UI components with a real/headless browser)

@sheremet-va
Copy link
Member

For now I recommend using a workaround with conditions.

This is much deeper problem than Svelte issue. Vitest processes svelte/vue/... files with web mode, and processes it with SSR transforms afterwards. But typescript and javascript files are processed with SSR mode enabled (plugins receive ssr flag).

This creates an issue, if code is imported from the same package in two different modes, - especially if code depends on a singleton. Currently to bypass this Vitest hardcodes node condition, but this is not reliable of course (as we can see here Svelte expects Svelte to always run in SSR mode, if it's running inside node, which is not true for Vitest - it's always running in Node, not in browser).

One of the reason why Vitest doesn't process everything with web mode is performance - Vite is (at least, was) really slow to transform a single ts/js file. The other issue is that library authors also expect browser condition to be run in a browser, so they don't follow node ESM spec (file in browser condition doesn't have correct extension).

@jgarplind
Copy link

I'm facing a similar issue and would be interested to know if the mentioned conditions workaround could help.

Would you mind expanding a little bit on what should be done to implement said workaround?

@sheremet-va
Copy link
Member

sheremet-va commented Feb 13, 2023

Would you mind expanding a little bit on what should be done to implement said workaround?

Add "browser" to your resolve.conditions property in the config file.

@vekunz
Copy link

vekunz commented Feb 14, 2023

If I set resolve.conditions to browser in vite.config.js

resolve: {
  conditions: [
    ...process.env.VITEST ? ['browser'] : []
  ]
}

I get another error multiple times, when running the unit tests

Error: require() of ES Module C:\...\node_modules\jose\dist\browser\index.js from C:\...\node_modules\openid-clien
t\lib\client.js not supported.
Instead change the require of index.js in C:\...\node_modules\openid-client\lib\client.js to a dynamic import() which is available in all CommonJS module
s.

@sheremet-va
Copy link
Member

If I set resolve.conditions to browser in vite.config.js

Yes, conditions work for both require and import calls. So, if we define the custom condition, it should always be in the same format (or just cjs, since import can handle it) as the file that imports it. @dominikg, you might want to keep this in mind 👀 It is also possible to define it with two types:

{
  "browser": {
    "import": "./path.mjs"
    "require": "./path.cjs"
  }
}

But this doesn't really make sense for library authors, because "browser" condition should be executed in the browser, and it doesn't support "require". This is a very complicated issue, and I hope we can discuss it with the Vite team when we start moving vite-node into Vite core.

Right now you can use hardcoded aliases for this instead of browser condition (but some svelte libraries might not work!):

export default defineConfig({
  test: {
    alias: {
      svelte: "svelte/internal"
    }
  }
})

As you can see, the state of ESM/CJS is a mess 😄

@dominikg
Copy link
Contributor

dominikg commented Feb 14, 2023

setting the browser condition via config directly can be a problem too, as vite resolve.conditions is treated a bit differently than other values by vite config merging.

To make sure the browser condition is added in the front and you don't modify conditions when vitest is not active, use a plugin with config hook:

// ...
const vitestBrowserConditionPlugin = {
  name: 'vite-plugin-vitest-browser-condition',
  config({resolve}) {
    if(process.env.VITEST) {
      resolve.conditions.unshift('browser'); 
    }
  }
}
export default defineConfig({
  // ...
  plugins: [vitestBrowserConditionPlugin,svelte()]
})

@dominikg
Copy link
Contributor

Unfortunately that won't help with @vekunz case, openid-client depends on jose and jose package.json has an interesting exports map. for deno and bun they expose the same export as for browser, but for generic import/require, they export a node specific file.

so with adding the browser condition you're changing what gets imported from jose and that doesn't work inside node. oops.

@sheremet-va
Copy link
Member

for deno and bun they expose the same export as for browser, but for generic import/require, they export a node specific file.

I think they have somewhat correct exports map. They don't expect their code to be imported with require and "browser" condition at the same time, which is a fair assumption to make.

I think the problem comes from what developers consider a "browser" condition. Should it be used, when "browser-like" environment is initiated (code has access to global "window" and "document" for example), or when the environment itself is a browser (meaning, the code will be processed by build tools)? I think most people consider it to be the second one, and that's why I am hesitant to add this condition in Vitest by default. For me, the biggest issue is that require will choose this export and fail (what happened in @vekunz and will happen to many other people).

@dominikg
Copy link
Contributor

Agree that what jose does works in regular environments and usecases, still interesting that bun/deno get the browser file via their own conditions whereas node gets a node file via "import"/"require".

The trick with alias above is a more targeted way to ensure svelte is resolved to what it exports for browsers, so in situations where there are competing needs it can be a workaround, though this might become a problem if svelte decided to change its exports structure in a future major release (you'll know from the release notes and your tests crashing so not too bad)

@vekunz
Copy link

vekunz commented Feb 22, 2023

Right now you can use hardcoded aliases for this instead of browser condition (but some svelte libraries might not work!):

export default defineConfig({
  test: {
    alias: {
      svelte: "svelte/internal"
    }
  }
})

Unfortunately, this does not work for me. If I set this configuration, a get a new error:

Error: Missing "./internal/internal" specifier in "svelte" package

@bluwy
Copy link
Contributor

bluwy commented Feb 22, 2023

I guess it needs to be:

export default defineConfig({
  test: {
    alias: [
      { find: /^svelte$/, replacement: "svelte/internal" }
    ]
  }
})

to be more accurate.

janosh added a commit to janosh/svelte-toc that referenced this issue Mar 12, 2023
* fix scroll to active ToC li aborting scroll from within-page links

* readme document new prop scrollBehavior: 'auto' | 'smooth' = `smooth`

* fix tests from vitest broken svelte/internal module resolution

vitest-dev/vitest#2834

* clean up
angryziber added a commit to codeborne/svelte-sample that referenced this issue Mar 21, 2023
Note that vite 4.1+ requires browser condition to run onMount() in svelte components properly during tests

vitest-dev/vitest#2834 (comment)
@josdejong
Copy link

Any news on this issue?

@sheremet-va
Copy link
Member

This should be fixed in the latest version. If you still have problems, considered enabling deps.experimentalOptimizer.

@Iuriy-Budnikov
Copy link

@sheremet-va it still has an issue. experimentalOptimizer doesn't help

@Iuriy-Budnikov
Copy link

 alias: [
      { find: /^svelte$/, replacement: "svelte/internal" }
    ]

It works. Thank you!

@josdejong
Copy link

The issue is indeed still there. The workaround works for me.

Not sure if this is helpful, but to reproduce the issue with the project that I'm working o:

  1. Clone the project

    $ git clone https://github.com/josdejong/svelte-jsoneditor.git
    $ cd svelte-jsoneditor
    $ npm install
    
  2. Open the file vitest.config.js, and remove the line:

    alias: [{ find: /^svelte$/, replacement: 'svelte/internal' }],
  3. Run the tests:

    $ npm test
    

    This throws an error:

    FAIL  src/lib/components/JSONEditor.test.ts > JSONEditor > render text mode
    TestingLibraryElementError: Unable to find an element with the text: "Joe".
    

So, for some reason, the Svelte component with these contents is not rendered during the test. All tests pass with the alias workaround (and with Vite 4.0.x).

@james-camilleri
Copy link

Just chipping in that this issue is still present as of vitest 0.34.6.

@sibiraj-s
Copy link

Another option would be to add browser to the resolve conditions

import { sveltekit } from '@sveltejs/kit/vite';
import { defineConfig } from 'vitest/config';

export default defineConfig({
  plugins: [sveltekit()],
  // see https://github.com/testing-library/svelte-testing-library/issues/222
  resolve: {
    ...(process.env.VITEST
      ? {
          conditions: ['browser', 'import', 'module', 'default']
        }
      : null),
  },
});

So far, in my project with a simple setup, it works regardless of the conditions order, as long as browser is added to the list.

@josdejong
Copy link

@sibiraj-s thanks for sharing. I tried your workaround but that doesn't work in my case (the alias workaround works fine though).

@sibiraj-s
Copy link

Oh, Not sure about the reason, but here is the link for the code that I used. https://github.com/sibiraj-s/svelte-tiptap/blob/3cc167aa67aeb3af1bcd74bbaa68d6011b1d6324/vite.config.ts#L8

samson84 added a commit to samson84/view-lib-comparison that referenced this issue Dec 30, 2023
Configure @testing-library/jest-dom correctly to
resolve the type definitions in the test files,
even the plugin is extended in a global setup file.

The Svelte component's onMount hooks right now executed
properly. Unfortunately vitest running in node but Svelte
render's the components in SSR mode, so the onMount callbacks
didn't executed in tests. An ugly workaround was needed to
solve this:

vitest-dev/vitest#2834 (comment)
@sheremet-va sheremet-va added bug p4-important Violate documented behavior or significantly improves performance (priority) and removed pending triage labels Feb 12, 2024
@sheremet-va sheremet-va moved this to P2 - 4 in Team Board Feb 12, 2024
@sheremet-va sheremet-va removed the status in Team Board Feb 15, 2024
@sheremet-va sheremet-va added p2-edge-case Bug, but has workaround or limited in scope (priority) p3-minor-bug An edge case that only affects very specific usage (priority) and removed p4-important Violate documented behavior or significantly improves performance (priority) bug labels Feb 15, 2024
@sheremet-va sheremet-va removed the p3-minor-bug An edge case that only affects very specific usage (priority) label Mar 14, 2024
@sheremet-va sheremet-va moved this to Has plan in Team Board Mar 28, 2024
@sheremet-va sheremet-va moved this from Has plan to Later in Team Board Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-edge-case Bug, but has workaround or limited in scope (priority)
Projects
Status: Later
Development

No branches or pull requests