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 Jest runner timeout accounting races #11148

Closed
wants to merge 3 commits into from

Conversation

stefreak
Copy link

@stefreak stefreak commented May 17, 2024

What does this PR do?

Fixes #11147

Jest runner would report incorrect timeouts, due to races in the timer
accounting code.

This commit arms and disarms timers before and after every test to
ensure correct timeouts without false-positives.

  • Documentation or TypeScript types (it's okay to leave the rest blank in this case)
  • Code changes

How did you verify your code works?

I ran the test suite in #11147 manually and added to the automated test suite.

  • I included a test for the new code, or an existing test covers it
  • I wrote TypeScript/JavaScript tests and they pass locally (bun-debug test test-file-name.test)

Fixes oven-sh#11147

Jest runner would report incorrect timeouts, due to races in the timer
accounting code.

This commit arms and disarms timers before and after every test to
ensure correct timeouts without false-positives.
@gvilums
Copy link
Contributor

gvilums commented May 21, 2024

Thank you for the PR, these changes look like they'll reduce some of the flakiness around timeouts

Copy link
Contributor

@gvilums gvilums left a comment

Choose a reason for hiding this comment

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

If CI passes I think we should merge this

Copy link

github-actions bot commented May 21, 2024

@stefreak, your commit has failing tests :(

💪 1 failing tests Darwin AARCH64

  • test/js/node/tls/node-tls-context.test.ts 1 failing

🐧🖥 1 failing tests Linux x64

  • test/js/node/child_process/child_process-node.test.js 1 failing

🪟💻 6 failing tests Windows x64 baseline

  • test/cli/install/bunx.test.ts 1 failing
  • test/js/bun/test/test-test.test.ts 1 failing
  • test/js/bun/test/timeout.test.js 3 failing
  • test/js/node/process/process.test.js 2 failing
  • test/js/node/tls/node-tls-context.test.ts code 1
  • test/js/node/watch/fs.watchFile.test.ts 3 failing

🪟💻 7 failing tests Windows x64

  • test/cli/install/bunx.test.ts 1 failing
  • test/cli/install/migration/out-of-sync.test.ts 1 failing
  • test/integration/next-pages/test/dev-server-ssr-100.test.ts 1 failing
  • test/js/bun/test/timeout.test.js 3 failing
  • test/js/node/process/process.test.js 2 failing
  • test/js/node/tls/node-tls-context.test.ts code 1
  • test/js/node/watch/fs.watchFile.test.ts SIGKILL

View logs

@gvilums
Copy link
Contributor

gvilums commented May 22, 2024

@stefreak Some of the new timeout tests seem to fail on windows. This is most likely because timers on windows are less accurate. I suggest increasing the tolerances in the tests and seeing if that fixes it.

@stefreak
Copy link
Author

@gvilums thank you for the feedback! I'll have a look at that

@stefreak
Copy link
Author

I'm trying to figure out which of these test failures are related to my change, and which are flakes. Could you approve the workflows again @gvilums? Thank you ❤️

@stefreak
Copy link
Author

I finally managed to get a working windows dev environment, but actually this has been fixed in the mean time by #11419

@stefreak stefreak closed this May 30, 2024
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.

Bun Jest runner incorrect timeouts
2 participants