Migrate type tests to TSTyche #1988
Conversation
| "test": "run-s format lint type-check build unit-test", | ||
| "test": "run-s format lint type-check build test:*", | ||
| "test:unit": "jest", | ||
| "test:types": "tstyche", |
There was a problem hiding this comment.
I added test:types script to make it easy to pass extra options. For example, in CI the command could be test:types --target 4.5,5.0,current. Just a suggestion, it can be set up somehow else.
Currently type-check script is used. In early stages I saw TSTyche as a type checker (this is where the name is coming from), but it does more. It uses TypeScript to perform static analysis and file are not executed, but with .only / .skip and other features it feels more like a type test runner.
There was a problem hiding this comment.
Actually, we do have this file telling that the minimum version is 4.5, and I think that we do only test 4.5 (not really sure about that, we might use the local typescript version).
It might be interesting to test multiple typescript version in CI (but not done actually, so if it can be do, it's great, but not a necessity).
Locally, I think that testing the local version of typescript is good enough.
There was a problem hiding this comment.
Right. I forgot to mention that current is currently (or locally) installed TypeScript. This is the default. I mean, tstyche is the same as tstyche --target current.
| @@ -65,7 +65,8 @@ jobs: | |||
| restore-keys: ${{ runner.OS }}-node- | |||
| - run: npm ci | |||
| - run: npm run build | |||
| - run: npm run unit-test | |||
| - run: npm run test:unit | |||
| - run: npm run test:types -- --target 4.5,5.0,current | |||
There was a problem hiding this comment.
@jdeniau What you think? Locally test:types would test only on the installed TypeScript. The lowest supported version (4.5, resolves to 4.5.5) is checked on CI. I would definitely recommend adding 5.0 (resolves to 5.0.2), because each major brings in breaking changes. And currently installed.
latest could be added. It points to typescript@latest in NPM registry, hence it is a changing target. It might start failing with any unrelated PR, i.e. even without changes in types. That’s somewhat unpredictable.
There was a problem hiding this comment.
latest (or even next) could work as a daily cron job. Just some food for though.
| // $ExpectType Set<unknown> | ||
| Seq.Set(); | ||
| expect(Seq.Set<string>()).type.toEqual<Seq.Set<string>>(); |
There was a problem hiding this comment.
Interestingly dtslint does not see difference between Set and Seq.Set types. Probably this is how it works. I did not know this detail. Learned something new.
| // $ExpectType Map<string, number> | ||
| mapImmutable.delete('foo'); |
There was a problem hiding this comment.
Here this is ImmutableMap not Map. Seems like dtslint is not aware of the context.
There was a problem hiding this comment.
If I understood dtslint, it does compare the string output, so like it is in the immutable.d.ts file, so Map and not ImmutableMap
There was a problem hiding this comment.
Yes. It works correctly, only looks strange.
|
Two more smaller files are added. Sorry, I just couldn’t stop (; How is it better: to push all tests here? or, to wait until this PR will be merged? |
If you are on fire, just push it here, and tell me when you want me to review 😄 The only thing is I need to manually trigger a "run" because of the project configuration, so I'll do it when I can, but you might be faster than me. |
|
Would be nicer, if CI could run automatically. All checks are green at the moment. Let’s merge this one, if all looks good for you. I see that in larger files I will have some questions. Would be easier to go through these in separate PRs. |
|
Merging this as everything is OK and the transition is promising |
|
Wonderful! Thanks for quick review. Let's iterate. I will open the next PR tomorrow. |
Reference #1985 (comment)
This PR migrates two type test files to TSTyche. The setup is painless and migration can be done gradually. Hope this makes it easier to review.
The test files were not passed through Prettier before (probably because of some conflict).
Good question: how CI checks should be setup? To test on different versions of TypeScript, I used TSTyche's
targetoption. This makes local runs noisy. It would be enough to havetest:types --target 4.5,5.0,currentin CI. @jdeniau what you think?