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

Reject a promise after a test will make test end earlier in browser mode. #7079

Open
6 tasks done
Tackoil opened this issue Dec 13, 2024 · 4 comments
Open
6 tasks done
Labels
feat: browser Issues and PRs related to the browser runner p4-important Violate documented behavior or significantly improves performance (priority)

Comments

@Tackoil
Copy link

Tackoil commented Dec 13, 2024

Describe the bug

If a test case will make an unhandled rejection just after one test and before the next test, the cases in the other files will not be tested.

After debugging, there will post a "done" message included all test files. And It will break the next test process.

async function reportUnexpectedError(
  type,
  error,
) {
  const processedError = serializeError(error)
  await client.waitForConnection().then(() => {
    return client.rpc.onUnhandledError(processedError, type)
  }).catch(console.error)
  const state = __vitest_browser_runner__

  if (state.type === 'orchestrator') {
    return
  }

  // `state.runTests` and `__vitest_worker__.current`  will both be `null`
  if (!state.runTests || !__vitest_worker__.current) {
    channel.postMessage({
      type: 'done',
      filenames: state.files,
      id: state.iframeId,
    })
  }
}

Reproduction

https://stackblitz.com/edit/vitest-dev-vitest-ceqdspje

This issue can also be reproduced by following steps.

  1. mkdir to create a new folder, and npm init.
  2. npx vitest init browser to init the test with browser mode.
  3. modify test command in package.json to run vitest run.
  4. make two test files as following.
import { it } from "vitest";
// Import some source file. It's necessary, but I don't know why.
import { add } from "../src/add";

it("should add two numbers", () => {
  new Promise((resolve, reject) => {
    reject("test");
  });
});
  1. run npm test:browser.

The result will show that only one file is tested.

 RUN  v2.1.8 /Users/<username>/Developer/vitest-playground

 ✓ test/add.test.ts (1)

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Unhandled Errors ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

Vitest caught 1 unhandled error during the test run.
This might cause false positive tests. Resolve unhandled errors to make sure your tests are not affected.

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Unhandled Rejection ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Unknown Error: test
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

 Test Files  1 passed (1)
      Tests  1 passed (1)
     Errors  1 error
   Start at  18:22:47
   Duration  842ms (transform 0ms, setup 0ms, collect 4ms, tests 1ms, environment 0ms, prepare 191ms)

 ELIFECYCLE  Command failed with exit code 1.

System Info

System:
    OS: macOS 15.2
    CPU: (8) arm64 Apple M2
    Memory: 111.73 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.15.1 - ~/Library/pnpm/node
    npm: 10.7.0 - ~/Library/pnpm/npm
    pnpm: 9.15.0 - ~/Library/pnpm/pnpm
  Browsers:
    Chrome: 131.0.6778.110
    Chrome Canary: 133.0.6890.0
    Safari: 18.2
  npmPackages:
    @vitest/browser: ^2.1.8 => 2.1.8
    vitest: ^2.1.8 => 2.1.8

Used Package Manager

npm

Validations

@Tackoil Tackoil changed the title Reject a promise after a test will make test stop earlier in browser mode. Reject a promise after a test will make test end earlier in browser mode. Dec 13, 2024
@sheremet-va sheremet-va added feat: browser Issues and PRs related to the browser runner p4-important Violate documented behavior or significantly improves performance (priority) and removed pending triage labels Dec 13, 2024
@sheremet-va
Copy link
Member

The result will show that only one file is tested.

Does it show that only one test failed? Or does it run only one test?

I think it's expected behaviour that Vitest doesn't start new tests when the environment is potentially broken which is why this safeguard mechanism exists. If we didn't have this check, Vitest could hang.

@Tackoil
Copy link
Author

Tackoil commented Jan 22, 2025

Does it show that only one test failed? Or does it run only one test?

It will run only one test, and the result of that test will be different with the actual test case. In sample above, it'll be passed.

I'm also agreed that safeguard mechanism is necessary to keep Vitest running as expected. But this handled rejection is caused by test case, not Vitest itself. It'll be better to start the following tests, and end up with unhandled error failed or exit with 0 when dangerouslyIgnoreUnhandledErrors is on.

As the discussion in discord, I'm shifting a historical project with lots of badly designed cases which all throw unhandled rejections. For rapid development, I would like to ignore them first and solve them gradually. Because I quite sure they will not cause any false positive tests as they are in another testing frameworks.

So in my opinion, it'll be better to run all tests first and alert for the unhandled errors. Safeguard can be implemented in other strategy like watchdog timer. And this cases making Vitest hanging may be also reported as a potential bug of Vitest.

@sheremet-va
Copy link
Member

But this handled rejection is caused by test case, not Vitest itself. It'll be better to start the following tests

It doesn't matter what called what in your reproduction. The error happened outside of the test body. If the error was handled inside the test, then Vitest won't fail. This code works correctly, for example:

test('test unhandled rejection', async () => {
  const promise = new Promise((resolve) => {
    window.addEventListener('unhandledrejection', () => resolve())
  })
  new Promise((_, reject) => reject('test'))
  await promise
})

Changing this behaviour is definitely not a priority.

As the discussion in discord, I'm shifting a historical project with lots of badly designed cases which all throw unhandled rejections. For rapid development, I would like to ignore them first and solve them gradually. Because I quite sure they will not cause any false positive tests as they are in another testing frameworks.

You can use Vitest's include option to include only tests that work correctly.

So in my opinion, it'll be better to run all tests first and alert for the unhandled errors. Safeguard can be implemented in other strategy like watchdog timer. And this cases making Vitest hanging may be also reported as a potential bug of Vitest.

As I already explained, we cannot do that because we will have to remove the safeguard. From the framework perspective, the current safeguard is much better than allowing some weird unhandled rejections to be ignored because the tests are written incorrectly.

@Tackoil
Copy link
Author

Tackoil commented Jan 23, 2025

I think I understand your opinion now. Keeping the safeguard unchanged seems to be ok to me.

As a developer using Vitest, the test result is confusing. It seems that Vitest only collect one test case and ignore the others. I cost lots of time to double check the include setting, and another lots of time to find out why Vitest failed to collect all tests. As a result, file an issue as it.

It would be better to adding more notes in this case. For example, "Vitest exited unexpectedly, checking unhandled errors or file an issue". Or just add the total count of collected cases in the result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: browser Issues and PRs related to the browser runner p4-important Violate documented behavior or significantly improves performance (priority)
Projects
None yet
Development

No branches or pull requests

2 participants