Question: global option for Promise implementation to use?

[fstoerkle]

• Sinon version : 2.2.0

Question:
Sinon 2.2.0 introduces stub.usingPromise(), which let's one use a custom Promise implementation on a per-stub basis. This is great, thanks for that! 🎉

My question is: what do you think about providing a global configuration option (e.g. per test file) for specifying a custom Promise implementation?

For example, similar to how sinon-as-promised did it back in the old days: https://github.com/bendrucker/sinon-as-promised#usage

const Bluebird = require('bluebird');
require('sinon-as-promised')(Bluebird);

[mroderick]

I am not opposed to making changes to accommodate per-instance configuration. The proposed changes will lead to a new MAJOR version, as the API will change significantly, which is fine.

Here's what I am thinking: currently sinon is an Object. What you're proposing will lead to sinon becoming a Function that accepts an options Object (let's not paint ourselves into a corner and expect the first argument to be a Promise implementation).

So something like this?

const Bluebird = require('bluebird');
const sinon = require('sinon')({ promiseImplementation: Bluebird});

What do you think? Ping @sinonjs/core

[mantoni]

I think @mroderick's idea to change the Sinon object into a function is great. Having a place to pass configs into a Sinon instance will sure be useful in the future, not only for promise configuration.

However, I think this can be done in a backward compatible way by setting all the current Sinon object properties on the function. I would like to keep the current "stateless" nature and not force everybody to do const sinon = require('sinon')() who wants to current behavior. We already do this with sinon.match, which is a function with additional properties like sinon.match.string etc.

[jonny-improbable]

Another option would be to expose a factory function on the sinon object; ie:

const mySinon = sinon.withConfig({ promiseImplementation: Bluebird });

[mroderick]

However, I think this can be done in a backward compatible way by setting all the current Sinon object properties on the function. I would like to keep the current "stateless" nature and not force everybody to do const sinon = require('sinon')() who wants to current behavior. We already do this with sinon.match, which is a function with additional properties like sinon.match.string etc.

That was what I was thinking, but failed to express.

To follow SemVer, we will still need to publish a new MAJOR version, as the module is not exporting an Object, but a Function. I do not see this as an obstacle.

I am slightly more in favour of changing the export to a factory Function, rather than creating a factory method on the exported Object.

[mantoni]

I have no hard feelings on either solution. @mroderick I agree that changing the object to a function is a major release, but adding the factory function is a minor, right? Wanted to add a label 🏷

[mroderick]

@mantoni that is my understanding too

[lucasfcosta]

Hi, friends, I was working on this a bit yesterday and I came up with a few moments of indecision and I'd like to explain my thoughts to you so you can approve what I'm doing before I submit a PR.

1. Since this is a property that affects the whole library, it must become part of it's "state" and therefore we need to have this available whenever using sinon. However, the way we expose Sinon's parts (collections, spies, stubs, etc) is by adding them to the object we export and so we're just providing our users a bunch of methods that do not access this state, as you can see in this file. This makes us not able to simply set a property on the object we're exporting: none of the functions added to it is aware of this object, this might cause troubles and make the code harder to read and track where things are coming from in the future when we have other config keys.
2. Creating a property in the global object indexed by a symbol avoids namespace clashes and we could make this symbol available through the current defaultConfigs so that we can get stuff from it whenever we need. However, I think this approach is not optimal because it opens up a great possibility of memory leaks (since garbage colletor will always keep global[sinonSymbol] alive.
3. Creating a separate module with sinon configs would be great, because given the way require currently works (it looks for an instance of the required file in cache and returns it if exists) we can simply get the same configs instance whenever we require sinonConfigs. However, in order to implement this, I think the best option would be to use the existing getConfig, but it is being listed as deprecated when we exposeCoreUtils.
   By using getConfig (renaming it to withConfig) we could simply overwrite the default globalPromise prop and use it whenever we want because the defaultConfig object would be cached and always be the same in the future.

Also, do we plan on returning a new Sinon instance whenever we call withConfig or do we want to return the same instance and just change its configs?

[luislobo]

+1 for this feature. I'm running some issues trying to use bluebird + sinon 1 + sinon-as-promised latest versions with an error about:

TypeError: Cannot set property 'resolves' of undefined
>>   at Object.<anonymous> (/home/lobo/dev/petrocloud/pc3/node_modules/sinon-as-promised/index.js:14:25)

That is because Sinon 2 has no resolves, as sinon 2 supports native promises.

[Druotic]

Copying from pull request #1542 to keep everything in one place:

fatso83:

I just wanted to ensure I understand you correctly in what you want to achieve before letting this go: I assume you wanted to change the sinon instance in one test file and have it automatically propagate to the rest of your test code. Am I right? If so, I think this is exactly what we are trying to avoid. Testing is best done when the code is explicit, and configuring a sinon instance that will work as advertised for that one specific file is usually what we strive for.

As to simplicity, I don't see all the negative downsides of doing:
const sinon = require('sinon')({promiseImplementation: bluebird})
in each of the test files you need to change this. Am I mistaking what the problem is? I think I might, as I am not sure I got what you meant by "override the require cache". I think you meant that if you wanted to reuse that same instance in all of your different test files then you would need to hack the require cache, but that seems more trouble than it's worth compared to simply passing in a config object.

druotic:

@fatso83 Nope, you're correct. In our case, we use Bluebird promises exclusively (though, that'll change when async/await/node 8 goes LTS in October). In our previous setup before upgrading to sinon 3, we just required sinon-as-promised before running our tests, and all future stubs/mocks used bluebird promises. We wanted to maintain that behavior since we never want to accidentally use native promises.

We could explicitly pass in the promise implementation in every test file where we need it, but that would be 100% of test files in our case. If another dev forgets to pass it in, that could lead to broken behavior/bad assertions. It'd be nice to be able to just specify it once since we never want to use native promises.

edit: Maybe there could be a setDefaultConfig function that clearly indicates by the name that it has global impact and that people shouldn't use it on a per-test basis? It feels like sinon.addBehavior could live there as well since it has global impact - e.g. customStubBehaviors: [ { 'returnsNum': (fake, n) => fake.returns(n) }, ... ]

[fatso83]

@lucasfcosta I would really like to land this to avoid bumping the major version twice in short succession (due to #1557), so if you have code to support a configurable Sinon instance, I think a PR would be most welcome. As I saw there was a bit of confusion, I'll touch on most of your points below.

As to 1 (regarding state), yes, we would have to pass in a config object to the various functions in order to support different instances of sinon, and this requires a bit of change in the underlying modules. Not a lot, though, as the amount of config that can be changed today pretty much amounts to promises, I think. So it would probably be a lot of passing the config into various places, but not necessarily making use of it everywhere.

However, in order to implement this, I think the best option would be to use the existing getConfig, but it is being listed as deprecated when we exposeCoreUtils.

getConfig is not deprecated. It is only the exported function that was deprecated from the public API. It is totally fine to keep on reusing it internally. We just didn't want external libs to depend on its behavior.

Also, do we plan on returning a new Sinon instance whenever we call withConfig or do we want to return the same instance and just change its configs?

I think the clearest option here is to return a new instance for each config, but perhaps caching the default instance, as it will probably be used in most cases. It's up to the clients to cache the instance of they feel they need to (say, as an export in my-sinon.js). As for performance, most of the time is probably used in system calls to the file system (via require), so caching individual instances is probably a micro-optimisation.

Another thing, is that from the discussion above, I think we landed on making sinon a factory, instead of adding a factory method, so withConfig is out the door 😸 As Phred sayd in the group discussed on Twitter:

require('sinon') should have the same api and properties as require('sinon')(config).

[fatso83]

P.S. We might consider moving the discussion to a separate issue, as this one deals with something entirely different, although dependent on the discussed change being present.

[HyperBrain]

@mroderick Regarding the factory that would be attached to the sinon object to get a configured instance instead of the currently available object, I think it should be done similarly to the sandbox naming.

So I just would use

const mySinon = sinon.create(config);

This is much more intuitive than introducing a different factory function name here.

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