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

feat: update jest v20 to v24 #243

Merged
merged 4 commits into from
May 9, 2019
Merged

feat: update jest v20 to v24 #243

merged 4 commits into from
May 9, 2019

Conversation

trivikr
Copy link
Member

@trivikr trivikr commented May 8, 2019

Issue #, if available:
N/A

Description of changes:
This PR updates jest from v20 to v24

Pending (to be done separately as this update is not delayed):

All experimental commits can be viewed in fork https://github.com/trivikr/aws-sdk-js-v3/tree/jest-update

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@@ -2,14 +2,14 @@ import { applyBodyChecksumMiddleware } from "./";
import { HashConstructor, HttpRequest } from "@aws-sdk/types";

describe("applyChecksumMiddleware", () => {
const mockEncoder = jest.fn(() => "encoded");
const mockHashUpdate = jest.fn(() => {});
const mockHashDigest = jest.fn(() => new Uint8Array(0));
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of curiosity, is this code allowed in jest 24?

Copy link
Member Author

Choose a reason for hiding this comment

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

jest.fn().mockReturnValue(value) is just a simple sugar function for jest.fn(() => value)
It's backward compatible, but was removed as it was failing pretest

@AllanZhengYP
Copy link
Contributor

AllanZhengYP commented May 8, 2019

@trivikr Now the default testMatch of jest 24 includes ts file directly(https://jestjs.io/docs/en/configuration#testmatch-array-string). This means we cannot run test command under each sub package, because jest will fail testing the .ts files without setting the transpiler. We might also need to add jest config in each sub-package to make jest only run test on .js files

@trivikr
Copy link
Member Author

trivikr commented May 8, 2019

@AllanFly120 I'll rename root file to jest.config.base.js and extend it in jest.config.js in packages/*
This way param testMatch will be removed when we move to ts-jest

@trivikr
Copy link
Member Author

trivikr commented May 8, 2019

I'd experimented with adding jest.config.js in packages/* while experimenting with removing lerna run pretest in trivikr@13aea99

lerna run pretest will be removed when we move to ts-jest

@AllanZhengYP
Copy link
Contributor

AllanZhengYP commented May 9, 2019

For the record of the off-line discussion on node-http-handler tests:

  1. Jest introduces some changes since 21.X. The override of NODE_TLS_REJECT_UNAUTHORIZED in the unit test is not received by https.request. So the tests for https request failed because of self assigned certs. We have tried different approaches including calling resetModules() but they didn't help.

  2. This situation can be fixed by ping the jest version specificly for this this package. But this require use to run jest test command individually in each packages. This approach make the testing very slow(because of lerna exec) and more importantly, we cannot get global coverage report.

  3. Current step is to temporarily remove the https tests and open issue report to Jest repo. Hopefully they will get it fixed or provide a solution. Then we will add back these tests.

@lock
Copy link

lock bot commented May 24, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked as resolved and limited conversation to collaborators May 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants