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

Test suite doesn't tsc type-check #3411

Closed
IgorMinar opened this issue Mar 8, 2018 · 6 comments
Closed

Test suite doesn't tsc type-check #3411

IgorMinar opened this issue Mar 8, 2018 · 6 comments

Comments

@IgorMinar
Copy link
Contributor

The current test suite under spec/ is being transpiled to JS and executed, but no type-checking is currently being enforced. As a result of that, lots of typing errors have been introduced into the test suite.

Repro:

yarn tsc -p spec/tsconfig.json

Expected: no type errors
Actual: lots and lots of errors

Once this problem is resolved, the tsc typecheck should be added to the CI suite so that we can avoid introduction of new errors in future commits.

@kwonoj
Copy link
Member

kwonoj commented Mar 8, 2018

@david-driscoll is working on this. #3329 . I've accidentally closed #3020 .

@IgorMinar
Copy link
Contributor Author

cool. let's keep this issue open or reopen 3020 and close this as dupe. thanks!

@kwonoj
Copy link
Member

kwonoj commented Mar 8, 2018

I think we can use this one until it's resolved, #3020 doesn't have additional detail specifically worth to reopen.

@cartant
Copy link
Collaborator

cartant commented May 10, 2018

I can't cope any longer with the test suite's not being type checked. I see that #3329 has been closed:

... in favor of a more incremental approach.

I'm going to start work on an incremental approach now. Unless anyone has objections, I plan on doing the following:

  • Create a temporary spec/tsconfig.check.json file.
  • Specify an include in said file so that files can be incrementally checked and fixed. E.g. 'include': ['./operators/a*.ts']
  • Create a temporary script named test_check (or whatever) in the package.json to run a no-emit tsc build that uses spec/tsconfig.check.json.
  • Include npm test_check in the .travis.yml configuration file so that the CI build runs it.
  • Incrementally update the -spec.ts files so that they pass the check. That is, update them with individual PRs or, at least, in very small batches.
  • When all -spec.ts files pass, remove the temporary scripts from the package.json and the .travis.yml files, delete the spec/tsconfig.check.json file and set the TS_NODE_TYPE_CHECK environment variable in the test script so that ts-node will throw an error if TypeScript errors are introduced into the tests.

Once the test suite is again type checked, I'd like to extend the type tests to include snippet-based testing, as discussed here and as done here.

@david-driscoll
Copy link
Member

👍 Let me know if you have any questions. I had worked in a bunch of changes before there were some large refactors.

Interestingly most of the required changes to fix the type checking (and enhance the typings themselves) usually boiled down to some simply regex replace with a little bit of manual clean up.

@cartant
Copy link
Collaborator

cartant commented Jun 8, 2018

It seems Microsoft have come through with the goods and built a tool for testing type expectations and errors. This looks like it will be pretty useful: Microsoft/dtslint

I intend to look into integrating this once the test suite is type checked.

Testing Types: An Introduction to dtslint

cartant added a commit to cartant/rxjs that referenced this issue Jun 10, 2018
And remove temporary scripts and config, etc.

Closes ReactiveX#3411
@lock lock bot locked as resolved and limited conversation to collaborators Jul 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants