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

WIP: chore(test): jest to vitest #28454

Closed
wants to merge 1 commit into from
Closed

Conversation

watany-dev
Copy link
Contributor

To the AWS CDK Team,

I hope this message finds you well. I'm writing to suggest the adoption of Vitest in place of Jest, as Vitest has recently reached version 1.0. As one of the contributors, I have experienced a noticeable amount of time spent waiting for tests to complete. I am more than willing to invest effort to expedite this process.

To give you an idea of the potential integration, I have prepared a sample commit where only aws-ec2 and aws-cloudfront have been transitioned to use Vitest. This sample is intended to demonstrate how Vitest could be integrated into our workflow, potentially enhancing our efficiency.

time npm run test aws-cloudfront
real    2m32.045s
user    5m30.198s
sys     0m49.922s

time npx vitest aws-cloudfront
real    0m13.847s
user    0m22.247s
sys     0m9.893s
time npm run test aws-ec2
real    0m55.543s
user    1m48.104s
sys     0m8.297s

time npx vitest aws-ec2
real    0m26.814s
user    0m50.636s
sys     0m17.872s

I'm looking forward to any thoughts or feedback you might have on this suggestion.

Best regards,

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@github-actions github-actions bot added p2 distinguished-contributor [Pilot] contributed 50+ PRs to the CDK labels Dec 21, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team December 21, 2023 15:40
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@mrgrain
Copy link
Contributor

mrgrain commented Dec 21, 2023

As someone not familiar with vitest, can you explain how vitest achieves such a drastic performance improvement?
I am sure it is just faster in general, but my concern is that your example is unintentionally removing "features" that we came to rely on.

Note there might be a related, but separate discussion if we really need all of the current jest configuration and if potentially some can be removed either way.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: a01ce40
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mrgrain
Copy link
Contributor

mrgrain commented Dec 22, 2023

Specifically vitest seems to use esbuild internally, and we would loose type checking on the test files. I bet this where most of the speed comes from.

Now again, we might not need the typechecking here if it's already done elsewhere anyway. But then we could also just use swc with ts-node. 🤔

@watany-dev
Copy link
Contributor Author

watany-dev commented Dec 23, 2023

@mrgrain

Thank you for the advice. I was careless. Vitest has introduced typecheck from version 1.0, so I measured it.
https://vitest.dev/guide/testing-types#run-typechecking

It seems there is a difference of a few seconds, but considering the benefits of several tens to hundreds of seconds from introducing Vitest, I think it's just a minor difference.

# vitest
npx vitest aws-cloudfront
…
 Test Files  21 passed (21)
      Tests  258 passed (258)
   Start at  14:18:13
   Duration  18.86s (transform 2.11s, setup 14ms, collect 5.60s, tests 56.88s, environment 57ms, prepare 15.92s)

# vitest with typecheck
npx vitest aws-cloudfront --typecheck
…
 Test Files  21 passed (21)
      Tests  258 passed (258)
Type Errors  no errors
   Start at  14:17:17
   Duration  21.06s (transform 2.31s, setup 3ms, collect 6.87s, tests 60.47s, environment 8ms, prepare 15.46s)
# vitest
npx vitest aws-ec2
 Test Files  27 passed (27)
      Tests  783 passed (783)
   Start at  14:27:41
   Duration  26.56s (transform 2.97s, setup 8ms, collect 8.24s, tests 117.51s, environment 15ms, prepare 12.46s)

# vitest with typecheck
npx vitest aws-ec2 --typecheck
 Test Files  27 passed (27)
      Tests  783 passed (783)
Type Errors  no errors
   Start at  14:25:14
   Duration  29.42s (transform 5.57s, setup 7ms, collect 14.18s, tests 113.66s, environment 19ms, prepare 15.46s)

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@mrgrain
Copy link
Contributor

mrgrain commented Jan 11, 2024

@watany-dev I don't think vitest is what make this fast here. With a pre-warmed cache, I can get jest to run pretty speedy as well. I disabled coverage reporting, which adds another 5s to 7s. Other than that it's just the pre-warming. I'm not sure what magic vitest is using to forgo the pre-warming stage or if you accidentally omitted that from your experimentation.

➜  aws-cdk-lib git:(main) ✗ yarn test aws-ec2
yarn run v1.22.19
$ jest aws-ec2

Test Suites: 27 passed, 27 total
Tests:       784 passed, 784 total
Snapshots:   0 total
Time:        18.481 s, estimated 68 s
Ran all test suites matching /aws-ec2/i.
✨  Done in 19.63s.

@mrgrain
Copy link
Contributor

mrgrain commented Jan 11, 2024

Unrelated, did you know you can setup VSCode to run single test cases via the UI? Would that be a better solution to the very real issue of long feedback cycles?

@watany-dev
Copy link
Contributor Author

I was researching a separate matter and realized that this --typecheck option doesn't seem to be useful for our current situation. My apologies!

@watany-dev
Copy link
Contributor Author

watany-dev commented Jan 11, 2024

@mrgrain

Unrelated, did you know you can setup VSCode to run single test cases via the UI? Would that be a better solution to the very real issue of long feedback cycles?

If the cache works and this proposal can be implemented, it would be a fantastic solution to reduce waiting times. I'm not very familiar with this setting, though. Are you referring to the content in the link below?
https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#run-a-cdk-unit-test-in-the-debugger

@aws-cdk-automation
Copy link
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@aws-cdk-automation
Copy link
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Jan 20, 2024
@aws-cdk-automation
Copy link
Collaborator

The pull request linter fails with the following errors:

❌ The title of this pull request does not follow the Conventional Commits format, see https://www.conventionalcommits.org/.

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. distinguished-contributor [Pilot] contributed 50+ PRs to the CDK p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants