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

Don't swallow test failures caused by POSIX signals #32688

Merged
merged 6 commits into from
Dec 21, 2021

Conversation

ramosbugs
Copy link
Contributor

The run-tests.js script is used for running the Next.JS regression tests on GitHub Actions. It uses child_process.spawn to run each test in a child process, whose exit event returns both a code and signal. Unfortunately, the current script only checks code, which swallows any test failures due to, e.g., Node.JS throwing a SIGSEGV. This has the potential to hide significant failures in the CI pipeline.

Before patch (succeeds despite SIGSEGV!):

$ node run-tests.js --timings --debug test/integration/amphtml/test/index.test.js
Running tests with concurrency: 2
Running tests: 
 test/integration/amphtml/test/index.test.js

Starting test/integration/amphtml/test/index.test.js retry 0/1
"next/jest" is currently experimental. https://nextjs.org/docs/messages/experimental-jest-transformer
info  - Using locally built binary of @next/swc
  console.log
    Running command "next build /Users/daramos/Documents/Projects/next.js/test/integration/amphtml/"

      at lib/next-test-utils.js:167:17

Finished test/integration/amphtml/test/index.test.js on retry 0/1 in 10.757s
$ echo $?
0

After patch (fails as expected):

$ node run-tests.js --timings --debug test/integration/amphtml/test/index.test.js
Running tests with concurrency: 2
Running tests: 
 test/integration/amphtml/test/index.test.js

Starting test/integration/amphtml/test/index.test.js retry 0/1
"next/jest" is currently experimental. https://nextjs.org/docs/messages/experimental-jest-transformer
  console.log
    Running command "next build /Users/daramos/Documents/Projects/next.js/test/integration/amphtml/"

      at lib/next-test-utils.js:167:17

CHILD EXITED WITH null
Cleaning test files at /Users/daramos/Documents/Projects/next.js/test/integration/amphtml/test
Starting test/integration/amphtml/test/index.test.js retry 1/1
"next/jest" is currently experimental. https://nextjs.org/docs/messages/experimental-jest-transformer
  console.log
    Running command "next build /Users/daramos/Documents/Projects/next.js/test/integration/amphtml/"

      at lib/next-test-utils.js:167:17

CHILD EXITED WITH null
test/integration/amphtml/test/index.test.js failed due to Error: failed with signal: SIGSEGV
test/integration/amphtml/test/index.test.js failed to pass within 1 retries
$ echo $?
1

Locally (on Mac OS 12.1 tested with both Node 16.13.1 and 14.18.2 on latest Next.JS canary branch), I'm running into segfaults due to nodejs/node#35889 (see above), and I have a worrying suspicion that this same issue may be causing Next.JS tests to silently fail on CI too.

run-tests.js Outdated Show resolved Hide resolved
@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

@ijjk

This comment has been minimized.

Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

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

Good catch, thanks for the PR!

@ijjk
Copy link
Member

ijjk commented Dec 21, 2021

Stats from current PR

Default Build (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary ramosbugs/next.js run-tests-errors Change
buildDuration 19.6s 19.7s ⚠️ +65ms
buildDurationCached 3.8s 4s ⚠️ +165ms
nodeModulesSize 348 MB 348 MB ⚠️ +579 B
Page Load Tests Overall decrease ⚠️
vercel/next.js canary ramosbugs/next.js run-tests-errors Change
/ failed reqs 0 0
/ total time (seconds) 3.26 3.343 ⚠️ +0.08
/ avg req/sec 766.79 747.72 ⚠️ -19.07
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.497 1.577 ⚠️ +0.08
/error-in-render avg req/sec 1669.99 1585.42 ⚠️ -84.57
Client Bundles (main, webpack, commons)
vercel/next.js canary ramosbugs/next.js run-tests-errors Change
450.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42.2 kB 42.2 kB
main-HASH.js gzip 30.3 kB 30.3 kB
webpack-HASH.js gzip 1.45 kB 1.45 kB
Overall change 74.1 kB 74.1 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary ramosbugs/next.js run-tests-errors Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary ramosbugs/next.js run-tests-errors Change
_app-HASH.js gzip 1.37 kB 1.37 kB
_error-HASH.js gzip 194 B 194 B
amp-HASH.js gzip 312 B 312 B
css-HASH.js gzip 326 B 326 B
dynamic-HASH.js gzip 2.37 kB 2.37 kB
head-HASH.js gzip 350 B 350 B
hooks-HASH.js gzip 919 B 919 B
image-HASH.js gzip 4.73 kB 4.73 kB
index-HASH.js gzip 263 B 263 B
link-HASH.js gzip 2.13 kB 2.13 kB
routerDirect..HASH.js gzip 321 B 321 B
script-HASH.js gzip 383 B 383 B
withRouter-HASH.js gzip 318 B 318 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 14.1 kB 14.1 kB
Client Build Manifests
vercel/next.js canary ramosbugs/next.js run-tests-errors Change
_buildManifest.js gzip 459 B 459 B
Overall change 459 B 459 B
Rendered Page Sizes
vercel/next.js canary ramosbugs/next.js run-tests-errors Change
index.html gzip 533 B 533 B
link.html gzip 547 B 547 B
withRouter.html gzip 528 B 528 B
Overall change 1.61 kB 1.61 kB

Default Build with SWC (Decrease detected ✓)
General Overall increase ⚠️
vercel/next.js canary ramosbugs/next.js run-tests-errors Change
buildDuration 21.1s 21.2s ⚠️ +123ms
buildDurationCached 3.8s 3.8s -50ms
nodeModulesSize 348 MB 348 MB ⚠️ +579 B
Page Load Tests Overall decrease ⚠️
vercel/next.js canary ramosbugs/next.js run-tests-errors Change
/ failed reqs 0 0
/ total time (seconds) 3.356 3.402 ⚠️ +0.05
/ avg req/sec 744.89 734.76 ⚠️ -10.13
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.51 1.58 ⚠️ +0.07
/error-in-render avg req/sec 1655.35 1581.84 ⚠️ -73.51
Client Bundles (main, webpack, commons)
vercel/next.js canary ramosbugs/next.js run-tests-errors Change
450.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42.3 kB 42.3 kB
main-HASH.js gzip 30.4 kB 30.4 kB
webpack-HASH.js gzip 1.44 kB 1.44 kB
Overall change 74.3 kB 74.3 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary ramosbugs/next.js run-tests-errors Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary ramosbugs/next.js run-tests-errors Change
_app-HASH.js gzip 1.35 kB 1.35 kB
_error-HASH.js gzip 180 B 180 B
amp-HASH.js gzip 305 B 305 B
css-HASH.js gzip 321 B 321 B
dynamic-HASH.js gzip 2.36 kB 2.36 kB
head-HASH.js gzip 342 B 342 B
hooks-HASH.js gzip 906 B 906 B
image-HASH.js gzip 4.75 kB 4.75 kB
index-HASH.js gzip 256 B 256 B
link-HASH.js gzip 2.19 kB 2.19 kB
routerDirect..HASH.js gzip 314 B 314 B
script-HASH.js gzip 375 B 375 B
withRouter-HASH.js gzip 309 B 309 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 14.1 kB 14.1 kB
Client Build Manifests
vercel/next.js canary ramosbugs/next.js run-tests-errors Change
_buildManifest.js gzip 458 B 458 B
Overall change 458 B 458 B
Rendered Page Sizes
vercel/next.js canary ramosbugs/next.js run-tests-errors Change
index.html gzip 530 B 530 B
link.html gzip 545 B 545 B
withRouter.html gzip 525 B 525 B
Overall change 1.6 kB 1.6 kB
Commit: 001cfdb

@ijjk ijjk merged commit 36a6e43 into vercel:canary Dec 21, 2021
@ramosbugs ramosbugs deleted the run-tests-errors branch December 21, 2021 23:18
@vercel vercel locked as resolved and limited conversation to collaborators Jan 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants