Skip to content
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

Extract sandbox and fake into separate modules #2145

Open
mantoni opened this issue Oct 18, 2019 · 12 comments
Open

Extract sandbox and fake into separate modules #2145

mantoni opened this issue Oct 18, 2019 · 12 comments
Labels

Comments

@mantoni
Copy link
Member

mantoni commented Oct 18, 2019

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?

@fatso83
Copy link
Contributor

fatso83 commented Oct 18, 2019

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 ...

@mroderick
Copy link
Member

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

@mantoni
Copy link
Member Author

mantoni commented Oct 19, 2019

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.

@fearphage
Copy link
Member

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?

@mantoni
Copy link
Member Author

mantoni commented Oct 20, 2019

@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.

@fearphage
Copy link
Member

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.

@mantoni
Copy link
Member Author

mantoni commented Oct 21, 2019

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 ❤️

@mroderick
Copy link
Member

I'm just expressing my opinion.

That is very valuable! Please keep doing that!

@fearphage
Copy link
Member

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.

@stale
Copy link

stale bot commented Dec 23, 2019

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 stale bot added the stale label Dec 23, 2019
@mantoni mantoni added pinned and removed stale labels Dec 23, 2019
@lo1tuma
Copy link
Contributor

lo1tuma commented Jan 12, 2022

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?

@fatso83
Copy link
Contributor

fatso83 commented Jan 19, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants