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

Multiple stubbings with argThat causes unexpected behavior #180

Closed
ackerdev opened this issue Jan 23, 2017 · 13 comments
Closed

Multiple stubbings with argThat causes unexpected behavior #180

ackerdev opened this issue Jan 23, 2017 · 13 comments

Comments

@ackerdev
Copy link

ackerdev commented Jan 23, 2017

Rehearsal invocations cause argThat matchers to be invoked, and can cause unexpected exceptions.

Example: This code...

const fooTest = td.function();
td.when(fooTest(td.matchers.argThat(arg => arg.foo.length > 2)).thenReturn(true);
td.when(fooTest(td.matchers.anything()).thenReturn(false);

When run, will throw the exception:

TypeError: Cannot read property 'length' of undefined

Originating from line 3 of the snippet above. The rehearsal invocation of foo to set a new stubbing causes the invocation of the existing argThat matcher, which receives the td matcher instance (td.matchers.anything()) as arg, which of course does not have the property foo, and causes the exception above.

This particular example can be fixed by ordering the argThat stubbing last, but scenarios where multiple argThat stubbings are required seem to have no current solution.

@searls
Copy link
Member

searls commented Jan 23, 2017

This is a good point, and well-argued. Would you be interested in trying to tackle a solution to this as a PR? If not, I'll try to get this prioritized

@ackerdev
Copy link
Author

I'm not sure if I feel comfortable contributing at this time, but I'll certainly take a look and see if I can do it myself or at least highlight some leads on potential fixes.

Do you have any initial thoughts as to how to best solve this? I'm not as familiar with the rehearsal invocation logic to know what kind of things one might do to prevent this invocation... Is there perhaps a method with which the argThat matcher could be altered to ignore an invocation if it is a rehearsal?

@searls
Copy link
Member

searls commented Jan 23, 2017

Thinking a moment longer, yes, we need to decide what a solution even looks like in this case before implementing something.

I'm honestly a little bit stuck on what can be done here from td's perspective.

Stubbings necessarily happen synchronously, so a return value needs to be resolved immediately. td.js also is not a "modal" library in that there's not a "rehearsal mode" and an "ok all my stubbings are in place now" mode to differentiate whether to evaluate an invocation to a return value. That means any rehearsal invocations need to act on the stubbings that came before them.

The more I think about this, the more it feels intractable from the library's perspective. My advice would be to make your argThat expressions more durable so that they don't blow up when they don't match. The contract with a matcher should be returns truthy for matched, falsey for not matched, and throwing an exception isn't really defined as anything other than "something went wrong" when evaluating matchers.

Here's an idea: write a custom matcher that's just like argThat except it catches errors and returns false. Wouldn't that serve your purpose?

@searls
Copy link
Member

searls commented Jan 23, 2017

If you can think of a good matcher name for argThatButThrowingIsCoolToo, I'd be happy to ship it as a built-in

@ackerdev
Copy link
Author

Wouldn't that matcher result in an incorrect state for the double, which would manifest as unexpected behavior in tests that might assert on double state like the number of invocations, or not matching?

My initial thought was seeing if it might be possible to somehow mark rehearsal invocations that occur within when somehow, so that the double can skip invoking stubs if it can see that it is a rehearsal invocation.

I'm trying to grok the when and store calls logic at the moment to see if there's any possibility for that.

@searls
Copy link
Member

searls commented Jan 23, 2017

Let's focus on the first statement:

Wouldn't that matcher result in an incorrect state for the double, which would manifest as unexpected behavior in tests that might assert on double state like the number of invocations, or not matching?

Why would this be? If an argThat predicate throws an error, calling that "not a match" seems reasonable to me in this case. Struggling to understand your point, but i don't doubt you.

Maybe try this for yourself to experiment with something like this (written in browser/untested):

var argWhich = td.matchers.create({
    name: 'argWhich',
    matches: function(matcherArgs, actual) {
      var predicate;
      predicate = matcherArgs[0];
      try {
        return predicate(actual);
      } catch (e) {
        return false;
      }
    }
  })

@ackerdev
Copy link
Author

I think my comment was erroneous based on a naive understanding of how td worked. That does work, but feels a bit sketchy especially as actual errors in the predicate will be difficult to debug. I also can't think of a good name for something like this.

@searls
Copy link
Member

searls commented Jan 23, 2017

Well, it's why I wouldn't replace argThat over it, but if you'd rather have terse predicates, that's probably your only avenue.

Other options:

  1. make your argThat predicate function more durable so it doesn't blow up so easily, like: arg => _.get(arg, 'foo.length') > 2
  2. only use argThat as a very last resort. For unit tests you should almost always know exactly what the arguments are, and so I would have written td.matchers.contains({foo: ['la','blah']}) if at all possible.

@ackerdev
Copy link
Author

That's fair; this isn't exactly something I'd expect anyone to see when stubbing directly in a test.

My usecase was for creating slightly more robust fakes that could be reused in a few tests, reducing some duplication of stubbing and potential for errors between those duplicated stubs.

An additional motivation for this was that with promise returning functions being stubbed with a specific expectation, a failed invocation can lead to an ambiguous error of Error: Cannot read property 'then' of undefined. By creating a double with a few stubbings, I could create a slightly more robust fake that would allow the function to execute in full and give a better error like a td.verify expectation failing.

@searls
Copy link
Member

searls commented Jan 23, 2017

Sure. The only guidance I'd give you is that more robust fakes require more robust edge-case handling, which can be particularly fraught unless you test the fakes themselves.

In a sense, the library is intended to be terse and expressive to decrease the number of times where a fancier regime of fakes seems preferable.

@ackerdev
Copy link
Author

You're not wrong; that's a fair stance to have. I wasn't necessarily sold on doing it that way, but playing around with it lead me to this problem.

Still, it seems to me that this is a small wart on an otherwise unmatched api that would improve flexibility, even if not the default recommended usage pattern. It doesn't seem like an easy problem to solve without altering the interface, and if it's not I suppose I'll live without it, but if there was any way that we could potentially affect this without creating a whole new set of gotchas I think it would be great. I think I'll continue perusing the source and pondering if there's any way we could potentially do this. Certainly looks like a long shot at this point, but I'm willing to poke around and see regardless.

Do you think this gotcha might be worth adding warnings for on the docs page for when?

@searls
Copy link
Member

searls commented Jan 23, 2017

I would be open to warning people of two things on the stubbing doc page along the lines of:

Any side effects in thenDo or a predicate matcher like argThat will be invoked when you set up subsequent stubbings, so plan accordingly to ensure that they are free of test-altering side effects and will behave appropriately for whatever input they might be passed

@ackerdev
Copy link
Author

Seems like a reasonable description to me 👍

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

No branches or pull requests

2 participants