Add test for type definition validation?

[kwonoj]

There are some cases of type definition does not work as expected like reported issue #660, #642, #635, Came to feel it'd be helpful if there's compile-time type signature validation test is included CI.

It'll not be kind of unit test, simply write down basic signatures of operator invocation in typescript instead of plain javascript, let CI try to compile it only (does not execute). Compilation result will tell if current type signature works in expected way or not. Course it can't cover whole aspect of signature, but at least can have minimum lines of basic usages.

[benlesh]

It sounds cool... @robwormald @jeffbcross what do you think about this? Do you guys do anything similar for Angular?

[robwormald]

Yeah, I think this is a great idea. It's a little tricky to test, but the original issue that I posted cropped up by doing something similar:

class Response {
  constructor(private _text:string){}
  toJson():Object { return JSON.stringify(this._text); }
}

let o = new Observable(obs => {
  obs.next(new Response('{"foo":"bar"}'));
  obs.complete();
});

obs.map(res => res.toJson()).subscribe() //complains - {} does not have a .toJson() method!

should be just a matter of setting up TS to compile some basic usage code with some typed classes. Would be happy to contribute here if you can get the mechanics set up @kwonoj

[robwormald]

In angular2, our tests are written in TS and so such issues get caught when the tests are being compiled.

[kwonoj]

As @robwormald mentioned, main difference between ng2 is RxJS test cases are written in JS.

I'll try to spend some times to think about ergonomics setup for this and create PR for proposal.

[kwonoj]

This could be achieved moving current test cases into typescript based one as same as Angular2 does, if circumstances allows.

[kwonoj]

I'm assigning this myself for evaluating if current test cases can be moved into typescript based one.

[kwonoj]

Currently there are 3 (including this) way proposed,

• convert unit test to typescript based, add additional test cases in existing tests (Proposal : migrate unit test to typescript  #826)
• separate test running for type validation only with custom runner using compiler api (Proposal for type validation test runner #667)
• separate test, running with compiler out of box (test(typings): Added tests for typings. #1189) by @david-driscoll

[benlesh]

@kwonoj, I'll defer to you on this one.

Personally, I kind of like the first option. But that might be to costly in terms of time/benefit.

[kwonoj]

Will take this, in discussion with @david-driscoll who has one of proposal's listed.

[david-driscoll]

I had the compiler only tests because it allows me to quickly validate if the type definition is clear and correct. I think they have value because they let you just compile out edge cases (expanding arguments until we variadic types for example).

[kwonoj]

@david-driscoll proposal's in here are mostly for those for now. (If you see other PR, those are also just validate compilation only as well). Might be expanded later, but not sure if it can be actually due to its nature.

[kwonoj]

@david-driscoll , let's narrow down discussion from 3 proposal to 2 proposal.

In core sense one of my proposal in #667 and yours #1189 is actually identical, by

• having separate typescript to only check compile time validity by using existing signature
• let compiler compiles it, spit out error if fails

The only difference is usage of compiler, while my PR uses compiler api (https://github.com/ReactiveX/RxJS/pull/667/files#diff-ab8a473f2a92e6517f9ada975e3ddb11R34) instead of let compile anything down, it gives small conveniences of not emitting compilation results per those test.

So in my mind, I'd like to discuss in this way

• convert unit test to typescript
  vs.
• using compiler api to have separate type-based test only.

(Let's think about ergonomics first to make actual changes happen)

Would it be ok? or strong preferences to use compiler instead of using its api?

[david-driscoll]

Nope! I just used the compiler because it was easier. 😄

[kwonoj]

Cool :) Let me come up with some pros vs. cons between those two then, discuss & decide from that point to bring this changes are checked in.

[kwonoj]

Now several type definition enhancements wanted to be ready before this are landed (or coming), think we can get back to this. As mentioned above, main thing to finalize is organization of test for type definition between using exist test case or having separate one.

My personal take on those are convert existing test case to typescript based one, then add type definition test as set of existing specs.

Pros

• single set of test spec gives usage of type definition as well as operator behavior
• basic type failure will be caught immediately while writing test cases (not even type tests)
  : test case becomes first dogfooding-consumer of code
• does not need additional setups

Cons

• need additional effort to convert existing specs into typescript
• test execution time will take bit longer by compilation

Though there are some cons, I think this is worth effort. I may possibly missing either pro or cons, so any opinion would be appreciated. I'll gather some opinion and try to proceed in one way.

/cc @david-driscoll , @masaeedu , @saneyuki also.

[david-driscoll]

I'm cool with whatever, and will help out where ever I can to get things working correctly.

[masaeedu]

@kwonoj Adding tests for the type signatures themselves seems a bit overkill to me, but I would definitely be for migrating the unit tests to use TypeScript. This way you'll be able to quickly spot pain points TypeScript users are facing when writing tests.

[david-driscoll]

The only reason I like having compiler based tests for the signatures, is it lets you see what happens in complex cases with overloads. In some cases, due to how TypeScript manages overload resolution order of your overload can mean the difference between the overload being found or not. Some operators this won't be necessary, as their tests would do the same thing. Other operators with more overloads, like zip, combineLatest, then it might make sense.

[kwonoj]

Updating status to notify this issue is not dead. I'm currently have work in progress changes for initial PR to migrate test cases into typescript to support type validation test as well, expecting to present it soon. Once this PR can be landed in codebase, quite lot of further tasks can be progressed based on this.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.