sinon icon indicating copy to clipboard operation
sinon copied to clipboard

Extract sandbox and fake into separate modules

Open mantoni opened this issue 6 years ago • 12 comments

I'd like to propose a way how we can make sinon more modular and allow to compose a version that only has the fake API and no spy, stub, mock and ideally also has optional lolex and nise.

Why?

The API surface of Sinon is too large and removing APIs is really difficult and a long process. Therefore, we might just keep it the way it is, have Sinon integrate all the features that we offer, but maybe allow to compose smaller installations as well.

How?

With #2144 I have separated the fake implementation from spy. This makes it possible to

  • move the common logic from util into @sinonjs/commons
  • extract the proxy.js, call.js, call-util.js, fake-api.js, ... into a separate module. Maybe @sinonjs/proxy?
  • extract a minimal, generic sandbox implementation that just offers hooks to install things on it, maybe called @sinonjs/sandbox. This would become the sinon default sandbox instance.
  • extract the fake.js implementation into @sinonjs/fake.

There are multiple options how fake, lolex and nise could be registered on the sandbox then.

What do you think?

mantoni avatar Oct 18 '19 10:10 mantoni

I like it. There are large swaths of the API I never use and I find needlessly complex. Having a minimal package would be nice, but I'm not all that confident anyone will use it, beside us 😅

If you can get away with just importing Sinon, instead of having to import 2-3 packages in ever test (yes, you could make a local module that you import instead), then I don't see it as likely it being used ...

fatso83 avatar Oct 18 '19 22:10 fatso83

All I would need from a package would be sandbox and fake.

mroderick avatar Oct 19 '19 08:10 mroderick

I thought about it some more. I think it would make sense to bundle sandbox, fake and lolex. Everything can be done with those 3 cross platform. nise is a browser library and doesn't make sense on Node, so it should be an add-on.

mantoni avatar Oct 19 '19 09:10 mantoni

For me, spy and stub are what make sinon. Why are we keeping fake and sandbox? Are they special? Do we just want to move everything out and make this a meta package?

fearphage avatar Oct 19 '19 13:10 fearphage

@fearphage sandbox is implicitly used all the time since sinon itself is a default sandbox. While spy and stub are the oldest parts of the API, the fakes where introduced later as a unified alternative to spy and stub. One of the key features of the fake API is that all instances have immutable behaviour. Tests that are written with the fake API only tend to be much cleaner. You should give it a try :)

Another reason for a smaller package is to make it easier to learn the sinon API. Just learning the difference between spies and stubs is already confusing, mocks are so complicated that I stopped using them entirely, the fake server API doesn't make sense in the context of Node, re-programming stubs with something like .withArgs("x").returns(true) is hard to reason about and a constant source of issues, ...

Regarding the question if we want to move everyting out: My goal is to have a more lightweight version of Sinon while we still want to maintain the existing project. I don't want to radically change the existing sinon package, because a lot of projects depend on it. Also, I don't want to rewrite a new library. Instead, I would like to make a transition with what we already have. If we extract the bits that are reused between Sinon and the new project (whatever that will be), we can reuse the logic and can maintain both packages.

mantoni avatar Oct 20 '19 11:10 mantoni

Do we have any data about what people actually use in sinon stub/spy vs fake? Obscuring or moving out the thing that people use most frequently in sinon would be a decrease in developer experience.

Another reason for a smaller package is to make it easier to learn the sinon API. Just learning the difference between spies and stubs is already confusing, mocks are so complicated that I stopped using them entirely

I feels like there's more to this statement. What do you believe is difficult now? I've taught sinon to many individually and in classes. The concepts are pretty straight-forward. Do you want people to learn to do things a different way? That's a different goal than stated. It seems that you would prefer people do things this new way, but should they have a more difficult time finding the code, docs, etc. if they do not? Stubs vs spies is two sentences or one question - do you wish to change the behavior? (simplest flowchart). At least that's how I teach it. I'm not sure how it can get any easier than that.

Stubs and spies are industry standard terms. A large number of people familiar with testing know the terms and that terminology is larger than sinon or any individual testing library. Segregating that functionality from sinon just feels a bit off. I'm just expressing my opinion.

fearphage avatar Oct 21 '19 19:10 fearphage

I think there is a missunderstanding here. I don't plan to remove any functionality from Sinon. I want to extract pieces into separate libraries that would still be part of Sinon, just like we separated lolex and nise.

My personal preference is with the fake API and they concur with spy and stub because they solve the same problem (see reasoning for introducing fakes). What I want is a library that doesn't offer alternatives, otherwise it's hard to achive consistency. If I introduce Sinon in a project with the intention to use the fake API, other developers might be tempted to use spy and stub instead and then the team has to learn both APIs.

Another example are mocks. I never use that part of Sinon at all and I wouldn't advise anyone to use them. We even deprecated them once, but had to roll it back. The conclusion at the time was, that mocks should be a separarate library, but nobody wanted to do the work.

And lastly, why do I have a fake XHR library in the test framework when I install it on Node? This also came up before.

Again, I don't want to change the sinon package. We can keep it the way it is. But I'd like to be able to take "the good parts" and compose a new, smaller library. It can have a completely different name, that's fine.

The reason for this issue is to find out what actually makes sense, not to tear Sinon apart ❤️

mantoni avatar Oct 21 '19 23:10 mantoni

I'm just expressing my opinion.

That is very valuable! Please keep doing that!

mroderick avatar Oct 22 '19 09:10 mroderick

I think there is a missunderstanding here.

You're right. I didn't understand the intent. I'm totally on board with what you're proposing.

fearphage avatar Oct 24 '19 03:10 fearphage

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.

stale[bot] avatar Dec 23 '19 03:12 stale[bot]

I’ve just found this issue and would be very interested in having a separate @sinonjs/fake library. Most of the time this is the only thing I need in small projects. Occasionally, in larger projects, I do need stub() to specify different return values based on the arguments (withArgs()) or call count (onFirstCall()).

I was already looking for alternative libraries, but haven’t found a good one of comparable quality.

Is there anything I could help with to move this topic forward?

lo1tuma avatar Jan 12 '22 16:01 lo1tuma

The only thing moving this forward is someone putting in the work 🙂 I don't think it's more than some afternoons of work; it's already modular. Fork, extract one bit, integrate with main repo, extract a new bit, rinse and repeat. Then create a new minimal Sinon core from the pieces.

fatso83 avatar Jan 19 '22 19:01 fatso83