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

fix: use tsd instead of forked mlh-tsd #25

Closed
wants to merge 1 commit into from
Closed

fix: use tsd instead of forked mlh-tsd #25

wants to merge 1 commit into from

Conversation

mrazauskas
Copy link
Contributor

@mrazauskas mrazauskas commented Oct 19, 2021

What if there is no need to patch tsd? I was trying to improve error message printed by this library and found something interesting.

(Long text. I added summary at the bottom.)

Here is a valid tsd test (see docs):

import concat from '.';

concat('foo', 'bar');
concat(1, 2);
concat([1], [2]); // this and following line will fail the test
concat({a: 1, b: 2});

This test does not include assertions (any of expectType, expectError, etc). Running tsd works as expected (two errors reported), but jest-runner-tsd prints buggy result (no tests were running, but two failed):

Screenshot 2021-10-19 at 14 05 26

The same, but with failing lines removed (no tests were running at all):

Screenshot 2021-10-19 at 14 07 47

In a way, the runner does not work in the same way as tsd. What is happening? tsd is using TS compiler to get diagnostics. This is first source of test errors. Next the list of errors is tweaked by passing it through filters of tsd assertions (for instance, expectError is silencing errors). The assertions is the second source of test errors.

The patched version of tsd is counting only the tsd assertions, but not the work done by TS compiler. The number of tsd assertions is reported as the total count of tests. In the examples above we see 0 total, because test file has no assertions. Hence, to report the number of assertions as the number of tests is not accurate.

Even more – tsd does not have a concept of test. I was trying to improve the error message printed by the runner. It would be nice to pass a test title to Jest, but there are no titles. As well as there are no tests in tsd.

In general tsd works rather similar to tsc --noEmit. It simply checks TypeScript type definitions (or .d.ts files) by parsing test files (nothing is executed). I think it is better to count test files than assertions. This is very similar to jest-runner-eslint.

It seems to me that tsd is not a testing utility, but a typechecker. So it should be treated differently than the current implementation.

Apologies for this long text. Hope it explains the problem and motivation. Thanks for reading.

Summary

  • I propose to use tsd instead of forked mlh-tsd
  • mlh-tsd fork is adding just one tiny feature (which made me to write all this text)
  • mlh-tsd returns a count of assertions (as a count of tests) together with the diagnostics list
  • tsd is a typechecker and it does not have the concept of a test, hence the implementation of that feature is questionable
  • one: semantics are broken (assertions are not tests)
  • two: see the bug mentioned above

"typescript": "^4.4.4"
},
"peerDependencies": {
"tsd": ">=0.15.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It felt like peerDependencies is good idea. v0.15 is the minimal version with necessary API.

@mrazauskas mrazauskas marked this pull request as draft October 19, 2021 14:07
@mrazauskas mrazauskas marked this pull request as ready for review October 19, 2021 17:05
@mrazauskas mrazauskas changed the title fix: do not count tsd assertions, but instead report the number of test files fix: use tsd in favour of forked mlh-tsd Oct 19, 2021
@mrazauskas mrazauskas changed the title fix: use tsd in favour of forked mlh-tsd fix: use tsd instead of forked mlh-tsd Oct 20, 2021
@SimenB
Copy link
Member

SimenB commented Oct 22, 2021

I'm not ignoring this btw, I just haven't found the time to sit down and actually think about what I think makes the most sense 😅

@mrazauskas
Copy link
Contributor Author

Sure. This is also messy PR. It has too many ideas in one place. Few already got moved to separate PRs. Cleaning it up.

@mrazauskas
Copy link
Contributor Author

Cleaned it up. One focused change and one new test.

The only change is the count of passed / failed assertions. For instance, Jest’s test suite now prints:

Test Suites: 4 passed, 4 total
Tests: 283 passed, 283 total

If this PR would land:

Test Suites: 4 passed, 4 total
Tests: 4 passed, 4 total

The only lost information is the count of failures. tsd prints the list of errors and at the bottom adds 4 errors (in red). Perhaps this could be added to error title?

Drafting (not implemented yet). Here is how 2 errors in 1 failed test could look:

Screenshot 2021-10-22 at 15 02 04

For my eye, this is more accurate compared with current Tests: 2 failed, 0 total. Just thinking out loud. It helps (;

@mrazauskas
Copy link
Contributor Author

jestjs/jest#11986 is very interesting case. If the test runs on the main branch, the runner prints Tests: 1 failed, 0 total. What felt like an imaginary edge case suddenly is real thing.

I was trying to write the test somehow differently, but couldn’t make it work. That seem like the best solution. Funny situation.

Screenshot 2021-10-22 at 19 38 24

@mrazauskas
Copy link
Contributor Author

See #41

@mrazauskas mrazauskas closed this Jan 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants