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

feat: global setup #372

Merged
merged 9 commits into from Jan 10, 2022
Merged

feat: global setup #372

merged 9 commits into from Jan 10, 2022

Conversation

dominikg
Copy link
Contributor

@dominikg dominikg commented Dec 30, 2021

fixes #300

ideas for tests, documentation and api improvements welcome

Questions:

  • this only worked for javascript files in my manual tests. how important is ts support and what needs to be done
  • should buildEnd be in a separate enforce: 'post' plugin
  • jest has a slightly different api with two separate files. This PR has more flexibility but is it ok to diverge from jest here

@netlify
Copy link

netlify bot commented Dec 30, 2021

✔️ Deploy Preview for vitest-dev ready!

🔨 Explore the source changes: 62d95d5

🔍 Inspect the deploy log: https://app.netlify.com/sites/vitest-dev/deploys/61dbf28b8a7c0500079e63bd

😎 Browse the preview: https://deploy-preview-372--vitest-dev.netlify.app

@patak-dev
Copy link
Member

Great! About the API, is there a reason we want to use a file-based API instead of accepting the setup function in the config?

/// <reference types="vitest" />

import { defineConfig } from 'vite'

export default defineConfig({
  test: {
    global: true,
    globalSetup: async function() {
      const server = await startServer()
      return async() => {
        await server.close()
      }
    },
  }
})

Is the idea that it could be less verbose because you normally will need to import this function from a file anyways? Maybe we could accept a function (or array of functions), but still, allow to pass a file name as a shortcut? (with the same API as you propose, but it looks better as an optional shortcut than as the API for this feature to me... but maybe everyone is very used to using files for this purpose)

@dominikg
Copy link
Contributor Author

i did take a cue from jests feature which uses files (likely because their config can also be json) and it aligns with vitest's own setupFiles

Directly using a function in config would simplify the internal implementation which i like a lot. But then you'd have to use regular imports for all the things you want/need in setup and that could bloat the config itself. I'm leaning towards the file approach due to prior art and dx result.

But if using the function approach enables ts support without extra work and the bloat is not an issue i'm all for it. in the end jest users can write a simple wrapper around their existing files and import that

@patak-dev
Copy link
Member

What about providing both ways? (both in setupFiles and globalSetup). You can pass a setup function (with optional cleanup as a return param) or a module path if that is more comfortable (or a list of these). I know this will be two ways to do the same things, but seeing the module path as a shortcut for the real function seems a good way to justify it (and also due to compat). We could maybe avoid the named exports in the module version and only accept the default export form to simplify the API?
@antfu do you have a preference here?

@antfu
Copy link
Member

antfu commented Dec 30, 2021

The setupFiles can't be an inline function as it is supposed to be running in each worker with different environments. For globalSetup I see no problem with that.

@dominikg
Copy link
Contributor Author

dominikg commented Dec 30, 2021

implementation as function here: https://github.com/dominikg/vitest/tree/feat/global-setup-function

the complexity in vitest is greatly reduced, except for this little "hack": https://github.com/dominikg/vitest/blob/22c5ec2ef05c4001ffaff43d581914c2328e2e62/packages/vitest/src/node/pool.ts#L80

Unfortunately stacktraces don't look great when globalSetup gets bundled into the config:

20:26 $ pnpm test

> @vitest/test-globalSetup@ test /home/dominikg/develop/vitest/test/global-setup
> vitest

file:///home/dominikg/develop/vitest/test/global-setup/vitest.config.ts.js?t=1640892430013:9
  throw new Error("ooops");
        ^

Error: ooops
    at vitest_global_setup_default (file:///home/dominikg/develop/vitest/test/global-setup/vitest.config.ts.js?t=1640892430013:9:9)

this is not good and i'd rather have a stacktrace referencing the correct files and lines. Is that something that can be improved in vite's config bundling?

@dominikg dominikg force-pushed the feat/globalSetup branch 2 times, most recently from 543a61a to cb2710a Compare January 3, 2022 09:27
- **Type:** `string | string[]`

Path to global setup files, relative to project root
```js
Copy link
Contributor

Choose a reason for hiding this comment

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

The setup file entry doesn't have an example of how to use the setting. Might consider removing it here (or we might end up needing to put examples for each rule.

}
```

Multiple globalSetup files are possible. setup and teardown are executed sequentially with teardown in reverse order.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe adding a callout to this will do it a little more justice 🙂

}
```

Files can either export named functions
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Files can either export named functions
Example

```

or a setup function as default that returns the teardown
```js
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be all in one example (divised by comments to show the different approaches

@antfu
Copy link
Member

antfu commented Jan 10, 2022

Let's ship it for now, and we can see how we could improve the stacktrace later.

@dominikg
Copy link
Contributor Author

The stacktraces in this PR should be fine. I'm still a bit worried about the intermittent test failures. Havn't been able to reliably reproduce/debug it unfortunately as it only happens on windows and trying to get to the bottom of a possible v8 issue inside a virtual machine running windows hasn't been a great experience at all.

maybe @userquin has an idea?

Steps to try to reproduce:

  • checkout this PR
  • run pnpm i && pnpm build
  • run pnpm test:ci

sometimes it fails with an error like this (different testcases)

examples/vue test: FATAL ERROR: v8::FromJust Maybe value is Nothing.
examples/vue test:  1: 00007FF63D8E30AF v8::internal::CodeObjectRegistry::~CodeObjectRegistry+112511
examples/vue test:  2: 00007FF63D872216 DSA_meth_get_flags+65542
examples/vue test:  3: 00007FF63D8730CD node::OnFatalError+301
examples/vue test:  4: 00007FF63E18F4C5 v8::V8::FromJustIsNothing+53
examples/vue test:  5: 00007FF63D85B203 v8::base::CPU::has_sse+31491
examples/vue test:  6: 00007FF63D945BF7 uv_timer_stop+1207
examples/vue test:  7: 00007FF63D9421CB uv_async_send+331
examples/vue test:  8: 00007FF63D94195C uv_loop_init+1292
examples/vue test:  9: 00007FF63D941AFA uv_run+202
examples/vue test: 10: 00007FF63D910C05 node::SpinEventLoop+309
examples/vue test: 11: 00007FF63D7AB4D0 v8::internal::wasm::SignatureMap::Freeze+35776
examples/vue test: 12: 00007FF63D7A6B28 v8::internal::wasm::SignatureMap::Freeze+16920
examples/vue test: 13: 00007FF63D9322DD uv_poll_stop+557
examples/vue test: 14: 00007FF63E743DF0 v8::internal::compiler::RepresentationChanger::Uint32OverflowOperatorFor+146416
examples/vue test: 15: 00007FFC64087034 BaseThreadInitThunk+20
examples/vue test: 16: 00007FFC64E82651 RtlUserThreadStart+33
examples/vue test: Failed
undefined

@antfu antfu merged commit eaa119f into vitest-dev:main Jan 10, 2022
vikunja-bot pushed a commit to go-vikunja/frontend that referenced this pull request Jan 10, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [vitest](https://github.com/vitest-dev/vitest) | devDependencies | patch | [`0.0.139` -> `0.0.140`](https://renovatebot.com/diffs/npm/vitest/0.0.139/0.0.140) |

---

### Release Notes

<details>
<summary>vitest-dev/vitest</summary>

### [`v0.0.140`](https://github.com/vitest-dev/vitest/releases/v0.0.140)

[Compare Source](vitest-dev/vitest@v0.0.139...v0.0.140)

##### Bug Fixes

-   inline snapshot if not called inside suite ([6d743c5](vitest-dev/vitest@6d743c5)), closes [#&#8203;484](vitest-dev/vitest#484)
-   **ui:** flex / percentage based layout ([#&#8203;492](vitest-dev/vitest#492)) ([c43ebaf](vitest-dev/vitest@c43ebaf))
-   correctly inline shapshot with properties ([4603ffd](vitest-dev/vitest@4603ffd))
-   mocking is lost with threads: false ([28b97d8](vitest-dev/vitest@28b97d8)), closes [#&#8203;482](vitest-dev/vitest#482)
-   Reflect.get called on non-object ([3c9073a](vitest-dev/vitest@3c9073a)), closes [#&#8203;479](vitest-dev/vitest#479)
-   snapshot ignores indentation ([aff1481](vitest-dev/vitest@aff1481))
-   **ui:** reduce graph container size ([#&#8203;478](vitest-dev/vitest#478)) ([23e1e62](vitest-dev/vitest@23e1e62))

##### Features

-   global setup ([#&#8203;372](vitest-dev/vitest#372)) ([eaa119f](vitest-dev/vitest@eaa119f))
-   **ui:** tasks state group ([7782e7d](vitest-dev/vitest@7782e7d))

</details>

---

### Configuration

📅 **Schedule**: At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Reviewed-on: https://kolaente.dev/vikunja/frontend/pulls/1348
Co-authored-by: renovate <renovatebot@kolaente.de>
Co-committed-by: renovate <renovatebot@kolaente.de>
@bhvngt
Copy link

bhvngt commented Jan 13, 2022

I was trying out this feature and came across this peculiar issue. I am using an isomorphic library supabase-js. When I import it in a file that is sourced by globalSetup, it throws following error.

21:38:19 [vite] Error when evaluating SSR module /node_modules/@supabase/supabase-js/dist/main/index.js:
ReferenceError: exports is not defined
    at eval (/node_modules/@supabase/supabase-js/dist/main/index.js:17:23)
    at instantiateModule (/home/projects/node-3cxxqh/node_modules/vite/dist/node/chunks/dep-76613303.js:60094:15)
21:38:19 [vite] Error when evaluating SSR module ./test/globalSetup.ts:
ReferenceError: exports is not defined
    at eval (/node_modules/@supabase/supabase-js/dist/main/index.js:17:23)
    at instantiateModule (/home/projects/node-3cxxqh/node_modules/vite/dist/node/chunks/dep-76613303.js:60094:15)
(node:6) UnhandledPromiseRejectionWarning: ReferenceError: exports is not defined
    at eval (/node_modules/@supabase/supabase-js/dist/main/index.js:17:23)
    at instantiateModule (/home/projects/node-3cxxqh/node_modules/vite/dist/node/chunks/dep-76613303.js:60094:15)

When I import the same in a normal test file it works fine.

Here's the playground where the error is reproduced.

@patak-dev
Copy link
Member

@bhvngt could you create an issue with this info so it is easier to track it?

@bhvngt
Copy link

bhvngt commented Jan 13, 2022

@patak-dev #522

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.

Support Global Setup
5 participants