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

chore: typecheck example and test files #13353

Merged
merged 7 commits into from
Oct 1, 2022
Merged

chore: typecheck example and test files #13353

merged 7 commits into from
Oct 1, 2022

Conversation

mrazauskas
Copy link
Contributor

@mrazauskas mrazauskas commented Oct 1, 2022

Summary

It would be nice to typecheck test files in CI. To make sure the test stay up to date and to have more tests for Jest types. For the start these are test files of three packages with types fixed and typecheck set up.

CI runs on the examples folder too, so I added type checks for examples written in TypeScript.

Test plan

If everything is setup correctly, tsc should find two type errors. (Which I will fix, of course.) All works as expected.

packages/babel-jest/src/__tests__/index.ts Outdated Show resolved Hide resolved
Comment on lines -23 to +26
const defaultBabelJestTransformer = babelJest.createTransformer(null);
const defaultBabelJestTransformer = babelJest.createTransformer();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

null is not allowed, but could be undefined. Perhaps it is fine to call it with no args:

https://github.com/facebook/jest/blob/eafabf99e64ac03f0af40ca26f24ebbb0f401c77/packages/babel-jest/src/index.ts#L151-L155

Comment on lines +18 to +19
beforeEach(async () => {
await TestBed.configureTestingModule({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Import of beforeEach was missing. After adding it, the test is failing. This change fixed the test. Reference: https://angular.io/guide/testing-components-scenarios#consolidated-setup

@mrazauskas mrazauskas marked this pull request as ready for review October 1, 2022 11:49
@mrazauskas
Copy link
Contributor Author

Looks like all is working.

@@ -50,6 +50,10 @@ jobs:
run: yarn test-ts --selectProjects type-tests
- name: verify [email protected] compatibility
run: yarn verify-old-ts
- name: typecheck examples
Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first I though to run these before type tests. The workflow is stopped after first fail. Feels like feedback from type tests could be more useful. Actually that is not so important, because it is possible to run any of these checks locally.

@@ -4,6 +4,7 @@
"composite": false,
"emitDeclarationOnly": false,
"noEmit": true,
"skipLibCheck": true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very good idea to keep config in a single root file ;D

@@ -110,6 +110,9 @@
"test-ts": "yarn jest --config jest.config.ts.mjs",
"test-types": "yarn test-ts --selectProjects type-tests",
"test": "yarn lint && yarn jest",
"typecheck": "yarn typecheck:examples && yarn typecheck:tests",
"typecheck:examples": "tsc -p examples/angular --noEmit && tsc -p examples/expect-extend --noEmit && tsc -p examples/typescript --noEmit",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using -p here, because --noEmit is not allow with -b. And it does not look right to have noEmit in example configs.

Copy link
Member

Choose a reason for hiding this comment

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

Can we use -b instead and pass all projects at once?

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 -b something --noEmit it is not allow to have --noEmit. Or you mean I include add noEmit: true to tsconfigs of examples? It felt strange to have it there, because that is not what one would do in real world.

Comment on lines +65 to +68
expect((result as BabelFileResult).map!.sources).toEqual(['dummy_path.js']);
expect(
JSON.stringify((result as BabelFileResult).map!.sourcesContent),
).toMatch('customMultiply');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah.. BabelFileResult is maybe even better since this is babel-jest test. The value and type comes from Babel:

https://github.com/facebook/jest/blob/eafabf99e64ac03f0af40ca26f24ebbb0f401c77/packages/babel-jest/src/index.ts#L240-L247

Copy link
Member

Choose a reason for hiding this comment

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

Perfect 👍

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.

Yay

@SimenB SimenB merged commit 8759d63 into jestjs:main Oct 1, 2022
@mrazauskas mrazauskas deleted the chore-typecheck-examples-test-files branch October 1, 2022 15:38
@github-actions
Copy link

github-actions bot commented Nov 1, 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 1, 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