Integrations: Sinon with QUnit

[Krinkle]

Another one for the "Integrations" section (ref qunitjs/qunitjs.com#42), we can promote use of SinonJS.

Perhaps a very plain and simple way, could be like so:

const { test } = QUnit;
const sandbox = require('sinon').sandbox.create();

QUnit.module('example', hooks => {

  hooks.afterEach(() => {
    sandbox.restore();
  });

  test('sinon stub', assert => {
    const spy = sandbox.stub(console, 'log');

    console.log('foo');

    assert.true(spy.calledOnce);
    assert.true(spy.calledWith('foo'));
  });
});

With Sinon 5 and later, the sandox creation is optional per https://sinonjs.org/guides/migrating-to-5.0 so it could even simpler:

const { test } = QUnit;
const sinon = require('sinon');

QUnit.module('example', hooks => {

  hooks.afterEach(() => {
    sinon.restore();
  });

  test('sinon stub', assert => {
    const spy = sinon.stub(console, 'log');

    console.log('foo');

    assert.true(spy.calledOnce);
    assert.true(spy.calledWith('foo'));
  });
});

Based on anecdotal searches in the wild, the above seems to be how most people use Sinon with QUnit.

However, the above would not benefit from Sinon's descriptive error messages. That's why often people avoid Sinon's boolean shortcut methods like calledWith() in favour of performing native QUnit assertions on the spy object, like so:

// assert.true(spy.calledWith('foo'));
const call = spy.getCall(0);
assert.equal(call.args[0], 'foo');

But, this isn't a great developer experience and means most documentation and tutorials out there for Sinon won't feel applicable. Instead, we can use Sinon.assert and let it throw descriptive exception messages. This is what https://sinonjs.org/releases/v9.0.3/sandbox/ recommends:

const { test } = QUnit;
const sinon = require('sinon');

QUnit.module('example', hooks => {

  hooks.afterEach(() => {
    sinon.restore();
  });

  test('sinon stub', assert => {
    const spy = sinon.stub(console, 'log');

    console.log('foo');

    sinon.assert.calledOnce(spy);
    sinon.assert.calledWith('foo');
  });
});

This seems much better for the developer, but it is by default optimises for exception-based test frameworks like Mocha and JUnit, which stop after the first error. QUnit won't see the exceptions as assertion failures so they would technically appear as unexpected exceptions. Which isn't so bad by itself as the UI for that is mostly the same, but it might also trigger "zero assertions" warnings if the test has no other assertions, and that's kind of a deal breaker imho.

Sinon provides an extensible assert API for that reason, so that you can really easily hook it up to QUnit. Per https://sinonjs.org/releases/v9.0.3/assertions/:

sinon.assert.pass = function (assertion /* string */ ) {
  // …
};
sinon.assert.fail = function (message /* string */ ) {
  // …
};

The thing is, these are globally static and not part if Sinon's restorable sandbox model. Which means even with before/after hooks, or Sinon's sandbox.injectInto feature, it would not be easy to connect it to a local assertion object. (Aside from using global QUnit.config.current.assert.)

But, there is a dedicated sinon.assert.expose(obj); method that does what we need by providing the assertion methods with support for local pass and fail callbacks. Below is a hacky way to connect that, as demonstration:

hooks.beforeEach(assert => {
  assert.sinon = {};
  sinon.assert.expose(assert.sinon, { prefix: '' });
  assert.sinon.pass = (message) => assert.pushResult({ result: true, expected: true, actual: true, message });
  assert.sinon.fail = (message) => assert.pushResult({ result: false, expected: true, actual: false, message });
});

  test('sinon stub', assert => {
    const spy = sinon.stub(console, 'log');

    console.log('foo');

    assert.sinon.calledOnce(spy);
    assert.sinon.calledWith('foo');
  });

This seems to provide the best of both worlds, although the setup is obviously impractical, so we'd want to ship that in a plugin that handles this automatically. There are numerous sinon-qunit plugins out there we could work with, perhaps some of them even do this already?

[Krinkle]

/cc @PrecisionNutrition

This ticket be of interest to you. I noticed your package at https://github.com/PrecisionNutrition/qunit-sinon-assertions, which seems to go further and even make the comparisons native to benefit from diffing. Awesome.

It seems to still be a work in progress, with Ember CLI dependencies bundled. Is that right? I'd love to promote it on qunitjs.com when ready!

[trentmwillis]

FWIW, I've used Sinon a lot and usually just call Sinon directly without doing any real integration with QUnit.

[elwayman02]

/cc @PrecisionNutrition

This ticket be of interest to you. I noticed your package at https://github.com/PrecisionNutrition/qunit-sinon-assertions, which seems to go further and even make the comparisons native to benefit from diffing. Awesome.

It seems to still be a work in progress, with Ember CLI dependencies bundled. Is that right? I'd love to promote it on qunitjs.com when ready!

@Krinkle it seems they just released 1.0 a few weeks ago :)

[elwayman02]

@Krinkle we've started trying to tackle this issue here: elwayman02/ember-sinon-qunit#529

The initial implementation just clobbers the pass/fail methods and calls QUnit.assert.ok. However, I'm wary of this caveat you called out:

The thing is, these are globally static and not part if Sinon's restorable sandbox model. Which means even with before/after hooks, or Sinon's sandbox.injectInto feature, it would not be easy to connect it to a local assertion object. (Aside from using global QUnit.config.current.assert.)

Can you explain the implications of this? I'd like to make sure that we're properly handling this integration in a way that makes sense for established QUnit and Sinon paradigms.

[mwagz]

@Krinkle Would you care to elaborate on what you're referencing by local assertion object? Is that QUnit.assert? sinon.assert? We've done a little digging in elwayman02/ember-sinon-qunit#529, but we're not certain what you mean for that.

Thanks!

[Krinkle]

@mwagz By "local assertion object" I was referring to the assert parameter given to each test function, as run via QUnit.test.

You might already know the next bit, but I'll add it for context:

• Each assert object "knows" the test it belongs to.
• There was a time where this wasn't the case (QUnit 1, ref Upgrade Guide). Introducing test and assert context made setup and teardown more reliable, and improved dev feedback and dev UX around async testing.
  For example, if you pass a function with assertions in it from a test case to application code, but did not attach the thing that runs the function to a chain of promises, async-await, or assert.async(), then the test may "succeed" before it runs, and it may end up running later during a different test.
• The context is how we determine if an assertion is made out of turn, and respond with an error message. This later gave us the confidence to de-mphasize assert.expect() from docs and examples. I believe well-written tests now generally can't succeed with unrun assertions, or with assertions from a previous test. These were common issues that the crude assertion counter was trying to detect.

If I understand correctly, the PR you linked would be calling QUnit.assert.ok(). That is essentially calling the internal Assert.prototype.ok() prototype function directly, without any this context. That happens to work today for some of the older and more basic assertions, like ok() and strictEqual(), because we never removed the QUnit 1 compatibility code from them. This has allowed some of the more popular QUnit 1.x plugins to keep working. Calling assertions in this kind of static way is not documented or officially supported, but it has allowed some things to internally keep working nicely, and so we haven't bothered removing it. Note that this pattern already fails for assert.async, assert.step, or assert.timeout.

The recommendation I hinted at, in my initial thought dump at the start of this issue, would be to either:

• Monkey-patch QUnit.module() such that you automatically call beforeEach and afterEach for all future module and tests. This is not practical in a modern import/require-based workflow.
• Call a setup function at the top of each module. This is what ember-qunit does for several of its features.

Then, from inside the beforeEach you can assign this.sinon with a Sinon sandbox that reports to the current assert object (which is available from beforeEach).

I'll also prioritize #1475 further, which would allow Ember to register its beforeEach hook universally, without requiring a function call in every test module.

[elwayman02]

Hey @Krinkle just wanted to check in to see how progress was coming on this end. We're super excited about this work!

[Krinkle]

@elwayman02 Thanks for the poke. I spent today figuring out different approaches and settled on one. I've jiggled around some code in preparation for this, and hope to land and release the rest this coming week.

[jherdman]

Hey friends. I know this is a bit of an old issue, but I'm one of the devs that manages the qunit-sinon-assert packages. I wasn't being paged! I'll try and read over this issue later this week to see if there are improvements we could/should make to our package.

[Krinkle]

@jherdman Thanks! I've since released the aforementioned global hooks. Docs at https://api.qunitjs.com/QUnit/hooks/. Let me know if you run into any issues.

[raycohen]

Looks like sinon does not currently allow the sinon.expose(target) target to receive successful assertion information, only failures:

https://github.com/sinonjs/sinon/blob/6c4753ef243880f5cdf1ea9c88b569780f9dc013/lib/sinon/assert.js#L94-L104

So QUnit will not know when things pass, only when they fail.