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

chore: migrate type tests to TSTyche #894

Closed
wants to merge 15 commits into from

Conversation

mrazauskas
Copy link
Contributor

@mrazauskas mrazauskas commented Nov 20, 2023

Some time ago I recommended to use jest-runner-tsd, which is using forked tsd-lite to test types (#731). I am maintaining the latter package, tsd-lite. The bad news is that it is being sunset, see Deprecation notice.

The good news: recently I published TSTyche, a mighty type testing tool. I was developing it for two years. TSTyche is using the same TypeScript's type checker to compare types. The rest is different.

Repo: https://github.com/tstyche/tstyche
Documentation: https://tstyche.org


This PR is a proposal to. migrate to TSTyche. I think you will like it.

Testing on Specific TypeScript Versions

TSTyche can test on several versions of Typescript: tstyche --target 4.9,5.0,latest. This simple.

Assertions

It ships expect style assertions. Two types or two inferred types can be compared:

import { expect } from "tstyche";
 
expect<string | undefined>().type.toEqual<string | undefined>();
 
expect('abc').type.totoEqual('def');

Performance

It is faster.

.github/CONTRIBUTING.md Outdated Show resolved Hide resolved
} else {
// this type is currently wrong, typescript cannot remove the case because the input is not restricted enough
expectAssignable<unknown>(someNewDoc);
expect<unknown>().type.toBeAssignable(someNewDoc);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don’t know what type is expected, but you could use expect.fail() here.

} else {
// this type is currently wrong, typescript cannot remove the case because the input is not restricted enough
expectAssignable<unknown>(someFoundDoc);
expect<unknown>().type.toBeAssignable(someFoundDoc);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also good candidate for expect.fail()

test/tests/types/basicTypegoose.test-d.ts Show resolved Hide resolved
@@ -405,7 +411,7 @@ function testFilterFunctionsType() {
public refArr?: typegoose.Ref<Nested>[];
}

expectType<{
expect<Pick<Root, keyof Root>>().type.toEqual<{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These can simply compared as two types, right?

Copy link
Member

Choose a reason for hiding this comment

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

if i see the context correctly, i think i made it explicitly as another type (not using Root) to know if something changed without me knowing (like in mongoose)

@mrazauskas
Copy link
Contributor Author

mrazauskas commented Nov 21, 2023

Forgot to mention:

  • TSTyche has test() and describe(). I think those can be useful in this case;
  • Node.js 18.0 is the lowest supported version. Hope that is fine.

Copy link
Member

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

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

Thanks for reaching out about the deprecation and a path forward (and a migration PR), but for now i cannot adopt tsyche because of the following reasons:

  • tstyche's minimal nodejs is 18.0, but typegoose 11.x is 14.17 (which this PR is basing on) and typegoose 12.x (in the beta branch)'s minimal is 16.20, so it will likely take a while before being able to transition (minimal nodejs matches what mongoose supports)
  • there are currently no problems using tsd-lite / jest-runner-tsd

the only annoyance / problem i have / had with tsd-lite is that expectError will error in vscode - but the error is actually expected so it shouldnt error, does tstyche change this in some way? (i have not yet tested if adding // @ts-ignore / // @ts-expect-error solves this while still having a working test just saw mrazauskas/tsd-lite#48)

also some other questions:

  • does tstyche support just running it against the currently installed typescript version without explicitly defining it (as defined in the package.json)?
  • does tstyche have some kind of jest integration (like tsd-lite)?
  • tstyche seems to be in pre-release, are any breaking changes expected before full release?

@@ -405,7 +411,7 @@ function testFilterFunctionsType() {
public refArr?: typegoose.Ref<Nested>[];
}

expectType<{
expect<Pick<Root, keyof Root>>().type.toEqual<{
Copy link
Member

Choose a reason for hiding this comment

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

if i see the context correctly, i think i made it explicitly as another type (not using Root) to know if something changed without me knowing (like in mongoose)

@hasezoey hasezoey added wont fix (currently) This will not be worked on in the near-future tests This Issue is about tests | This PR is modifying tests labels Nov 24, 2023
@mrazauskas
Copy link
Contributor Author

Thanks for detailed feedback.

Below is CI output. I will use it as a reference to explain few details:

Screenshot 2023-11-24 at 18 22 17

Integration

You are right, there are no problems with tsd-lite. Only some annoyances. For instance, tsd has only assertions, but Jest cannot report count of assertions. Therefor jest-runner-tsd reports those as tests. Plain wrong, but it works.

TSTyche has a concept of test and assertion. In the output above only count of assertions is reported, because file has no test(). Integration with Jest would mean there is no way to report count of assertions. Other problem, Jest is passing test file paths one by one and expects status to be reported for each file. The above holds three status reports for the same file, but Jest would expect only one.

Not to speak that passing --targets to Jest would simply raise validation error. In the very beginning TSTyche was a Jest runner and it was much harder for me to build it as a standalone CLI tool, but this solved all what I mentioned here. I don’t see how integration could work keeping all functionality TSTyche has today.

Pre-release

I left only minimal amount of features for pre-release. Some minor breakages can happen, but nothing dramatic. The lowest supported Node.js version is the most dramatic question I have to solve.

Currently Installed TypeScript Version

By default, calling tstyche is equivalent to tstyche --target latest. Installed version is not used for type testing. TSTyche has to install TypeScript and to apply a tiny patch. The result is similar to @tsd/typescript, but is it created on demand. This is how different version can be installed and loaded for testing. (By the way, the installed typescript packages are not kept in node_modules. So patching is no harm at all.)

I was thinking, which version should be pick up by tstyche? Because TSTyche knows how to install TypeScript, it can be used in repos without TypeScript installed. It could still look at package.json and fallback to latest. The tricky part is that tags in package.json is in semver syntax, but --target does not use semver tags. Other problem is monorepos: running tstyche from root and from some package folder could end up in different result if TypeScript version differ in package.json.

Nice thing about latest is that it keeps updating itself (because it is the latest from npm, of course). It might be tricky as well (that is why TypeScript version is always printed in the report), but I like that TSTyche is proactive. For example, there are times I can not upgrade TypeScript for some reason, but it does not mean users of my library did not upgrade.

These are the reasons why tstyche --target latest is the default. I was thinking to add --target current, which would test on currently installed version. Not sure if naming it current is clear enough for user. Sounds very mush like current version on npm.

Thanks for reading this lengthy text. And thanks for giving TSTyche a try.

@hasezoey
Copy link
Member

thanks for the response, i now understand that it currently does not use the current installed typescript version

for me i only ever support a specific typescript version, because supporting multiple is a real pain and may not even work (example is typescript 4.9 which completely broke a specific functions type narrowing requiring the types to be completely rewritten for that function (isDocument and derivatives))

but i guess it is good to know if the current types will break in a future (not officially supported version) typescript version

@mrazauskas mrazauskas mentioned this pull request Nov 25, 2023
@mrazauskas
Copy link
Contributor Author

I had time to think a bit. It makes sense to down level the Node.js requirement. TSTyche has zero dependencies and this should be its strength. Depending on APIs like fetch() is somewhat similar to having a dependency. It should be rather easy to have support for Node.js 14.

Perhaps let's leave this PR open for now?

Thanks again for you feedback!

@hasezoey
Copy link
Member

Perhaps let's leave this PR open for now?

i will leave it open, though it may get marked "stale" at some point

TSTyche has zero dependencies and this should be its strength
It should be rather easy to have support for Node.js 14.

well it has to handle typescript, which has its own engine requirement (like typescript 5.1 requires nodejs 14.17)

Copy link

codecov bot commented Nov 25, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.65%. Comparing base (bab4f92) to head (2eaa421).
Report is 38 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #894   +/-   ##
=======================================
  Coverage   98.65%   98.65%           
=======================================
  Files          17       17           
  Lines         967      967           
  Branches      261      261           
=======================================
  Hits          954      954           
  Misses         13       13           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mrazauskas
Copy link
Contributor Author

@hasezoey Just to draw your attention, I upgraded TSTyche to the latest beta.

It has a new current target, which checks the version of the installed typescript package and uses the same version for type testing. (The version is checked via require("typescript").version, there is no need to resolve semver or so.)

The lowest supported is now Node.js ^16.14.

Also note that replacing expectError() with the .not.toBeAssignable() matcher (as suggested in this PR) makes it so that nothing will be mark red in VS Code.


Of course, there is no rush. I am opening the migration PRs to gather feedback and I try to listen to the users. Targeting the current version was requested in another migration PR as well. Sounded like a good idea.

@github-actions github-actions bot added the stale This Issue | PR had no activity in a while and will be closed if it stays so label Dec 28, 2023
@github-actions github-actions bot closed this Jan 4, 2024
@hasezoey hasezoey removed the stale This Issue | PR had no activity in a while and will be closed if it stays so label Jan 4, 2024
@hasezoey
Copy link
Member

hasezoey commented Jan 4, 2024

didnt mean to close this issue, have not noticed the stale tag (wheres the message?)

@hasezoey hasezoey reopened this Jan 4, 2024
@mrazauskas
Copy link
Contributor Author

mrazauskas commented Jan 5, 2024

Something new: in the latest beta of TSTyche current (the installed Typescript) is the default target.

First I made it possible to load the installed typescript package for type testing. Next step was changing the default to get the best possible performance. There is no need to download TypeScript anymore, that is obviously big improvement for the first run (cold cache). Since test always run with cold cache in CI, that is the best performance for CI too. Benchmarking confirms this is true (I have seen up to 1/4 faster runs in CI).

UPDATE I checked older runs in this repo. Now we have 3s instead of 4.1s earlier.

@mrazauskas
Copy link
Contributor Author

Trying out freshly released tstyche@1.0.0-rc. No more breaking changes.

A stable release is planned to be published in 2 weeks.

@mrazauskas
Copy link
Contributor Author

TSTyche 1.0.0 is published. I have nothing more to add here (;

@github-actions github-actions bot added stale This Issue | PR had no activity in a while and will be closed if it stays so and removed stale This Issue | PR had no activity in a while and will be closed if it stays so labels Mar 22, 2024
@github-actions github-actions bot added the stale This Issue | PR had no activity in a while and will be closed if it stays so label Apr 22, 2024
@github-actions github-actions bot closed this Apr 29, 2024
@mrazauskas mrazauskas deleted the migrate-to-tstyche branch April 29, 2024 04:20
@hasezoey
Copy link
Member

i didnt intend it to get closed (i thought it would add a comment again, and forgot to add a tag), though i also didnt intend to immediately merge it soon - sorry about that

@mrazauskas
Copy link
Contributor Author

Got it. Thanks for clarifying.

Many matchers will be renamed in TSTyche 2. So better I will put together a fresh PR after 2.0.0-rc will be out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale This Issue | PR had no activity in a while and will be closed if it stays so tests This Issue is about tests | This PR is modifying tests wont fix (currently) This will not be worked on in the near-future
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants