Migrate sinon-test from sinon package.

[jonnyreeves]

• sinon.test
• sinon.testCase
• Update README

Extract sinon.test and sinon.testCase from the current sinon 2.0-pre package. Modify both methods to be independent of sinon.config.

Still need to get the test-case tests back on their feet (they stubbed on sinon.test so are a little tricky to migrate) - but otherwise it's taking shape.

Please let me know your thoughts about the proposed API.

[cjohansen]

Looking good! I have some thoughts:

The dependency on Sinon is unfortunate, as it will require constant republishing of this package. I understand the appeal of var sinon.test = require('sinon-test'), but catering to convenience is what introduced the problematic sinon.config and other singletons in the first place. Besides, it is very unfortunate that var sinon.test = require('sinon-test') will make sinon and sinon.test use potentially different versions of Sinon.

I suggest the following API instead:

sinon.test = require('sinon-test')(sinon);
sinon.test = require('sinon-test')(sinon, config);

or, more explicit:

sinon.test = require('sinon-test').configure(sinon);
sinon.test = require('sinon-test').configure(sinon, config);

But then, how to access testCase? Maybe:

sinon.testCase = require('sinon-test').configureTestCase(sinon, config);

Other suggestions?

Now that we're using browserify, do we need all the closures and scaffolding in the modules? Couldn't we just make them regular CommonJS modules?

[fatso83]

Agree with removing the explicit sinon dependency. With regards to injecting, I prefer the first alternative: sinon.test = require('sinon-test')(sinon, config);. Then you could throw an error on require if you fail to pass a sinon instance, something which is not done with the configure version.

[cjohansen]

How can you throw an error on require? I don't see the difference:

sinon.test = require('sinon-test');

sinon.test(); // This can throw a predefined error regardless of the chosen API

[fatso83]

I was struck by temporary retardedness 😭 You are absolutely right.

[mantoni]

I think the ideal solution requires sinon-core and maybe even sinon-sandbox to be standalone so that this module can depend on them. Then there is no need for injections and potential incompatibilities.

[cjohansen]

I'm not sure I agree. Those modules would still come with the version conundrum. I think injection is the ideal solution, as it would allow sinon-test to be slapped with a 1.0.0 and basically never be touched again. It's been stable for years.

[mantoni]

Well, I was thinking of using the default npm versioning and depend on ^1.0.0 which essentially means all minor and patch versions are fine. In case of a major version change, two major versions could even co-exist - at least in node and browserify. With the injection, the API dependency is implicit and it's not possible to upgrade one Sinon module without also upgrading the others.

One example might help to illustrate my point:
Imagine this module would depend on sinon-sandbox and we would release a major version of that library. A Sinon user could opt-in to use the new sandbox version without the need to also upgrade sinon-test, which could still depend on ^1.0. If the breaking change does not affect users of sinon-test, we're even free to release a minor version to upgrade to ^2.0 without forcing this version on users.

On the other hand, with the injection, a major upgrade of either module most likely requires the other one to be upgraded too.

[cjohansen]

I still think the problem that if using sinon and sinon-test alongside each other, it will be difficult to ensure all areas of your test suite are using the same version of sinon (or smaller constituents, but that's beside the point). Also, if sinon-test depends on any other Sinon module, it will require meaningless version bumps and pushes everytime a dependency fixes a bug etc. This is busy work we should strive to avoid.

With injection, sure there might be problems when sinon and/or sinon-test have major version upgrades, although I don't expect our public APIs to change on a very regular basis. There is for example little reason for the sandbox API to change. All in all, injection offers the most flexibility and predictability while also reducing busy-work on our side, while having very good odds for staying quite stable.

[mantoni]

Sorry @cjohansen, but I think you misunderstand my suggestion. If we use carets in the version numbers, e.g. ^1.0.0, then there is no meaningless busy work going on. Any bumps would propagate implicitly with the exception of a breaking change.

[jonnyreeves]

Gents, I have updated the API so it no longer has an NPM dependency on sinon; however the packaged tests are now failing as (I believe) they're picking up the sinon instance shipped with buster that gets exported onto the window which causes three of the tests to fail.

Any input would be greatly appreciated; I would love to just bundle up the tests themselves using browserify (and thereby not have to reference the window object in any of the tests); but I have no idea how to get buster and browserify to play nice :( Are there any buster experts in the room (paging @mroderick )

[jonnyreeves]

Anyone? Can't help but think things would be much simpler to test if were using a less opinionated test-runner (like mocha) instead - would there be any objections to me migrating these tests over? Then I can just browserify all the tests and run them through phantom.

[mroderick]

@cjohansen is the buster expert ;-)

I am returning from vacation and still have tourist mode engaged. Once I am back to daily life, I can look into the discussion of test runners

Anyway, I think we should discuss test runners in it's own thread

[jonnyreeves]

Thanks for the reply @mroderick. So after my rant above I set out to figure out how to fix this; and hopefully I now have a working solution.

The browserify-shim package allows us to replace require() calls with global references; this allows us to require sinon modules but not buster.

[jonnyreeves]

Hey all, so the tests are passing on travis, but the Node 0.8 build is hanging / failing on buster; I copied the config straight from the sinon repo; anyone have any idea what's causing this? Any objections to turning off the 0.8 build?

[fatso83]

Hmm ... I restarted the 0.8 build, and it still failed. I do not really see why we should keep on to the 0.8 version of Node as it is almost 4 years old.

[cjohansen]

My opinion on two issues raised:

1. Do we have to use Buster? IMO, no. As long as npm test does something useful I don't care too much about what is powering it. As we're now using Browserify (yay!) there are probably better options than Buster. I also don't think we need to use the exact same stack on every module. Personally, I have had decent luck with @mantoni's mochify, combined with referee (Buster's assertions).
2. The only 0.x node we should spend time on is 0.12. We can keep 0.10 around for as long as it doesn't cause problems, but honestly, I don't think it's worth the trouble if/when it stops working.

[mantoni]

+1 for removing old node support. It goes without saying, if we're switching to Mochify I'm keen to support whatever is needed in the Sinon context.

[jonnyreeves]

OK, dropped Node 0.8 builds from Travis and everything is green. This PR is ready for review, please take a look.

[fatso83]

LGTM 👍

[mroderick]

Do we need to keep all this boilerplate in all files? If we're providing a built version using Browserify, can't we just add a banner during the build step?

[jonnyreeves]

Hmm, I was under the impression this was the copy's standard copyright / description block. I have no issue with leaving it in, or taking it out - just say the word :)

[mantoni]

I'm not sure whether it's good enough to prepend this in the browserified version. This is also shipped and executed as-is in node.

[fatso83]

I have no idea why it's there in the first place. We do have a LICENSE, so repeating it in every file seems superfluous. It should be enough with a short description of the module. The rest is redundant.

[mantoni]

Fair point.

[mroderick]

Other than the comment about the boilerplate, I have no comments. Excellent work @jonnyreeves

[jonnyreeves]

Gents, anything stopping this one from landing? Once it's merged we can merge sinonjs/sinon#973 and I can get the ball rolling on migrating all the sinon tests over to using CommonJS.

[cjohansen]

👍 Sorry for the delay.