create-react-app-typescript icon indicating copy to clipboard operation
create-react-app-typescript copied to clipboard

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

Open obsoke opened this issue 7 years ago • 4 comments

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?

obsoke avatar Apr 16 '18 17:04 obsoke

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.

wmonk avatar Apr 16 '18 17:04 wmonk

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.

DorianGrey avatar Apr 17 '18 06:04 DorianGrey

@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.

obsoke avatar Apr 18 '18 13:04 obsoke

Note that enableTsDiagnostics was added on [email protected]. react-scripts-ts is still on [email protected]. 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.

dosentmatter avatar May 18 '18 07:05 dosentmatter