-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add resolveValue and rejectValue spy strategies #1688
Add resolveValue and rejectValue spy strategies #1688
Conversation
|
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 I think it makes more sense to have this be an explicit configuration call or setting the in |
I'll pitch the multiple fakes: testing retry functions. Seems to come up frequently for me for some reason.
Yes, you can implement mocks that check |
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. |
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.) |
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:
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 (We can turn |
I think I would actually be more inclined to remove the ability to set |
99b70ae
to
8c429eb
Compare
8c429eb
to
72aa0bb
Compare
Updates:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 (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...) |
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. |
6b246f4
to
4731b4e
Compare
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:
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 |
I also have a thought on the naming:
|
I'm definitely not wedded to the |
Updates:
(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.) |
…son/jasmine into elliot-nelson-enelson/spystrategies - Merges #1688 from @enelson - See #1590
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":
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 likej$.isPromiseLike
than the options passed to the Expectation factory.Based on that reasoning, this implementation:
j$.getPromise()
. This is where we pick up global promise if it exists.getPromise()
(this only returns a lib if one was configured).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
Checklist: