Add resolveValue and rejectValue spy strategies

[elliot-nelson]

Description

This is a feedback-only PR (no specs yet) that adds some additional spy strategies to core.

.and.resolveValue

Return a resolved promise for the specified value.

.and.rejectValue

Return a rejected promise for the specified value.

Promise: end user experience

I've been looking through my various jasmine helper files and I think for least surprise as an end user, either of these two approaches would be "what I expect":

// in a helpers.js file, etc.:
jasmine.setPromiseLibrary(require('bluebird'));

// or:
jasmine.getEnv().configure({
    promiseLibrary: require('bluebird')
});

Could it be more like addMatchers, where it has to happen in a beforeEach? Sure, it definitely could, although I didn't go that route here (see next section).

Promise: internal experience

I saw your (Greg's) notes on my other PR. This is my attempt at a compromise :).

I'm not sure it's worth option-plumbing Env > SpyFactory > Spy just for providing a pointer to a useable Promise. I see a Promise as being more of a globally available utility; more like j$.isPromiseLike than the options passed to the Expectation factory.

Based on that reasoning, this implementation:

• Internal usage of promises is intended to be through j$.getPromise(). This is where we pick up global promise if it exists.
• End users can configure a custom promise lib through Env, so, Env also provides a getPromise() (this only returns a lib if one was configured).
• Promise-checking (i.e., determining whether a promise lib is available) is the responsibility of the code using it. In this case, the individual spy strategies check and generate custom messages if it isn't available.

Alternative: option-plumbing

If you're convinced plumbing for Env > SpyFactory > Spy is the right approach, then it's about equal work to go the "global Promise impl" route like I have, or to make it a runnable per-spec setting instead. Per-spec feels a little odd to me, but, it perhaps does make sense for large repos that are testing multiple components (perhaps client & server components that use different promise libraries).

Motivation and Context

The spy strategies above are the ones I find myself missing the most, and now that custom spy strategies are a thing individual projects can solve the problems one-off -- but putting these strategies that almost any project could use in core makes tests more portable and gives an "accepted" syntax.

How Has This Been Tested?

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[sgravrock]

and.rejectValue might help fewer people run into problems like #1648.

[slackersoft]

I'm not sure what the use case for multiple fake implementations is. At the point where you're telling Jasmine which code to run, you can already do different things on different invocations.

I can definitely see the need for and.resolveValue or and.rejectValue, and possibly the multiple call variants as well. The Promise stuff gets tricky due to Jasmine's current browser support matrix. We do actually still support IE 10 & 11, which don't even have built-in Promise, so Jasmine would need to provide an intelligible message when the Promise-based strategies are used but there is no default implementation for Promise available, as you have here.

I think it makes more sense to have this be an explicit configuration call or setting the in config() function. jasmine.getGlobal() is really intended for internal-to-Jasmine usage to find whatever the global object happens to be (window in a browser, global in node.js, etc.).

[elliot-nelson]

I'll pitch the multiple fakes: testing retry functions. Seems to come up frequently for me for some reason.

request.get.and.callFakes(
    () => Promise.reject(new Error("404")),
    () => Promise.reject(new Error("404")),
    () => Promise.resolve("OK")
);

expect(await myModule.getWithRetry()).toEqual("404");

Yes, you can implement mocks that check calls.count directly, but it gets tiresome for complicated tests and is not as readable as above (imo). And you can't use my suggested Promise-based strategies because you need a mix of both resolves and rejects.

[slackersoft]

Ok, that use case makes sense. I might actually also look at baking some better knowledge of multiple calls into the higher level spy strategy such that you could do something like:

request.get.and.rejectValue(new Error('404'))
    .andThen.rejectValue(new Error('404'))
    .andThen.resolveValue('OK')

or:

request.get.and.do(
  (call) => call.rejectValue(new Error('404')),
  (call) => call.rejectValue(new Error('404')),
  (call) => call.resolveValue('OK')
)

The first is probably going to require more work to the SpyStrategy to make it work, but the interface feels a little cleaner to me. The latter would "just" be a new strategy that changes the strategy after each spy call. I like either of these as longer term solutions as they allow all the other spy strategies to get the multiple variant form without extra effort.

I also understand if you don't want to take this on at the moment and this might be something I look for in Jasmine 4.0.

[elliot-nelson]

Actually, I like that a lot, and may propose a solution in a different PR. (I think this PR is basically going to become "let's get some promise-based strategies", which is a separate thing.)

[elliot-nelson]

I'm still working through the specifics, but I think the implementation really depends on what behavior you want. Especially with regards to the current spy interface which allows stuff like:

if (spy.and.plan !== myMock) { spy.and.plan = myMock; }

I would want to be careful not to break that not-documented-but-OK behavior. I think the correct solution is to change SpyStrategy's core guts to be a planned queue. Then andThen can return a proxy that ensures new plans are added to the queue, whereas the default SpyStrategy behavior is to set the queue to [plan].

(We can turn plan into a property, to ensure that the not-documented behavior I mentioned earlier still holds true -- for example, directly assigning plan should be equivalent to setting the plan qeueue to [plan].)

[slackersoft]

I think I would actually be more inclined to remove the ability to set plan directly (leaving the getter). Reaching into the spy strategy to set one of its properties shouldn't really be necessary now that you can provide custom spy strategies. Note that we shouldn't remove the setter immediately per your concern but probably use Object.defineProperty to mark it as deprecated if anyone is still using it.

[elliot-nelson]

Updates:

• Updated body + title.
• Trimmed PR to only 2 new promise-based strategies, with the necessary promise handling.
• Added unit tests.

[slackersoft]

The matchers themselves look pretty good. There are a couple of changes I'd like to see so that Jasmine doesn't expose the configured library to the outside world unless it really needs to.

[elliot-nelson]

Thanks for all your assistance. I've made your suggested changes to the specs & "no promise" failure messages (open to additional thoughts on the error message wording).

I'm confused by your other comments though and want to check my assumptions:

I don't think it's even possible (right now) to allow the user to configure promiseLibrary through jasmine.getEnv(), and have that value be accessible to SpyStrategy, without exposing it somewhere to the end user. Either in jasmine.getEnv() or jasmine. So I'm not sure how to follow through on those comments.

(The only examples I see in the code currently are where Env has local vars and passes them directly down through constructors. This would accomplish your goal but would require plumbing through SpyFactory etc. I'm willing to do that if we want it totally invisible, I just want to make sure I'm understanding properly before I tackle that...)

[slackersoft]

For hiding the Promise library, I was thinking of something like wiring the library through to the SpyFactory and on to the particular strategies involved. I'm open to some discussion on this and I promise I'm not trying to make your life harder. The goal is to not expand Jasmine's public interface unless we really need to. Obviously the expansion is necessary to allow the library to be set, but I would like to explore keeping it private after that.

[elliot-nelson]

I think that's totally reasonable.

I've pushed a commit that goes all the way to the other side (zero new surface area), just so we have a checkpoint.

Specific changes:

• Env supports configure({ Promise: Blah }) (changed from promiseLibrary, I think "Promise" is more consistent with other packages out there that support setting a custom promise).
• Promise is made available through a getPromise handler that is wired through SpyFactory -> Spy -> SpyStrategy.
• SpyStrategy has zero new surface area; in order to do this, we add the promise-based strategies directly instead of adding them to prototype.
• Moved some specs around so each module has unit tests just to cover basic LOC; SpySpec now has the "integration tests" to confirm promise-based strategies work as expected.

Let me know if this is what you were thinking, or we can scale back a little bit (for example, resolveValue etc. could be on prototype, but only if you're willing to expose a getPromise_() on SpyStrategy).

[elliot-nelson]

I also have a thought on the naming:

• How closely do we need to track to .returnValue? Maybe we could consider spy.and.resolve(42) and spy.and.reject(new Error()) instead. Or resolveWith and rejectWith.

[slackersoft]

I'm definitely not wedded to the resolveValue/rejectValue naming. I would tend to stay away from just resolve/reject just to ease on naming overloads. resolveWith/rejectWith sound great.

[elliot-nelson]

Updates:

• resolveValue -> resolveWith
• rejectValue -> rejectWith

(I'm liking these a lot after writing some sample tests; "spy on method and resolve with value" & "spy on method and reject with error" are imo a good match for the rest of jasmine's semi-conversational API.)