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

test(setup): fix server closure #3056

Merged
merged 1 commit into from
Apr 19, 2021
Merged

test(setup): fix server closure #3056

merged 1 commit into from
Apr 19, 2021

Conversation

nihalgonsalves
Copy link
Member

@nihalgonsalves nihalgonsalves commented Apr 19, 2021

Description

Attempt to fix test teardown issues.

The current failures can be reliably reproduced using:

yarn test-serve packages/playground/ssr-vue/__tests__/ssr-vue.spec.ts --detectOpenHandles

This fixes that case.

Additional context

Fixes these errors:

ReferenceError: You are trying to `import` a file after the Jest environment has been torn down.

Cannot log after tests are done. Did you forget to wait for something async in your test?

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

Comment on lines +49 to +54
await new Promise((resolve) => {
server.close(resolve)
})
if (vite) {
await vite.close()
}
Copy link
Member

Choose a reason for hiding this comment

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

I think you can shorten this to

Suggested change
await new Promise((resolve) => {
server.close(resolve)
})
if (vite) {
await vite.close()
}
await new Promise((resolve) => server.close(resolve))
await vite?.close()

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 use an optional chain because of pre-Node-14 environments – is this file transpiled by Jest?

Copy link
Member

Choose a reason for hiding this comment

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

ah okay, I'm so used to TypeScript ^^

@Shinigami92 Shinigami92 added feat: ssr p1-chore Doesn't change code behavior (priority) test labels Apr 19, 2021
@patak-dev
Copy link
Member

Failed on windows because of the 30000 ms general jest timeout, we should bump it to 40000 at least. We may do that in another PR

@Shinigami92
Copy link
Member

Shinigami92 commented Apr 19, 2021

On the windows runner there was an error https://github.com/vitejs/vite/pull/3056/checks?check_run_id=2384573026#step:10:29

FAIL packages/playground/ssr-react/__tests__/ssr-react.spec.ts
   Test suite failed to run

    Timeout - Async callback was not invoked within the 30000 ms timeout specified by jest.setTimeout.Error: Timeout - Async callback was not invoked within the 30000 ms timeout specified by jest.setTimeout.

      at mapper (node_modules/jest-jasmine2/build/queueRunner.js:27:45)

@patak-js 30s are already very high, isn't it? 🤔

@patak-dev
Copy link
Member

Looks like 30 sec is not enough, no?

@Shinigami92
Copy link
Member

Looks like 30 sec is not enough, no?

But what is it waiting for? Will this occur in real production environment?
Will humans want to wait so long? Or think that something hung up?

@Shinigami92
Copy link
Member

Shinigami92 commented Apr 19, 2021

Hey I will re-run the job and look if the windows runner fails with the same timeout-issue


Works ✔️

@Shinigami92 Shinigami92 requested review from antfu and patak-dev April 19, 2021 20:57
@patak-dev
Copy link
Member

@nihalgonsalves since checks have passed, I think you could make this one ready for review and work on further improvements in another PR

@nihalgonsalves nihalgonsalves marked this pull request as ready for review April 19, 2021 21:02
@patak-dev patak-dev merged commit 70536b4 into vitejs:main Apr 19, 2021
@nihalgonsalves nihalgonsalves deleted the jest-fix branch April 19, 2021 21:04
@brillout
Copy link
Contributor

@nihalgonsalves Thanks this has been quite a pain ❤️ CC @dominikg

This was referenced Apr 22, 2021
TobiasMelen pushed a commit to TobiasMelen/vite that referenced this pull request May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p1-chore Doesn't change code behavior (priority) test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants