td.when return type could be typesafe

[mohaalak]

trafficstars

Description

let's say that we have an interface like this in typescript

interface Test {
   getT: () => {id: number}
}

now we can mock it like this

const test = td.object<Test>();

but when we want to use td.when the problem is that thenReturn and thenResolve can accept any value

right now I write a new declaration for custom merging for testdouble it's like this

declare module 'testdouble' {
  export function when<D>(...args: D[]): Stubber2<D>;
  export interface Stubber2<D> {
    thenReturn<T>(first: D, ...args: D[]): TestDouble<T>;
    thenDo<T>(f: Function): TestDouble<T>;
    thenThrow<T>(e: Error): TestDouble<T>;
    thenResolve<T>(first: D, ...args: D[]): TestDouble<T>;
    thenReject<T>(e: Error): TestDouble<T>;
    thenCallback<T>(...args: any[]): TestDouble<T>;
  }
}

so when I use it like this

td.when(test.getT()).thenReturn({})

this will raise an error that id is missing

I said "could" cause this will be a breaking change, but it's good to have

[searls]

Sorry as I haven't used td.js with typescript. @lgandecki?

[lgandecki]

If we decide to do this we need an additional overloaded declaration for when, like so (the order is important apparently):

  function when<P, D>(f: Promise<P>, ...args: D[]): { thenResolve(response: P): TestDouble<P> };
  function when<D>(...args: D[]): Stubber<D>;

Otherwise it will require people to return a Promise as an argument to thenResolve (which will clearly be wrong and annoying)

I do have my own wrapper for td.when for the times I want to stub using thenResolve. And I love the typesafety that comes with testdoubles in those cases.

But, as @mohaalak mentioned, this is a breaking change and I think whether someone wants it or not might depend on someone testing style. (With a wrapper I might decide NOT to use it, and opt-out of the typesafety for the returned value, but with changes to the type definitions I think we will force everyone to use it).

I'd actually love to hear your thoughts here @searls!

Without the type safety we can return partial stubs and for example check an early-exit scenario. For example, assuming we sell tickets and have complex pricing rules, but we never sell to kids below 13 years old, and between 14-15 the tickets are always free, all other scenarios being complex.

As you can see in those cases typescript will complain:

TS2345: Argument of type ‘{age: number;}’ is not assignable to parameter of type ‘UserInfo’. 

Type ‘{age: number;}’ is missing the following properties from type ‘UserInfo’: name, lastName, clientSince

Even though we don't need them, and I think one might make an argument that forcing someone to construct the whole object might make unit testing less readable.

This is especially problematic for stubbing results of external API calls that could return really large objects. (but then we could probably make a wrapper that transforms the data to the shape we want, instead of calling external APIs directly)

At the same time - this breaking change will probably find some edge-case bugs in people codebases :) Especially when the return types changed overtime but tests were not updated to reflect that.

And you could always write a factory method that creates an object while specifying only the fields you are interested in for the given test.

I hope I made the case understandable enough to make some decision here. If not, please let me know I will try to reiterate or provide a different example. The same discussion could apply even if you don't use typescript - would you always stub the return values fully, or would you be happy with (and actually advise) partial stubbing?

[mohaalak]

If we decide to do this we need an additional overloaded declaration for when, like so (the order is important apparently):

  function when<P, D>(f: Promise<P>, ...args: D[]): { thenResolve(response: P): TestDouble<P> };
  function when<D>(...args: D[]): Stubber<D>;

Otherwise it will require people to return a Promise as an argument to thenResolve (which will clearly be wrong and annoying)

my bad you are correct for promises we should overload declaration and I think this is good there were sometimes that the function that I wanted to mock was Promise but I used thenReturn we can overload it to use thenResolve and thenReject

This is especially problematic for stubbing results of external API calls that could return really large objects. (but then we could probably make a wrapper that transforms the data to the shape we want, instead of calling external APIs directly)

At the same time - this breaking change will probably find some edge-case bugs in people codebases :) Especially when the return types changed overtime but tests were not updated to reflect that.

And you could always write a factory method that creates an object while specifying only the fields you are interested in for the given test.

I know what you are talking about sometimes is easy to just create the data that test wants, but also when the return type of function changes typescript complain and it will be easy to correct the tests.

[lgandecki]

Actually, Thinking about it a bit more what I would recommend is wrapping the return with partial https://www.typescriptlang.org/docs/handbook/utility-types.html#partialt

Then I believe we get the best of both worlds. We don't require every single field to be there but the fields that are there have to be correct

[mohaalak]

I think it's better than any type

[searls]

Apologies, as this conversation makes sense but the typescript bits are something I can't speak to with any confidence. I think in general, greater type enforcement is a good thing from a mocking library like this one, because it can help define what types you need to create, but I agree with @lgandecki that enforcing the test to specify fully-fleshed-out objects is way too much pain for what it's worth. This utility type feature seems like a good middle point.

As for breaking change, I leave how to handle it up to you, @lgandecki. What's next?

[lgandecki]

If we go with the "partial type" route - breaking tests should catch wrong type definitions or wrong stubs - that should be a valuable feedback and I think it'd be ok to do this as a minor change, even if it forced people to fix things after updating.

To that end - we should probably add documentation about this with an example or two. As for what's next - @mohaalak if you want to prepare a PR with the code changes I can work on the documentation. Once your PR is in I'd try it on the codebases where I have testdouble with typescript to make sure all is fine.

( If we went with the full-enforcement there would be a ton of failures for people stubbing not-fully-fleshed-out objects, which I'd consider a breaking change that would require a major bump, but I think we all agreed that this is not the way to go. )

[mohaalak]

ok, I will make a PR for these changes

[mohaalak]

I have a question to make PR @lgandecki I don't know why td.when need multiple function calls,

  export function when(...args: any): Stubber<D>;

I think the correct is that when have only one function call and the second parameter is optional that have options

[lgandecki]

@mohaalak sorry, I don't understand the question. I see you added two overloaded when definitions as I suggested. Have you figured it out? Or are you concern about something else? In particular I'm not sure I understand what do you mean by "multiple function calls"

[mohaalak]

I asked why when function can accept multiple arguments, in pull request it accepts one argument for stubbed function and another optional argument for stubbing config

[lgandecki]

Ok, I understand what you mean now. I was iterating on what was already there in the typings and was mostly concerned about the promise mismatch. But you are correct, another problem with the current typings is that there was this ...args: any part, being more specific is definitely better.

Now that I know you are done I can get to reviewing it, will get it through this week.

Thanks for all your help!

[natesilva]

3.14 and higer breaks cases where we were returning “symbolic” results from a test instead of mocking out the full return object.

Say we have a function that returns a complex object. We don’t care about the details of that object for this test. So we mock it like this:

td.replace(SomeModule, 'someFunction');
td.when(SomeModule.someFunction).thenResolve('the result');

Starting with 3.14 this fails unless we do as any which is smelly.

thenResolve and sister functions will allow Partial responses. So if we only wanted to mock part of the response this would work:

td.when(SomeModule.someFunction).thenResolve({ oneParticularProperty: 'foo' });

I don’t think this is a bad thing! But it probably shouldn’t be in a minor point release.

And what is the opinion behind TestDouble? Do you think it’s better to mock out realistic return values, or not?

[natesilva]

We can also do this:

….thenResolve({})

But later when we do an assert against a generic empty object, that seems less trustworthy than testing a specific symbolic string like "the user" or "the order" which is what we were doing.

[searls]

Hi @natesilva apologies as I misread @lgandecki's comment above suggesting a major bump was appropriate. Force of habit, I missed it. I'll leave him to comment on your specific issue

[lgandecki]

Sorry for the troubles caused by a backwards-incompatible change.

@natesilva could you please show an example of a function and a test where you would want to resolve something as a string and then make an expectation on that string?

Seems to me that doing the "as" shouldn't be considered a smell - it's a way to tell the compiler "I know this is incorrect but I know what I'm doing".

I think there are more cases where it's important that your mocks match the types, so it's better to have the balance this way - allow for incorrect values with the extra syntax, but check the correctness by default.

As for having a generic empty object, the other pattern you could use is to create a helper function that returns an object with the correct shape and takes arguments that allow for extending.

Your idea with the object being partially resolved seems right as well, I'd just extract it to a variable, so you could reuse it in the expectation.

const USER = {name: "name"}
td.when(SomeModule.fetchUser(id)).thenResolve(USER);

const res = functionUnderTest(id)
expect(res).toEqual(USER)

[natesilva]

Here’s an example. I don’t claim it’s great code.

This is testing that a wrapper function calls fetch(…) with the proper arguments and returns its result. It could be rewritten to use td.verify, although that wouldn’t test that it’s returning the result.

    it('SHOULD POST to the URL if it is valid', async () => {
      td.replace(Validator, 'isURL');
      td.when(Validator.isURL('the url')).thenReturn(true);

      td.replace(nodeFetch, 'default');
      td.when(
        nodeFetch.default('the url', td.matchers.contains({ method: 'POST' }))
      ).thenResolve('the response');

      assert.strictEqual(await postJSON('the url', 'the data'), 'the response');
    });

We worked around it by pinning to version 3.13.1. Next we’ll go through and fix these tests, doing something similar to what you suggested:

      const theResponse = td.object<Partial<nodeFetch.Response>>();
      td.replace(nodeFetch, 'default');
      td.when(
        nodeFetch.default('the url', td.matchers.contains({ method: 'POST' }))
      ).thenResolve(theResponse);

      assert.deepStrictEqual(await postJSON('the url', 'the data'), theResponse);

I just want to say, I think this is a good feature. Those of us using TypeScript are doing so for a reason. We were just surprised to see it in a minor version.

Regards, Nate

[giltayar]

Stale. Closing. Please reopen if still relevant and I will look into it.