improve-stubs-doc.md

[andrewdroz]

#2062

Purpose (TL;DR) - mandatory

Improvement to stubs doc

• Focus more on the basics of stubbing an object's function (stub pre-programmed to throw exception/return value)
• Less discussion on spies
• Provide very simple code to test as an example, instead of external library's (PubSub) code
• Show why restoration of a stub is necessary

How to verify - mandatory

N.A. because no code change

Checklist for author

• npm run lint passes
• References to standard library functions are cached.

[fatso83]

Hi, thanks for you contribution! While I can sympathize with the intent, unfortunately the execution doesn't go that well with the new work that has gone into making runnable examples. See all the work done on #2122 for the spies docs.

If there should be changes to the example code in the documentation, then the changes should be made so that they are actually runnable. The new example you posted would not pass this, as ./emailService is not an actual module.

Provide very simple code to test as an example, instead of external library's (PubSub) code

I don't see how it's actually easier? The interface of PubSub is already pretty simple, so any consumer could be made pretty simple as well. I think adjusting your example might end up being just as simple.

[fatso83]

As tests have a tendency to fail, it's better to have restore steps that will run irrespective of the failure status. This is better left to a afterEach hook.

[fatso83]

Avoid let and other ES2015+ syntax in the examples. We try to have all produced be ES5.1 compatible.

[fatso83]

Is there something I can do to get the train back on its tracks?

[andrewdroz]

Hello, sorry for not giving a reply thus far. Okay, I understand your points, and probably there's no need to remove the use of PubSub (I didn't really know what PubSub was about). I plan to try implementing runnable code this weekend, so I'll keep you posted.

[fatso83]

One shouldn't really need to know anything about the implementation of pubsub either. It's just an example of a very common pattern used practically everywhere: Publish/Subscribe (which is more or less the same as the Observer Pattern). As it usually just deals with two methods; publish({topic,msg}) and subscribe(topic) the concept is usually known to most, which is why I assume it was used to begin with.

[andrewdroz]

I have replaced almost all the examples to be runnable in my latest change. There are 2 examples (yieldsTo, callArg) for which I had trouble making tests that would pass, perhaps due to me not fully understanding the two functions.

[fatso83]

Wow, this seems great. I will need to check it out to understand what (if anything) is wrong with the 2 examples you mention, but in any case, this is a huge improvement.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[fatso83]

Ooh, bad me. I have forgotten about this ...

[fatso83]

I have looked into this and there was a test I could not get passing, spies-2-wrap-object-methods.stub. It always failed due to the sandbox not replacing all the fields with spies. This seems like a bug within Sinon, somehow, as changing the beforeEach to this made it pass:

           sandbox.spy(jQuery, 'getJSON');
           sandbox.spy(jQuery, 'ajax');
           //sandbox.spy(jQuery);

I will look some more into it.

---

EDIT: the example was at fault. jQuery is a function and so Sinon will try to wrap that function, not its fields.

[fatso83]

This is not abandoned, btw. I am actively working on a fork of this branch to get everything working, but I am struggling with a last test, as pubsub is throwing exceptions after the event loop has finished the current work (the test). So we are just talking amongst us about how to rewrite this to not cause Node (or Mocha) to exit with a non-zero exit code.

[fatso83]

Included this in #2239 (leaving your commit intact!), so when/if that gets merged, then so will this :)

[fatso83]

Thanks for all the work, @andrewdroz! I had some additions as you can see in the PR that built upon this, but this should be live as soon as the next release is published.