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.

Sign up for GitHub

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

Jump to bottom

chore: Upgrade Jest #12607

Merged
merged 9 commits into from
Nov 28, 2023
Merged

chore: Upgrade Jest #12607

merged 9 commits into from
Nov 28, 2023

Conversation

cshfang
Copy link
Member

@cshfang cshfang commented Nov 21, 2023 •
edited
Loading

Description of changes

This PR

  • Upgrades Jest from version 24, which is now over 4 years old, to the current version 29
  • Upgrade monorepo TS version to 4.3 (as minimally supported by ts-jest
  • Removes Jest configuration from all package.json files into configuration files which extend from a singular base configuration at the root of the monorepo
  • Fixes some type errors now that tests are also more strictly typechecked
  • Fixes unit tests which were relying on both async/await and done - which is an anti-pattern and no longer even allowed by newer versions of Jest
  • Updates tests which were importing modules which are not defined by exports in their respective packages (mainly the fetch handler from internal utils) - as Jest now respects the exports property
  • Touches the following non-test files as stricter typing was causing these files under test to fail:
  • Reorganized DataStore main test suite to be broken down into smaller chunks AND run on its own (i.e. explicitly ignored by Lerna during yarn test) as it is starting to run against memory issues
  • Updates various Jest functions (e.g. toBeCalled) which have been marked as deprecated aliases
  • Bumped unit test coverage thresholds up to their current levels

Description of how you validated changes

yarn test

Checklist

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

@cshfang cshfang force-pushed the chore/upgrade-jest branch 6 times, most recently from 4038aa6 to 37a5b73 Compare November 22, 2023 19:26
@cshfang cshfang marked this pull request as ready for review November 22, 2023 19:54
@cshfang cshfang requested review from a team as code owners November 22, 2023 19:55
@HuiSF HuiSF mentioned this pull request Nov 22, 2023
Closed
4 tasks
svidgen
svidgen reviewed Nov 22, 2023
View reviewed changes
Copy link
Member

@svidgen svidgen left a comment

Choose a reason for hiding this comment

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

I have some comments below. But also, are "diagnostics" (type checks) "on" by default now?

packages/api-graphql/__tests__/AWSAppSyncRealTimeProvider.test.ts Outdated Show resolved Hide resolved
packages/api-rest/__tests__/apis/common/publicApis.test.ts Outdated Show resolved Hide resolved
...e/__tests__/custom-pk-typings/model-init-mutable-model-typings/composite-identifier.test.tsx Show resolved Hide resolved
...tore/__tests__/custom-pk-typings/model-init-mutable-model-typings/custom-identifier.test.tsx Show resolved Hide resolved
packages/datastore/__tests__/indexeddb.test.ts Outdated Show resolved Hide resolved
..._/custom-pk-typings/model-init-mutable-model-typings/legacy-backwards-compatibility.test.tsx Show resolved Hide resolved
...ore/__tests__/custom-pk-typings/model-init-mutable-model-typings/managed-identifier.test.tsx Show resolved Hide resolved
...__/custom-pk-typings/model-init-mutable-model-typings/optionally-managed-identifier.test.tsx Show resolved Hide resolved
@cshfang
Copy link
Member Author

cshfang commented Nov 27, 2023

I have some comments below. But also, are "diagnostics" (type checks) "on" by default now?

I believe it is on by default yes. I tried to leave much to the default settings but may be unaware of some issues that I didn't experience first hand - is there an issue with this?

AllanZhengYP
AllanZhengYP reviewed Nov 27, 2023
View reviewed changes
Copy link
Member

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

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

I'm still working on this PR but post some early comments. Overall it looks great! Thank you @cshfang

jest.setup.js Show resolved Hide resolved
jest.config.js Show resolved Hide resolved
jest.config.js Outdated Show resolved Hide resolved
jest.config.js Outdated Show resolved Hide resolved
packages/api-graphql/__tests__/AWSAppSyncRealTimeProvider.test.ts Outdated Show resolved Hide resolved
packages/api-graphql/__tests__/helpers.ts Outdated Show resolved Hide resolved
AllanZhengYP
AllanZhengYP reviewed Nov 27, 2023
View reviewed changes
Copy link
Member

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

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

I think I have covered most of the categories, I only have a fiew more questions.

packages/auth/__tests__/providers/cognito/rememberDevice.test.ts Show resolved Hide resolved
packages/storage/__tests__/providers/s3/utils/client/S3/cases/shared.ts Show resolved Hide resolved
packages/storage/__tests__/providers/s3/utils/client/S3/cases/listObjectsV2.ts Show resolved Hide resolved
AllanZhengYP
AllanZhengYP previously approved these changes Nov 27, 2023
View reviewed changes
Copy link
Member

@AllanZhengYP AllanZhengYP left a comment

Choose a reason for hiding this comment

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

🚀🚀🚀

stocaaro
stocaaro previously approved these changes Nov 28, 2023
View reviewed changes
Copy link
Member

@stocaaro stocaaro left a comment

Choose a reason for hiding this comment

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

Most of this change looks really straightforward. Some of the auth mock rework introduces enough diff that it would be hard to tease out logic difference.

The real test is, "are the tests passing?", followed by "did we drop coverage?". I've done enough review to satisfy myself that the upgrade is more valuable than the risks. I think I made a single comment about coverage, maybe there are more that I missed.

Good work fighting this one through. It'll be good to get back to current with jest. Thank you.

packages/storage/__tests__/providers/s3/utils/client/S3/cases/getObject.ts Show resolved Hide resolved
@cshfang cshfang dismissed stale reviews from stocaaro and AllanZhengYP via 38e0e99 November 28, 2023 18:10
elorzafe
elorzafe approved these changes Nov 28, 2023
View reviewed changes
Copy link
Contributor

@elorzafe elorzafe left a comment

Choose a reason for hiding this comment

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

Looks good
thanks @cshfang 🎖️

@cshfang cshfang merged commit 8ca7c15 into aws-amplify:main Nov 28, 2023
31 of 40 checks passed
@svidgen
Copy link
Member

svidgen commented Dec 4, 2023

I have some comments below. But also, are "diagnostics" (type checks) "on" by default now?

I believe it is on by default yes. I tried to leave much to the default settings but may be unaware of some issues that I didn't experience first hand - is there an issue with this?

Sorry for the delay on response. No issue. "On" is definitely what we want. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@svidgen svidgen svidgen left review comments

@stocaaro stocaaro stocaaro approved these changes

@elorzafe elorzafe elorzafe approved these changes

@HuiSF HuiSF HuiSF approved these changes

@AllanZhengYP AllanZhengYP AllanZhengYP approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cshfang @svidgen @AllanZhengYP @HuiSF @elorzafe @stocaaro