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: handle error from pool.run #1348

Merged
merged 4 commits into from
May 21, 2022

Conversation

sapphi-red
Copy link
Contributor

When an error was thrown from pool.run, vitest finished without any error messages.
This PR handles that error.

I found this while implementing vitejs/vite#8049.

Before

 RUN  v0.12.8 D:/documents/GitHub/vite

····*···---******************************---------··········***····································******···*********·····*·**··········***

There was no error message.

After

 RUN  v0.12.8 D:/documents/GitHub/vite

stderr | unknown test
"default" is imported from external module "path" but never used in "playground-temp/ssr-vue/src/entry-server.js".

················································---································································---------··················-----------··············································--·*··························-··································-****·············································································-------··---····································----------·······················································*·············************··

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Failed Suites 1 ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
 FAIL  playground/react-emotion/__tests__/react.spec.ts [ playground/react-emotion/__tests__/react.spec.ts ]
Error: Cannot find package '@emotion/babel-plugin' imported from D:\documents\GitHub\vite\babel-virtual-resolve-base.js
 ❯ new NodeError node_modules/.pnpm/@babel+core@7.17.10/node_modules/@babel/core/lib/vendor/import-meta-resolve.js:2669:5
 ❯ packageResolve node_modules/.pnpm/@babel+core@7.17.10/node_modules/@babel/core/lib/vendor/import-meta-resolve.js:3325:9
 ❯ moduleResolve node_modules/.pnpm/@babel+core@7.17.10/node_modules/@babel/core/lib/vendor/import-meta-resolve.js:3359:18
 ❯ defaultResolve node_modules/.pnpm/@babel+core@7.17.10/node_modules/@babel/core/lib/vendor/import-meta-resolve.js:3398:13
 ❯ node_modules/.pnpm/@babel+core@7.17.10/node_modules/@babel/core/lib/vendor/import-meta-resolve.js:3421:14
 ❯ asyncGeneratorStep node_modules/.pnpm/@babel+core@7.17.10/node_modules/@babel/core/lib/vendor/import-meta-resolve.js:63:103
 ❯ _next node_modules/.pnpm/@babel+core@7.17.10/node_modules/@babel/core/lib/vendor/import-meta-resolve.js:65:194
 ❯ node_modules/.pnpm/@babel+core@7.17.10/node_modules/@babel/core/lib/vendor/import-meta-resolve.js:65:364

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯Serialized Error: {
  "code": "PLUGIN_ERROR",
  "hook": "transform",
  "id": "D:/documents/GitHub/vite/playground-temp/react-emotion/index.html?html-proxy&index=0.js",
  "plugin": "vite:react-babel",
  "pluginCode": "ERR_MODULE_NOT_FOUND",
  "watchFiles": [
    "D:/documents/GitHub/vite/playground-temp/react-emotion/index.html",
    "vite/modulepreload-polyfill",
    "D:/documents/GitHub/vite/playground-temp/react-emotion/index.html?html-proxy&index=0.js",
  ],
}
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯
Test Files  1 failed | 57 passed | 3 skipped (64)
     Tests  422 passed | 47 skipped (487)
      Time  22.63s (in thread 174.57s, 12.97%)

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Unhandled Errors ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Vitest caught 1 unhandled error during the test run. This might cause false positive tests.
Please, resolve all the errors to make sure your tests are not affected.

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Unhandled Error ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯AggregateError: 
 ❯ Object.runTests ../../../../file:/D:/documents/GitHub/vitest/packages/vitest/dist/vendor-cli-api.bcfd31a5.js:8250:15
 ❯ processTicksAndRejections ../../../../node:internal/process/task_queues:96:5        
 ❯ async ../../../../file:/D:/documents/GitHub/vitest/packages/vitest/dist/vendor-cli-api.bcfd31a5.js:10440:9
 ❯ async Vitest.runFiles ../../../../file:/D:/documents/GitHub/vitest/packages/vitest/dist/vendor-cli-api.bcfd31a5.js:10450:12
 ❯ async Vitest.start ../../../../file:/D:/documents/GitHub/vitest/packages/vitest/dist/vendor-cli-api.bcfd31a5.js:10377:5
 ❯ async startVitest ../../../../file:/D:/documents/GitHub/vitest/packages/vitest/dist/vendor-cli-api.bcfd31a5.js:11113:5
 ❯ async start ../../../../file:/D:/documents/GitHub/vitest/packages/vitest/dist/cli.js:665:9
js:661:3

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯Serialized Error: {
  "errors": [
    [Error: ENOENT: no such file or directory, open 'D:\documents\GitHub\vite\playground-temp\js-sourcemap\dist\assets\index.704baece.js'],
    [Error: ENOENT: no such file or directory, open 'D:\documents\GitHub\vite\playground-temp\worker\dist\iife\index.html'],
    [Error: ENOENT: no such file or directory, open 'D:\documents\GitHub\vite\playground-temp\worker\dist\iife-sourcemap\assets\my-worker.b928aeec.js'],
  ],
}
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

It shows as a Unhandled Error but it is now much better than no error message.
To be honest, I don't know where it should be handled.

@netlify
Copy link

netlify bot commented May 21, 2022

Deploy Preview for vitest-dev ready!

Name Link
🔨 Latest commit 7478246
🔍 Latest deploy log https://app.netlify.com/sites/vitest-dev/deploys/6288f18174ba8f00084b8a48
😎 Deploy Preview https://deploy-preview-1348--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 settings.

Copy link
Member

@sheremet-va sheremet-va left a comment

Choose a reason for hiding this comment

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

CI is not happy :S

packages/vitest/src/node/pool.ts Outdated Show resolved Hide resolved
packages/vitest/src/utils/index.ts Outdated Show resolved Hide resolved
@sheremet-va
Copy link
Member

Also, we are calling this function in

await this.pool.runTests(files, invalidates)

Which has a try/catch 🤔

@sapphi-red
Copy link
Contributor Author

Also, we are calling this function in

await this.pool.runTests(files, invalidates)

Which has a try/catch 🤔

When an error happens here, these two .close() will not be called.

await pool.run(data, { transferList: [workerPort], name })

port.close()
workerPort.close()

I don't know what happens if it is not called, but it seems it needs to be called.

@sheremet-va
Copy link
Member

When an error happens here, these two .close() will not be called.

I guess they need to be called, yes (so we need to keep your try/finally), but do we need to throw AggregateError then? Or you think it's better for overall DX? If so, I think we might need a better error message showing where errors came from.

@sapphi-red
Copy link
Contributor Author

sapphi-red commented May 21, 2022

If I use Promise.all instead of Promise.allSettled, vitest finishes immediately with an error.
Yes I thought it is better to use AggregateError for DX.

I think we might need a better error message showing where errors came from.

I thought it will be better to have those too but I didn't come up with how to implement it.
Currently, I don't understand why the error is coming from pool.run.

@antfu antfu merged commit fbaa546 into vitest-dev:main May 21, 2022
@sapphi-red sapphi-red deleted the fix/handle-error-pool-run branch May 22, 2022 07:48
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.

3 participants