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

fix(runner): correctly initialize suite in task #7374

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

Conversation

wmertens
Copy link
Contributor

@wmertens wmertens commented Jan 28, 2025

Description

this adds a test with a custom runner that allows you to wait for other tests, even when they weren't scheduled. It requires suite to be defined at context extension time, and this is corrected in vitest.

  • It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

  • Run the tests with pnpm test:ci.

Documentation

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

Copy link

netlify bot commented Jan 28, 2025

Deploy Preview for vitest-dev ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 55a96d5
🔍 Latest deploy log https://app.netlify.com/sites/vitest-dev/deploys/6799555426be480008a3c9e6
😎 Deploy Preview https://deploy-preview-7374--vitest-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@wmertens
Copy link
Contributor Author

I can't really figure out why the test fails. It seems that the fnMap in the runner gets re-inited, somehow that module is being imported twice.

But while I was creating it, the test was passing.

@hi-ogawa
Copy link
Contributor

I haven't fully understood the logic around here, but potentially this delete task.suite might prevent what you're trying to do?

file.tasks.forEach((task) => {
// task.suite refers to the internal default suite object
// it should not be reported
if (task.suite?.id === '') {
delete task.suite
}
})

@wmertens
Copy link
Contributor Author

@hi-ogawa so when I just initialize the task with the suite in suite.ts, everything works in my repo. It only removes the suite if the id is '', which is fine for my use case.

I suppose the test.only errors can be disabled for linting, but linting seems to pass?

The problem is that somehow the runner support map.ts is imported twice. When I put a console.log in the module, it sometimes fires twice, and then the test functions added for getFn are undefined. When using vitest from an npm install, this doesn't happen.

@wmertens
Copy link
Contributor Author

@hi-ogawa oh disregard sorry, I now found the actual error you explained https://github.com/vitest-dev/vitest/actions/runs/13021120069/job/36321826300?pr=7374#step:8:226
Fixing now.

this adds a test with a custom runner that allows you to wait for other tests, even when they weren't scheduled. It requires `suite` to be defined at context extension time.
@@ -304,7 +304,7 @@ function createSuiteCollector(
const task: Test = {
id: '',
name,
suite: undefined!,
suite,
Copy link
Contributor

@hi-ogawa hi-ogawa Jan 30, 2025

Choose a reason for hiding this comment

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

Related to #7374 (comment), technically we want to keep this undefined for top-level task? (in your example, "supports passing options" test)
Otherwise, context.task.suite is defined during extendTaskContext, but becomes undefined during test execution for example.

@hi-ogawa
Copy link
Contributor

Does this support getting ancestor like test.suite.suite? This might also require assigning suite of suite early here?

suite = {
id: '',
type: 'suite',

@wmertens
Copy link
Contributor Author

wmertens commented Feb 3, 2025

@hi-ogawa I didn't look into the suite logic really. What matters is that the extendTaskContext will get the same suite name as runTask.

I couldn't easily find where the suite id gets set, isn't that known from the start?

Maybe the suite could be set in the task only if suite.name is defined?

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Feb 4, 2025

I think there's a way to properly fix the initialization, but it might require more careful considerations (maybe something like this #7414). In the mean time, I don't mind having your use case fixed with a simple change and follow up separately. I'll check with team.

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Feb 4, 2025

@wmertens Btw, do you have your custom runner somewhere in open source? It would be nice to know more context and concrete code if it's available.

@hi-ogawa hi-ogawa added the p2-to-be-discussed Enhancement under consideration (priority) label Feb 4, 2025
@wmertens
Copy link
Contributor Author

wmertens commented Feb 4, 2025

@hi-ogawa Well, it's fully open-sourced in this PR now ;) we use it internally, because our e2e test suite does expensive initializations and we want to reuse them in subsequent tests.

For example, suppose you want to test an S3 bucket, filling it with 1000 files for various things, this runner allows you to do it in stages, skipping stages you're not testing right now etc.

I'd be happy to help make this part of the actual API as well. Another API approach would be to export a result() function and have test() return a handle:

import {it, result} from 'vitest'

const init = it('should initialize', async () => {
  const stuff = await makeStuff()
  expect(stuff.foo).toEqual(500)
  return stuff
})

it('should bar', async () => {
  const stuff = await result(init)
  await expect(stuff.doThing()).resolves.toBeTruthy()
})

you could also allow -1, -2 etc as arguments to result(), which would refer to previous tests, but that's more error prone when inserting tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-to-be-discussed Enhancement under consideration (priority)
Projects
Status: P2 - 3
Development

Successfully merging this pull request may close these issues.

2 participants