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

Type error reporting #79

Closed
pelotom opened this issue Dec 6, 2016 · 22 comments
Closed

Type error reporting #79

pelotom opened this issue Dec 6, 2016 · 22 comments

Comments

@pelotom
Copy link
Contributor

pelotom commented Dec 6, 2016

Currently it seems that the preprocessor doesn't report type errors. Would it be possible to treat type errors in TS files (whether source or test files) as if they were test failures?

@kulshekhar
Copy link
Owner

@pelotom can you create a tiny repo that illustrates the issue and your expectations. It would help in quickly getting started on this

@gyzerok
Copy link

gyzerok commented Dec 6, 2016

@kulshekhar there is an example in linked issue. It do not use ts-jest however both this example and ts-jest have same problem. There are expectations defined as well.

@kulshekhar
Copy link
Owner

kulshekhar commented Dec 6, 2016

@pelotom @gyzerok

It doesn't seem that this can be fixed in ts-jest. When running tsc against example.test.ts from the repo, the corresponding example.test.js file is generated. This is what ts-jest receives and uses. The transpiler returns transpiled code even with noEmitOnError set to true.

If there were a way to get tsc the transpiler to abort and throw an exception on type errors, that would help. Without such an option, the only way to do this would be to launch an external process to transpile the source code and check for the exit code of the process. This would slow down tests and I'm not sure it's worth the returns given that any build step (not to forget the IDEs) will display these errors anyway.

I'm closing this for now but if you have alternative solutions or want to discuss this further, feel free to do so here. If tsc were to get such a feature, it might be worth reopening this issue.

@Igmat
Copy link
Contributor

Igmat commented Dec 6, 2016

Really, the main problem is that TS needs project compilation and not file compilation, in order to correctly show type errors.
We could try to find a workaround that compiles overall project instead of one file for Jest, but it definitely wouldn't be a preprocessor, because Jest API sends only one file to it.

@gyzerok
Copy link

gyzerok commented Dec 6, 2016

If we can somehow serialize compilation and just restore in subsequent calls to preprocessor. Can we cache compilation results within one run of Jest somehow?

@gyzerok
Copy link

gyzerok commented Dec 6, 2016

I have no time now to dig deeper but this looks like what we need.

@Igmat
Copy link
Contributor

Igmat commented Dec 6, 2016

@gyzerok, ok it's a good point. I'll try to dig into it later, but at first look it seems that we will have to inform Jest that it should wait for project compilation and then inform it each time when file changes instead of default Jest watcher. So it could be very difficult to achieve.

@pelotom
Copy link
Contributor Author

pelotom commented Dec 6, 2016

It might be instructive to look at how the ts-loader for webpack works, since it's solving a similar problem... it is using the TS language services to incrementally recompile files and display type errors, and it's adapted to fit the webpack (and also jest) model of servicing compilation requests for individual files.

@gyzerok
Copy link

gyzerok commented Dec 7, 2016

Should we reopen an issue since we might have possible solution?

@kulshekhar
Copy link
Owner

kulshekhar commented Dec 7, 2016

I haven't dug deep into this but this doesn't look like a workable solution at first sight.

The TS docs linked to and ts-loader both use file watchers to do incremental transpilation.

Since a preprocessor doesn't run in it's own process and is only invoked by jest on a file by file basis, the expectation is that the preprocessor would process the file given to it and return the result. I would personally like to limit the scope of ts-jest to that.

That said, if there's a PR that extends the capability of ts-jest without adding complexity to the core codebase and functionality (something like a plugin/extension), I'm open to it. (Although it would be somewhat amusing to see a plugin, ts-jest, have its own set of plugins)

The only reasonable solution I can see is if tsc starts throwing an exception (or uses some other mechanism to indicate a failure) when there's a type error.

All this aside, I'm also curious about why failing on a type error would be needed in a preprocessor. Shouldn't this be part of the build process where the build fails if there's a type error?

@Igmat
Copy link
Contributor

Igmat commented Dec 7, 2016

Ok, after small investigation I have some thoughts on how it could be achieved. It definitely wouldn't be a preprocessor itself, but we already have coverage processor in our package, so IMO it's not a problem to provide such functionality as sub-module of ts-jest.
Actually I can't say that it will be implemented (there are a lot of possible caveats), but I'll try.

@Igmat Igmat reopened this Dec 7, 2016
@kulshekhar
Copy link
Owner

@Igmat As I mentioned in the earlier comment, I can't see a valid reason for type checking to be the responsibility of a preprocessor. However, if you want to add it, go ahead. Just ensure that this doesn't affect the complexity and performance of the core functionality/code.

Bear in mind that it's hard to remove a feature (even a poorly performing one) once it has been added and other people depend on it.

@mikehaas763
Copy link

mikehaas763 commented Jan 4, 2018

All this aside, I'm also curious about why failing on a type error would be needed in a preprocessor. Shouldn't this be part of the build process where the build fails if there's a type error?

test code is still code, if there's type errors in such test code, shouldn't it fail?

@hjfreyer
Copy link

hjfreyer commented Feb 17, 2018

As mentioned in #250, it's unclear where else type error reporting could go when using jest --watch. In that case, the build process happens within jest, so somebody has to own it. Relying on IDE support is a real drag since everyone's got their own workflow, and type-aware editors aren't always an option.

@kulshekhar
Copy link
Owner

@hjfreyer does this help?. It was recently added

@hjfreyer
Copy link

Woot! Indeed it does. Thanks!

@lyy011lyy
Copy link
Contributor

@kulshekhar Thanks for the suggestion, but when I enable it, the test is really slow..... say 30sec for a single test...
Is there any other way to solve this?
Thanks!

@kulshekhar
Copy link
Owner

@lyy011lyy yes, by keeping type checks out of testing (that's what I do)

@Dean177
Copy link

Dean177 commented Aug 8, 2018

@lyy011lyy I use tsc --no-emit as one of my CI steps usually just before running any unit tests.
That way tests stay fast and you still get typechecking as part of your workflow.

@lyy011lyy
Copy link
Contributor

lyy011lyy commented Aug 13, 2018

Hi @Dean177 : Thanks for the suggestion and it works well locally.
But I want to add the typer errors info into the jUnit test result report which is used to fail the voter.
With this method, if there are type errors, there is no test result report will be generated.

I retried the "enableTsDiagnostics": true and found that seems like this will trigger the type checking for the whole imported dependency tree, which makes it slow. I was wondering that is there a way to make it only check the current test file?

Thanks

@lyy011lyy
Copy link
Contributor

Found the issue at

if (tsJestConfig.enableTsDiagnostics) {

I found that ts-jest is now checking the type errors for source codes and all their dependencies if "enableTsDiagnostics" is turned ON. This makes it very slow and I don't think that's what we want since the src code type errors will be detected during the build time. What we need should be a way to report the type errors inside the test files, because usually test code will be excluded when build.

It should be easy to check whether the filePath is inside one of the root's pathes to achieve that only check the type errors inside test code. I'm creating the pull request.

@PiotrOwsiak
Copy link

Hold on, this is actually one of the main features that ts-jest is advertising here https://kulshekhar.github.io/ts-jest/docs/
"It supports all features of TypeScript including type-checking. Read more about Babel7 + preset-typescript vs TypeScript (and ts-jest)."

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

No branches or pull requests

9 participants