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: introduce the new reporter API #7069

Draft
wants to merge 39 commits into
base: main
Choose a base branch
from

Conversation

sheremet-va
Copy link
Member

@sheremet-va sheremet-va commented Dec 11, 2024

Description

THIS IS A DRAFT.

interface Reporter {
  /**
   * This method is called when the error happens outside of the test.
   *
   * Possible errors:
   * - unhandled error thrown during the test run
   * - error thrown during collection phase when importing a module (global throw)
   * - error thrown during collection phase when calling `describe` callback
   * - error thrown inside `beforeAll`/`afterAll` hooks since it can't be attributed to a specific test
   */
  onError(error: SerialisedError, suite?: TestSuite): void

  /**
   * This method is called when all tests inside a single file were collected.
   * The `file` object contains `children()` method with all suites and tests.
   * Tasks won't have `result` property yet.
   *
   * **Note:** This method can be called in parallel if multiple files are collected at the same time (`--single-worker` is used)
   */
  onFileCollected(file: TestModule): void

  /**
   * This method is called when the runner is ready to start running a test. The `result` will always be `undefined`.
   * **Note:** If test is marked as `todo` or `skip`, this method will still be called.
   */
  onTestPrepare(test: TestCase): void
  /**
   * This method is called when the test finished running. It will always have a `result` property.
   */
  onTestFinished(test: TestCase): void
  /**
   * This method is called only after the test has finished running and the `result.type` is `failed`.
   */
  onTestFailed(test: TestCase): void

  /**
   * This method is called when user logs something to the console.
   * The order of logs is not guaranteed.
   */
  onConsoleLog(type: 'stderr' | 'stdout', log: UserConsoleLog): void

  /**
   * This is called when all tests have finished running.
   */
  onTestRunFinished(files: TestModule[], errors: SerialisedError[], reason: 'passed' | 'failed' | 'timedout' | 'interrupted'): void

  /**
   * Coverage is performed for all files that were executed.
   * This is called only if `--coverage` flag is used.
   */
  onCoverage(summary: any): void
}

The current lifecycle ("run tests" has its own lifecycle):

image

image

tenor

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • 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 Dec 11, 2024

Deploy Preview for vitest-dev ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit eb36354
🔍 Latest deploy log https://app.netlify.com/sites/vitest-dev/deploys/677d29b609f6d10008361455
😎 Deploy Preview https://deploy-preview-7069--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.

@AriPerkkio AriPerkkio mentioned this pull request Dec 17, 2024
6 tasks
@AriPerkkio

This comment was marked as resolved.

@@ -0,0 +1,3 @@
# Test Lifecycle

<!-- TODO: lifecyle diagram and reporter API -->
Copy link
Member

Choose a reason for hiding this comment

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

Could we use vitepress-plugin-mermaid for this? 🤔

https://emersonbottero.github.io/vitepress-plugin-mermaid/guide/styles.html

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe when the lifecycle is finalised

@sheremet-va
Copy link
Member Author

sheremet-va commented Dec 19, 2024

  • onHookStart(...) - before* hooks is called

if we report suite hooks (beforeAll/beforeEach), should we report onSuiteStart?

  • onTestFinish / onTestFail

I think now it's better to have a single hook (onTestFinish) and check the result there - I updated the TestCase to always return a result() (now return pending if no result is set)

  • onTestFile*

Should be onTestModule*. The idea with the name is that I expect us to implement support for virtual modules.

@sheremet-va
Copy link
Member Author

@AriPerkkio should we update the onTaskUpdate payload to include the type of action to make it easier to parse? (test-started, file-finished, etc)

@AriPerkkio
Copy link
Member

if we report suite hooks (beforeAll/beforeEach), should we report onSuiteStart?

Do we need that anywhere? I don't think onTaskUpdate reports this atm. Currently slowly running beforeAll etc. hooks are showed by reporters so we need to support those.

Should be onTestModule*. The idea with the name is that I expect us to implement support for virtual modules.

Yup, let's use onTestModule* and onTestCase* naming convention in the hooks. It matches the TS interfaces too.

should we update the onTaskUpdate payload to include the type of action to make it easier to parse? (test-started, file-finished, etc)

I'm not yet sure. The entity.type and entity.state() should be enough. 🤔

@sheremet-va
Copy link
Member Author

sheremet-va commented Dec 29, 2024

I'm not yet sure. The entity.type and entity.state() should be enough. 🤔

entity is a user facing API. I am asking if we should send the type to ourselves when we trigger onTaskUpdate in the test runtime to parse it more easily on the server. Instead of sending updateTask(task), we do something like updateTask('test-start', task) - it's an internal improvement

@AriPerkkio AriPerkkio force-pushed the feat/new-reporter-api branch from a9c5863 to 64abbda Compare December 29, 2024 11:03
@sheremet-va
Copy link
Member Author

sheremet-va commented Dec 29, 2024

Do we need that anywhere? I don't think onTaskUpdate reports this atm. Currently slowly running beforeAll etc. hooks are showed by reporters so we need to support those.

If it doesn't report it, we can start reporting it. I just don't understand why some hooks are reported and some not - I see it from the practical standpoint, but philosophically - why? What makes them more special? beforeAll/afterAll can also take a long time to execute

@AriPerkkio
Copy link
Member

beforeAll/afterAll are showed by reporters, let's keep supporting them like before. I feel like I'm missing something here. 🤔

if (task?.result?.hooks) {
for (const [hook, state] of Object.entries(task.result.hooks)) {
if (state === 'run' || state === 'queued') {
startingHooks.push({ name: hook, file: task.file, id: task.id, type: task.type })
}
else {
endingHooks.push({ name: hook, file: task.file, id: task.id, type: task.type })
}
}
}

@sheremet-va
Copy link
Member Author

beforeAll/afterAll are showed by reporters, let's keep supporting them like before. I feel like I'm missing something here. 🤔

if (task?.result?.hooks) {
for (const [hook, state] of Object.entries(task.result.hooks)) {
if (state === 'run' || state === 'queued') {
startingHooks.push({ name: hook, file: task.file, id: task.id, type: task.type })
}
else {
endingHooks.push({ name: hook, file: task.file, id: task.id, type: task.type })
}
}
}

beforeEach hook is tied to a test case, it's obvious what reporter hooks will be called and in what order. I feel like with beforeAll it's not so obvious because there is no corresponding onTestSuite* hook

@sheremet-va
Copy link
Member Author

sheremet-va commented Jan 3, 2025

I'd like to remove the public state in the future (we can keep errors and stuff there internally) and replace it with a testRun. Maybe we should make the testRun public, but keep the methods internal?

@sheremet-va
Copy link
Member Author

sheremet-va commented Jan 7, 2025

some questions I have after working on the lifecycle diagram (need to polish it and add to the docs when we are finished) - right now it just reports when we call onTaskUpdate, but doesn't mention new reporters (at least, it's easier to see where things are wrong)

  1. How can the test have a failed state even before we run it? (there is an if for it and and event test-failed-early). A similar event can happen in a suite when it has an error during collection
  2. Should we add more hook events to remove parsing of onTaskUpdate's hooks altogether? Should we rely more on events now overall? There are some events that are not so easy to distinguish from one another (for example, the hook update also reports the test state - if the test was retried, this is one extra call)
  3. onTestFinished is not called if the test was skipped with a dynamic context.skip - should it be called still? 🤔
  4. Should we report onTestSuiteStart/onTestSuiteEnd? We call onTaskUpdate in this case already (notice that if suite was skipped, it will still report a start update, but won't report end, unlike testCase that doesn't report anything if it was skipped)
  5. How do we report test retrying? Right now we report the start, but retrying is updated after afterEach is called making onTaskUpdate harder to parse (btw, both retrying and repeating is reported in the same way as a single update)

Updated hooks proposal:

image

@AriPerkkio AriPerkkio force-pushed the feat/new-reporter-api branch from a0861c2 to b5b962f Compare January 7, 2025 16:15
}

if ((event === 'suite-hook-start' || event === 'suite-hook-end') && entity.task.result?.hooks) {
for (const hook of Object.keys(entity.task.result.hooks)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Wouldn't it report all hooks that it has? For example, after afterAll, the task can have beforeAll, beforeEach, afterEach and afterAll in this object

Copy link
Member

Choose a reason for hiding this comment

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

I haven't been able to get this part working fine yet. Feel free to change it, the test case in test-run.test.ts should print tasks correctly once fixed.


// Skipped tests need to be reported manually once test module has finished
for (const test of entity.children.allTests()) {
if (test.result().state === 'skipped') {
Copy link
Member Author

Choose a reason for hiding this comment

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

do we need to report skipped tests? they won't have start and calling them arbitrary at the end seems weird to me

Copy link
Member

Choose a reason for hiding this comment

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

Currently this is how dot and summary reporters display skipped tests. Are there better ways for it?

Copy link
Member Author

@sheremet-va sheremet-va Jan 7, 2025

Choose a reason for hiding this comment

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

We can report them when any suite-finished is emitted so it's at least timed with other tests, although the order is still wrong, - and they won't have start event. Also if suite skips all tests, suite-finished is never fired 🤔

I don't know what to do here, honestly (a new hook?), but the current solution is wrong on all levels 😄

We can just never report them, and do the thing you did here manually in every reporter

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.

2 participants