Skip to content
This repository has been archived by the owner on Jan 26, 2019. It is now read-only.

Jest: Test cases with TypeScript errors report as 'pass' #306

Open
obsoke opened this issue Apr 16, 2018 · 4 comments
Open

Jest: Test cases with TypeScript errors report as 'pass' #306

obsoke opened this issue Apr 16, 2018 · 4 comments

Comments

@obsoke
Copy link

obsoke commented Apr 16, 2018

Firstly, thanks to all who contribute to this project -it saved me a lot of headache in getting started with TS + React!

I'm currently writing some smoke tests for React components that have required properties. Currently, I am not passing these required properties to the shallow renderer. I'm expecting these tests to fail with an error such as Property 'title' is missing in type '{ location: {}; }'. However, the test passes without issue. Is this expected behaviour? Because I kind of expect tests that contain a tsc error to fail.

I looked through the documentation for ts-jest and noticed the following option is available: enableTsDiagnostics. Setting this value to true will "enable Syntactic & Semantic TypeScript error reporting". However, due to the supported keys whitelist, I cannot enable this option without ejecting.

Personally, I think this option should be configurable without ejecting. I'm surprised this setting isn't enabled by default. My reasoning is that we shouldn't lose out on one of the biggest advantages of TypeScript (type checking) in our tests. Am I alone in thinking this? Is there a reason this option isn't enabled by default, or configurable at all?

@wmonk
Copy link
Owner

wmonk commented Apr 16, 2018

No reason, just wasn't something I was using when developing this module. Will accept a PR to enable this and any other TS related options.

@DorianGrey
Copy link
Collaborator

I'm surprised this setting isn't enabled by default

Because it affects performance drastically. E.g.: One of my projects currently contains 418 unit tests. With a clean cache, they take ~30s to execute. With diagnostics, this is raised up to ~270s. Not sure about the watch mode, though, but it might similarly bad.

Additionally, I was quite often facing problems regarding included or used types that are no problem during the webpack build, and even worked well on a "manual" build via tsc. I.e. this option might cause quite a lot of false positives.

In the current setup, the plugin we're using for type-checking and linting also tracks the test files, even though it does not react on changes to it.

It might be cheaper to use tsc directly for this purpose (maybe in a different terminal tab), just without emitting any files:

npx tsc -p tsconfig.test.json --noEmit -w

All open for a way to make this option configurable, though. Just keeping in mind that it might not be working without any glitches or problems.

@obsoke
Copy link
Author

obsoke commented Apr 18, 2018

@DorianGrey Thanks for the information. That does seem like a good reason to keep this option disabled by default. Running tsc wtih the --noEmit flag does seem like a cheaper & simpler way to test for this. However, I still think in some cases the option can be worth using.

@wmonk Nice! I'll try to whip up a PR allowing one to enable this option. Knowing that it comes with performance concerns, I'll continue to leave it disabled by default.

@dosentmatter
Copy link

dosentmatter commented May 18, 2018

Note that enableTsDiagnostics was added on ts-jest@22.0.3. react-scripts-ts is still on ts-jest@22.0.1. I was trying to use it and was wondering why it didn't work.

I don't know if we have enable this option ourselves because you can easily add in the enableTsDiagnostics by using react-app-rewired. What we can do is update the ts-jest version to >22.0.3 so users can use enableTsDiagnostics. react-app-rewired lets you modify the configs but I don't think there's any tool to update the dependencies of react-scripts.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants