Ember smoke test should be cleaned up

[mike-north]

Originally posted by @BryanCrotaz https://github.com/DefinitelyTyped/DefinitelyTyped/issues/29119

---

Comes out of https://github.com/DefinitelyTyped/DefinitelyTyped/issues/29079

The smoke test that checks module stuff is imported into Ember global requires a clean up.

If it is a pure smoke test that the correct exports have been declared, then clearly it doesn't need 100% code coverage of the module code. So a single call into each class or function would be enough to check that the export is present. At present some calls are made with multiple overload patterns, and some are not made at all. We should either test the minimum or the maximum - as this is a smoke test it should just test the minimum, otherwise other contributors will be tempted (as I was in my first review) to fill the gaps.

Checking that the correct export mapping has been made should be possible by checking the type of the new object for example.

Some/most test calls use $ExpectType to confirm the result type. But many don't and if we're going to do it at all we should do it everywhere.

Tasks:

• [ ] clean up unnecessary overloads
• [ ] add $ExpectType to every call

[BryanCrotaz]

#confused Why is this relevant to e-c-typescript? Is there another smoke test here?

[mike-north]

@BryanCrotaz we track issues relating to @types/ember here, because (among other reasons) we have no ability to assign/label/organize issues on DefinitelyTyped

[mike-north]

@BryanCrotaz I'll reply to your question here

ahh I see.

Do you agree with my thought that we should remove overloads?

The goal of this particular smoke test is to protect against regression on the import Ember from 'ember'; namespace. As we start to move the actual types themselves to respective @ember/* packages (these are packages like @types/ember__object), we'll want to use this test to ensure that we don't acidentally forget to re-export things through @types/ember.

In the spirit of this, we should be testing that the appropriate thing is present on Ember, and that it "at a glance" seems to be the appropriate type. In particular, common things we'll want to detect

Example case: Ember.MutableArray

1. Is there Ember.MutableArray at all?
2. "at a glance", is it capable of doing the things that MutableArray should be able to do?
3. "at a glance", should it refuse to do things that MutableArray shouldn't be able to do?

Testing TS ambient types is unfortunately, a little tricker than testing other types of TS or JS code. For example, // $ExpectType literally just stringifies the name of the type and lets us assert against this string. This is why I pushed back a little bit against you wanting something like this

Ember.Object.extend(Ember.Evented, { foo: 'bar' }); // $ExpectType ?

The type of this thing will be

Readonly<typeof Object> & (new (properties?: object | undefined) => Evented & { foo: string; } & Object) & (new (...args: any[]) => Evented & { foo: string; } & Object)

Bonkers, right? This has to do with the way .extend() is defined, and the way we have things set up to support ES5 and ES6 flavors of ember.

We could try something like this

const: MyObject = Ember.Object.extend(Ember.Evented, { foo: 'bar' });

but if some change causes the return value of .extend to be an any, this test will PASS despite the fact that we've lost all type information here.

Sometimes testing an overload is more palatable than the above methods, for validating goals (2) and (3) above. It's really something that has to be judged on a case-by-case basis.

[BryanCrotaz]

I see your pain...

is there an equivalent of ExpectType that can assert that a call does not return any?

[mike-north]

is there an equivalent of ExpectType that can assert that a call does not return any?

Not that I'm aware of. any is type-equivalent to anything, so an assignment inherently can't catch this.

[BryanCrotaz]

Am I right in thinking that TS is all about duck typing, so it's impossible to say what the declared return type was, only to be able to ask questions about the object you got?

[mike-north]

TS can often infer a return type, or you can explicitly provide one. For example

const foo = () => 'hello';
function bar(): string {
 // something
}

ReturnType<typeof foo> // string
ReturnType<typeof bar> // string

these functions both have a return type of string. Depending on your tsconfig.json, you can allow a high or low degree of flexibility around these kinds of things. Ambient type declarations like @types/ember generally are written with a high level of strictness, but consuming apps may be far more lax.

[BryanCrotaz]

these functions both have a return type of string

But we can't test that the function declares a return type of string, only that we happen to get one back. I meant in terms of checking that no one has changed the return type to any.

[mike-north]

We have three options

1. We can write a test based on the question of type equivalence (i.e., "Is the return type of this function assignable to the following type"). This, of course, allows for return types of any that are assignable to anything.

2. We can write a test that asserts an exact string match on the return type of something ( // $ExpectType ). This can be prohibitively brittle, in that some of our types are very complicated in fully-expanded form.

3. We can assert that an error is thrown // $ExpectError. This is the most effective way to ensure that something isn't an any (i.e., we want a value to be a string, the compiler should error if we attempt to treat it like a number).

[BryanCrotaz]

We can assert that an error is thrown

sounds like the best way. Does this fit into smoke tests, or should this be in the detailed test?

[BryanCrotaz]

We could even define a type for testing this

type NotAny = { ____thisIsCompatibleOnlyWithAny: true }; //defined globally somewhere
let test:NotAny = myObj.doSomethingThatShouldReturnComplexThing(); // $ExpectError

Nope. Duck typing. Argh - does this plant an idea in your head for how to make this neat?

Interesting - this should fail at compile time, but not at run time...

[mike-north]

Yeah this is one of the strategies we already apply quite broadly in the deeper tests. There's a cleaner way to do it though

type NotAny = { ____thisIsCompatibleOnlyWithAny: true }; //defined globally somewhere


const myObj = {
    doSomethingThatShouldReturnComplexThing() { return [1, 2, 3]; }
}

myObj.doSomethingThatShouldReturnComplexThing() as NotAny; // $ExpectError

https://www.typescriptlang.org/play/#src=type%20NotAny%20%3D%20%7B%20____thisIsCompatibleOnlyWithAny%3A%20true%20%7D%3B%20%2F%2Fdefined%20globally%20somewhere%0D%0A%0D%0A%0D%0Aconst%20myObj%20%3D%20%7B%0D%0A%20%20%20%20doSomethingThatShouldReturnComplexThing()%20%7B%20return%20%5B1%2C%202%2C%203%5D%3B%20%7D%0D%0A%7D%0D%0A%0D%0AmyObj.doSomethingThatShouldReturnComplexThing()%20as%20NotAny%3B%20%2F%2F%20%24ExpectError%0D%0A

[chriskrycho]

Resolved by using expect-type to write much more robust type tests in Ember’s own published types.