Skip to content

chore: add typechecks for tests of expect-utils and jest-circus #13387

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

Merged
merged 8 commits into from
Oct 6, 2022
Merged

chore: add typechecks for tests of expect-utils and jest-circus #13387

merged 8 commits into from
Oct 6, 2022

Conversation

mrazauskas
Copy link
Contributor

Summary

Fixed types and added typechecks for tests of expect-utils and jest-circus packages.

Test plan

Green CI.

@@ -5,7 +5,7 @@
* LICENSE file in the root directory of this source tree.
*/

import {Circus} from '@jest/types';
import type {Circus} from '@jest/types';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TS caught this. Good. This means that __mocks__ directory is also covered.

@@ -46,16 +46,19 @@ describe('isError', () => {
});

it('should detect errors from another context', () => {
testErrorFromDifferentContext(win => new win.Error());
testErrorFromDifferentContext(
win => new (win as typeof globalThis).Error(),
Copy link
Contributor Author

@mrazauskas mrazauskas Oct 5, 2022

Choose a reason for hiding this comment

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

Error does not exist on type Window, but all works with typeof globalThis. Is fine? Or?

@@ -3,5 +3,6 @@
"compilerOptions": {
"rootDir": "../"
},
"include": ["../**/*"]
"include": ["../**/*"],
"references": [{"path": "../../../jest-expect"}]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@SimenB SimenB Oct 5, 2022

Choose a reason for hiding this comment

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

Shouldn't types be built before type testing tests? I.e., no need for a reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case tsc needs the reference for augmentation. The packages/jest-circus/tsconfig.json has this reference, but the packages/jest-circus/src/__tests__/tscongo.json is extending the root config. That’s why the reference is missing.

Copy link
Member

Choose a reason for hiding this comment

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

Reference is only so the build is in the correct order right? I don't understand why the test builds needs a specific order

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also thought so, but typecheck was not passing. Just removed references and now it is passing. Not sure why? ;D Let’s see what CI thinks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the error in CI. With references it goes away. I did not know that, just guessed that reference might work. Adding a .d.ts also works, but reference seemed to be cleaner.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

👍

@@ -3,5 +3,6 @@
"compilerOptions": {
"rootDir": "../"
},
"include": ["../**/*"]
"include": ["../**/*"],
"references": [{"path": "../../"}]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To typecheck test files, tsc runs with -b flag. Also it gets files in __tests__ and src directories still in TypeScript, not the compiled ones from build. It has to build them (in memory). Logically src has to be build first, so pointing references to packages/jest-circus/tsconfig.json makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

this makes more sense!

@SimenB SimenB merged commit e23c5c7 into jestjs:main Oct 6, 2022
@mrazauskas mrazauskas deleted the chore-typechecks-for-expect-utils-and-jest-circus branch October 6, 2022 09:14
@github-actions
Copy link

github-actions bot commented Nov 6, 2022

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants