-
Notifications
You must be signed in to change notification settings - Fork 12
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
Automatic Sinon spies/stubs restoring #72
Comments
This looks like a repeated piece of code. So it should be defined one in the sinon or ckeditor bender plugin. DRY. |
@pjasiun wrote in priv chat:
This is something I've been also aware, so I rejected things like https://www.npmjs.com/package/mocha-sinon which tries to automate the thing too much (I like to have a choice :D). The test author should be able to decide what should happen with a specific spy/stub. Both solutions that I proposed above allow you to make that decision. On the other hand, one could argue that spies/stubs should never ever be shared between tests. This is also totally valid, so IMO we should avoid sharing them between tests. However, it's good to have a choice. |
The problem here is that it cannot go to benderjs-sinon or benderjs-mocha, because it's a link between those two plugins. That's why in the solution I was going to propose you need to call However, if we want to be super strict about DRY, then we can add something to CKE's plugin for Bender. But I'm not a big fun of this solution as you'll start repeating that in other projects using Bender and Sinon. Therefore, I think that we need some simple mechanism and a convention. If you think that Sinon sandoboxes violate DRY, then perhaps the second option is fine? |
I'm also checking if that could work (first tests are promising):
|
It seems to work fine. I'm OK with this approach. |
We take care of this manually. |
I want to propose that everytime we create a Sinon spy or stub that needs to be restored before the next test executes we use Sinon sandboxes like this:
I've been also considering extending
benderjs-sinon
with something like this (note, it must be testing framework agnostic):It'd be shorter, but it isn't that much simpler, so the default Sinon way looks satisfying.
Any thoughts?
The text was updated successfully, but these errors were encountered: