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

Typings test #33

Closed
wants to merge 59 commits into from
Closed

Typings test #33

wants to merge 59 commits into from

Conversation

smartclash
Copy link
Member

@smartclash smartclash commented Aug 11, 2020

Typings tests for public facing API would be now written inside the test-types/ folder. It uses TSD and jest-runner-tsd.

We still haven't covered every API available. This is at it's start and we need to get more inputs on our approach. We'd like to know if

  • The way tests are split is okay or not?
  • The place where tests are placed is okay or not?

... and suggestions would be really good too :)

We are now using jest's projects to use the custom runner and test the types.

Builds are still failing because jest-runner-tsd is not published to NPM and can be published only after the PRs are merged in TSD.

TODOs:

  • Get reviews / suggestion.
  • Implement the suggestions.
  • Fix the ESLint issues.

Copy link

@jevakallio jevakallio left a comment

Choose a reason for hiding this comment

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

@smartclash, I left a couple of comments -- the main one being, that when we are testing types, it would be ideal not to be testing for any. Kind of defeats the point?

I understand that the TSD assertion tests do add value even when we don't test the return type, as it does test against the function parameters and reports any type errors to jest.

I'd ask Simen for feedback on this ASAP. In order to help him give good feedback, I'd improve the PR description explaining what you are asking feedback on -- overall approach, not individual tests. Also, I would explain that this is not full type coverage yet, and you intend to do that after the approach has been validated.

Personally, if you don't have anything else to do, I'd not wait any longer for feedback and proceed with writing tests for further APIs using this approach.

@smartclash
Copy link
Member Author

@smartclash, I left a couple of comments -- the main one being, that when we are testing types, it would be ideal not to be testing for any. Kind of defeats the point?

I understand that the TSD assertion tests do add value even when we don't test the return type, as it does test against the function parameters and reports any type errors to jest.

I'd ask Simen for feedback on this ASAP. In order to help him give good feedback, I'd improve the PR description explaining what you are asking feedback on -- overall approach, not individual tests. Also, I would explain that this is not full type coverage yet, and you intend to do that after the approach has been validated.

Personally, if you don't have anything else to do, I'd not wait any longer for feedback and proceed with writing tests for further APIs using this approach.

@jevakallio, the functions itself return any type. And so, we've also used any at tests too :)

@SaurabhAgarwala
Copy link
Member

@SimenB please review this PR and let us know if the approach we are following is good or requires any changes?

This PR is related to jestjs#8794

@SimenB
Copy link
Collaborator

SimenB commented Aug 12, 2020

Yeah, I like this approach! I'd like to see some tests for type errors tho - missing arguments and arguments with wrong type etc.

I wonder if we should just run this as part of the Node CI / Running TypeScript compiler & ESLint (pull_request) workflow and not all tests, but we can tweak that later.

package.json Outdated Show resolved Hide resolved
test-types/describe.test.ts Outdated Show resolved Hide resolved
@SaurabhAgarwala
Copy link
Member

Moved to #jestjs#10407.

Merged there. Closing this.

@github-actions
Copy link

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 May 14, 2021
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.

5 participants