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: support --segfault-retry=3 #1854

Merged
merged 7 commits into from Aug 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/ci.yml
Expand Up @@ -9,6 +9,9 @@ on:
branches:
- main

env:
VITEST_SEGFAULT_RETRY: 3

jobs:
lint:
runs-on: ubuntu-latest
Expand Down
1 change: 1 addition & 0 deletions packages/vitest/rollup.config.js
Expand Up @@ -18,6 +18,7 @@ const entries = [
'src/index.ts',
'src/browser.ts',
'src/node/cli.ts',
'src/node/cli-wrapper.ts',
'src/node.ts',
'src/runtime/worker.ts',
'src/runtime/loader.ts',
Expand Down
96 changes: 96 additions & 0 deletions packages/vitest/src/node/cli-wrapper.ts
@@ -0,0 +1,96 @@
/* eslint-disable no-console */
/**
* Wrapper of the CLI with child process to manage segfaults and retries.
*/
import { fileURLToPath } from 'url'
import c from 'picocolors'
import { execa } from 'execa'

const ENTRY = new URL('./cli.mjs', import.meta.url)

interface ErrorDef {
trigger: string
url: string
}

// Node errors seen in Vitest (vitejs/vite#9492)
const ERRORS: ErrorDef[] = [
{
trigger: 'Check failed: result.second.',
url: 'https://github.com/nodejs/node/issues/43617',
},
{
trigger: 'FATAL ERROR: v8::FromJust Maybe value is Nothing.',
url: 'https://github.com/vitest-dev/vitest/issues/1191',
},
{
trigger: 'FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal.',
url: 'https://github.com/nodejs/node/issues/42407',
},
]

async function main() {
// default exit code = 100, as in retries were exhausted
const exitCode = 100
let retries = 0
const args = process.argv.slice(2)

if (process.env.VITEST_SEGFAULT_RETRY) {
retries = +process.env.VITEST_SEGFAULT_RETRY
}
else {
for (let i = 0; i < args.length; i++) {
if (args[i].startsWith('--segfault-retry=')) {
retries = +args[i].split('=')[1]
break
}
else if (args[i] === '--segfault-retry' && args[i + 1]?.match(/^\d+$/)) {
retries = +args[i + 1]
break
}
}
}

retries = Math.max(1, retries || 1)

for (let i = 1; i <= retries; i++) {
if (i !== 1)
console.log(`${c.inverse(c.bold(c.magenta(' Retrying ')))} vitest ${args.join(' ')} ${c.gray(`(${i} of ${retries})`)}`)
await start(args)
if (i === 1 && retries === 1) {
console.log(c.yellow(`It seems to be an upstream bug of Node.js. To improve the test stability,
you could pass ${c.bold(c.green('--segfault-retry=3'))} or set env ${c.bold(c.green('VITEST_SEGFAULT_RETRY=3'))} to
have Vitest auto retries on flaky segfaults.\n`))
}
}
process.exit(exitCode)
}

main()

async function start(args: string[]) {
const child = execa('node', [fileURLToPath(ENTRY), ...args], {
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't it hurt performance for "--segfault-retry=1" that we always starts a child process? Shouldn't we just import ./cli.mjs in this case?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, but we can't show useful error message that way 🤔

I guess it's ok for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't count how many times we invoke vitest in our test, but at the high level, it doesn't make CI time increase. Using this would allow us to provide information about the segfault reasoning and issue link.

reject: false,
all: true,
stderr: 'pipe',
stdout: 'inherit',
stdin: 'inherit',
})
child.stderr?.pipe(process.stderr)
const { all: output = '' } = await child

for (const error of ERRORS) {
if (output.includes(error.trigger)) {
if (process.env.GITHUB_ACTIONS)
console.log(`::warning:: Segmentfault Error Detected: ${error.trigger}\nRefer to ${error.url}`)

const RED_BLOCK = c.inverse(c.red(' '))
console.log(`\n${c.inverse(c.bold(c.red(' Segmentfault Error Detected ')))}\n${RED_BLOCK} ${c.red(error.trigger)}\n${RED_BLOCK} ${c.red(`Refer to ${error.url}`)}\n`)
return
}
}

// no segmentfault found
process.exit(child.exitCode!)
}

1 change: 1 addition & 0 deletions packages/vitest/src/node/cli.ts
Expand Up @@ -39,6 +39,7 @@ cli
.option('--changed [since]', 'Run tests that are affected by the changed files (default: false)')
.option('--sequence <options>', 'Define in what order to run tests (use --sequence.shuffle to run tests in random order)')
.option('--no-color', 'Removes colors from the console output')
.option('--segfault-retry <times>', 'Return tests on segment fault (default: 0)', { default: 0 })
.help()

cli
Expand Down
2 changes: 1 addition & 1 deletion packages/vitest/vitest.mjs
@@ -1,2 +1,2 @@
#!/usr/bin/env node
import './dist/cli.mjs'
import './dist/cli-wrapper.mjs'