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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to use sinon types? #36019

Closed
nicojs opened this issue Jun 7, 2019 · 20 comments
Closed

How to use sinon types? #36019

nicojs opened this issue Jun 7, 2019 · 20 comments

Comments

@nicojs
Copy link
Contributor

nicojs commented Jun 7, 2019

Authors: @MrBigDog2U, @rationull, @lumaxis, @43081j, @JoshuaKGoldberg, @gjednaszewski, @johnjesse, @alecf, @SimonSchick, @bergundy

Hi fellow sinon type authors 馃憢 . Sorry for the inconvenient cry for attention (it always comes at a bad time, feel free to ignore).

I've updated to the latest version @types/sinon yesterday. I needed to add generic classes to my stubs like so:

image

My question is: am I using the sinon types correctly? I don't feel like these typings improved the maintainability of my code. As soon as a method changes its signature I will need to update the mocks in all classes. That doesn't feel helpful. Am I doing something wrong here?

The problem is that with mocha we declare stubs before we initialize them (in beforeEach). Is there a way around it that I'm unaware of? Or is everyone typing all these generic types by hand?

Thanks in advance for any help.

@43081j
Copy link
Contributor

43081j commented Jun 7, 2019

the aim isn't to let you write less, its to have stronger types. those stubs will now have the correct type information with all the assertion methods correctly typed (e.g. returns will only accept what you said the method returns).

having to be a bit more verbose is the cost of declaring all your variables before they are assigned to, its not a problem and is making good use of the types still.

Technically you can declare them as sinon.SinonStub; (defaults to any[]) but you will lose type information so its best avoided

@nicojs
Copy link
Contributor Author

nicojs commented Jun 7, 2019

I've found a way around having to manually type the stubs using a small helper class:

    class TestHelper {
        existsSyncStub = sinon.stub(fs, 'existsSync');
        streamStub = {
            pipe: sinon.stub(),
            on: sinon.stub()
        };
        streamToPromiseStub = sinon.stub(Utils, 'streamToPromise');
        gotStreamStub = sinon.stub(got, 'stream');
        mkdirSyncStub = sinon.stub(fs, 'mkdirSync');
        statSyncStub = sinon.stub(fs, 'statSync');
        createWriteStream = sinon.stub(fs, 'createWriteStream');

        constructor() {
            this.gotStreamStub.returns(this.streamStub as any);
            this.streamStub.pipe.returns('expected piped value');
        }
    }
    let testHelper: TestHelper;

    beforeEach(() => {
        log = sinon.stub();
        testHelper = new TestHelper();
    });

Is this the way to go? Could we maybe document this somewhere so people use this way of writing code? Reaping the benefits from a short syntax and strong typing.

(getting old Java vibes from this way of working 馃檲)

@43081j
Copy link
Contributor

43081j commented Jun 10, 2019

@nicojs it seems typescript 3.5.1 release affected this, i hadn't realised!

// TS3.4
let stub: sinon.SinonStub; // this means its <any[], any>
stub = sinon.stub(obj, 'method'); // WORKS!

// TS3.5.1
let stub: sinon.SinonStub;
stub = sinon.stub(obj, 'method'); // Type 'SinonStub<[string, string], string> is not assignable to 'SinonStub<any[], any>'

@rbuckton do you happen to know which change between 3.4 and 3.5.1 might have caused this?

I guess the solution would be to correctly type the declaration now?

let stub: sinon.SinonStub<[string, string], string>;
// or if you dont care about the strong typing
let stub: sinon.SinonStub<any>;

in reality, the same code in 3.4 would have just been defaulting to any[] anyway, so all strong typing was already lost. it just didn't error and now does.

@JohannLai
Copy link

JohannLai commented Jun 10, 2019

I got this error when I use resolves to stub a promise function, I got the error:

Type 'string' is not assignable to type 'never
(method) Sinon.SinonStub<TArgs extends any[] = any[], TReturnValue = any>.resolves(value?: never): sinon.SinonStub<any[], any>

how can i make it work ? thanks

@Codex-
Copy link
Contributor

Codex- commented Jun 11, 2019

yeah the recent changes unfortunately broken the spy method typing too

@rbuckton
Copy link
Collaborator

@DanielRosenwasser: Do you know what change in 3.5 could have affected this?

@43081j
Copy link
Contributor

43081j commented Jun 11, 2019

Do note for those of you appearing here, it is not a sinon types problem, they have not changed and are correct.

The problem exists between TS3.4 and 3.5.

The error is correct: you can't assign [TFoo, TBar] to any[] as it isn't an array, it is a tuple.

TypeScript 3.4 must have inferred looser types or some such thing, or maybe it inferred a union (any[]|[TFoo,TBar]) whereas 3.5 now uses the default (any[]).

@43081j
Copy link
Contributor

43081j commented Jun 11, 2019

As for the people having problems with resolves, it is due to changes in #31401.

That PR correctly tries to infer the promise's value type and, if it is impossible, is typed as never which means you can't use it.

Here's some examples to explain its behaviour:

Works

class Foo {
 myMethod(): Promise<boolean>;
}

const stub = sinon.stub(instanceOfFoo, 'myMethod'); // TReturnType will be Promise<boolean>
stub.resolves(true); // Because it is a Promise, the types correctly infer this needs a boolean

Does not work because return type is not a promise

class Foo {
 myMethod(): any;
}

const stub = sinon.stub(instanceOfFoo, 'myMethod'); // TReturnType will be `any`
stub.resolves(true); // Because it is a `any`, it isn't typed as a Promise, so resolves doesn't know what to accept: hence, `never`

Does not work because there are overloads, multiple possible return types

class Foo {
 myMethod(): Promise<boolean>;
 myMethod(foo: boolean): void; // This is an overload
}

const stub = sinon.stub(instanceOfFoo, 'myMethod'); // TReturnType will be `void`
stub.resolves(true); // Because there is an overload, TS can't infer which return type you're expecting, so again it is `never`

@pjo336 this example above, in particular the overloads (3rd example), is why bcryptjs types don't work with it as there are two compare overloads with different return types.

The sinon types should probably be loosened to default to any if the type can't be inferred, not never.

This resolves problem is separate and different to the one we're having above I think

cc @JohannLai

@pjo336
Copy link

pjo336 commented Jun 11, 2019

@43081j Thanks for the detailed explanation. That makes a lot of sense.

@JohannLai
Copy link

JohannLai commented Jun 12, 2019

@43081j Thank you

@43081j
Copy link
Contributor

43081j commented Jun 13, 2019

FYI a change was recently merged which at least fixes the resolves problem some of you were seeing above.

Still not sure why the other problem appeared though.

@DanielRosenwasser do you know if between 3.4 and 3.5 the inferred type of a late assigned variable changed? The current behaviour (3.5) is a pain for some but seems correct and what you would expect in a type system. However it still means 3.5 introduced a breaking change

@davloperez
Copy link

Still not sure why the other problem appeared though.

Nope. Issue still happening for overloaded methods like this:

class Foo {
 myMethod(): Promise<boolean>;
 myMethod(foo: boolean): void; // This is an overload
}

const stub = sinon.stub(instanceOfFoo, 'myMethod'); // TReturnType will be `void`
stub.resolves(true); // Because there is an overload, TS can't infer which return type you're expecting, so again it is `never`

I didn't found any Open specific issue related exctly with this, just this one. Anyone knows how to solve it without using a workaround like as any ?

@43081j
Copy link
Contributor

43081j commented Mar 15, 2020

Last time this was discussed, we came to the conclusion to tell people just to cast to the right interface:

sinon.stub(obj, 'method') as SinonStub<[boolean], void>;

Reason being if we try add generics so you can override it when creating a stub, we will slow the compiler down a fair amount.

Overloads can't be inferred well since the compiler doesn't know which you mean. So casting seems the way to go.

@davloperez
Copy link

I really thank you your fast response.

I thought that previous post was the same problem than I'm having in my code, but maybe I was wrong, and it is a different issue that should be in its own thread. The real problem I'm having is the following:

class Foo {
    myMethod(): boolean;
    myMethod(foo: boolean): void;
    myMethod(foo?: boolean): boolean | void {
        return foo;
    }
}

class Bar {
    constructor(private foo: Foo) { }
}

const foo = sinon.createStubInstance(Foo);
const bar = new Bar(foo); /* Fails with:
error TS2345: Argument of type 'SinonStubbedInstance<Foo>' is not assignable to parameter of type 'Foo'.
The types returned by 'getOneByType(...)' are incompatible between these types.
Type 'Promise<unknown>' is not assignable to type 'Promise<R>'.
    Type 'unknown' is not assignable to type 'R'.
    'unknown' is assignable to the constraint of type 'R', but 'R' could be instantiated with a different subtype of constraint '{}'.
*/

The problem is not only the return type of myMethod, but the entire type returned by sinon.createStubInstance. It is not fully "compatible" with the original type.

I have try the following change in: node_modules/@types/sinon/ts3.1/index.d.ts

image

By adding that change, the type returned is fully compatible with the original, and it still having all extra features added by SinonStubbedMember on each method.

Do you think this is a safe change?
Should I create a PR for this change?

@43081j
Copy link
Contributor

43081j commented Mar 15, 2020

It's a similar cause than the other problems in this thread so you're in the right place.

Iterating over the keys (P in keyof TType) means it'll only pick up one of the overloads in the resulting interface.

Your fix probably means one of those overloads is right, but the rest don't have the stub properties. Which isn't the perfect solution but is better than those members being inaccessible... I'll have a play around with it later and see what we can do, if it turns out there isn't any better way I'll let you know so you can do a pr

@saraid
Copy link
Contributor

saraid commented Apr 28, 2020

I have a fairly similar issue. I'm trying to stub history.push. I've stripped my code down to the essentials:

import sinon, { SinonStub } from 'sinon';
import { History, createBrowserHistory } from 'history';

const history = createBrowserHistory();
const doSomething = (path: History.Path) => {};
(sinon.stub<History, keyof History>(history, 'push') as SinonStub<[History.Path], void>).callsFake((path) => {
  doSomething(path);
});

鈽濓笍This is my current iteration. Here's my previous iteration: 馃憞

(sinon.stub<History, 'push'>(history, 'push') as SinonStub<[History.Path], void>).callsFake((path) => {});

It failed by picking the wrong type signature; the relevance of the change seems inexplicable. I've found that various changes that, to my novice knowledge, shouldn't affect the type signature... are doing so. Does anyone have some advice on how best to perform the stubbing such that tsc has consistent results?

I'm worried about how unreliable my solution currently is, but I do not understand why it is that way. Especially since I just tried to create a repl.it and it is now failing there.

@johnjesse
Copy link
Contributor

johnjesse commented Apr 28, 2020

I think this is still the way I'd be doing it #36019 (comment)

you shouldn't need to explicitly pass in the types to the stub call - as they should be inferred.

Depending on how much the overloads overlap, you might need to cast in between - e.g. see https://codesandbox.io/s/dealing-with-overloads-2xgow?file=/src/index.tsx

I wish there was a better way, but it's an issue with overloads

@saraid
Copy link
Contributor

saraid commented Apr 28, 2020

@johnjesse did you link the wrong sandbox? I don't see any overloads in that file, or in the other files of that sandbox.

@johnjesse
Copy link
Contributor

@saraid you are absolutely right, just edited the comment

@nicojs
Copy link
Contributor Author

nicojs commented Jan 21, 2021

I'm closing this issue since this code sample now works again 馃憤

describe('fs', () => {
  let existsSyncStub: sinon.SinonStub;
  beforeEach(() => {
    existsSyncStub = sinon.stub(fs, 'existsSync');
  });
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants