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

fix(test-helper): Fix Sinon/QUnit Assertion Issue #529

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mwagz
Copy link

@mwagz mwagz commented Nov 11, 2021

This change wraps the pass/fail from sinon.assert in a Qunit.ok assertion. This makes it so that Qunit detects sinon.assert as an actual assertion. sinon.assert methods are not detected as QUnit Assertions. When no qunit assertions exist in the test, Qunit will throw an error:

Expected at least one assertion, but none were run - call expect(0) to accept zero assertions.

This is expected behavior from Qunit, but when we assert with sinon.assert, there is technically assertion there.

This change wraps the pass/fail from sinon.assert in a Qunit.ok assertion. This makes it
so that Qunit detects sinon.assert as an actual assertion.

sinon.assert methods are not detected as QUnit Assertions. When no qunit assertions exist
in the test, Qunit will throw an error:

"Expected at least one assertion, but none were run - call expect(0) to accept zero assertions."

This is expected behavior from Qunit, but when we assert with sinon.assert, there is technically
assertion there.
@@ -11,4 +12,7 @@ import { createSandbox, restoreSandbox } from './sinon-sandbox';
export default function setupSinon(testEnvironment = self.QUnit) {
testEnvironment.testStart(createSandbox);
testEnvironment.testDone(restoreSandbox);

sinon.assert.pass = (assertion) => self.QUnit.assert.ok(true, assertion);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it ok that we are importing sinon here? Should this be something passed in? Should it be self.sinon?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to import sinon here, because it's a required dependency through ember-sinon. Even beyond that, though, we shouldn't have to expect people to use this package if they're not using sinon. That's not a workflow we need to support, so import is fine here!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we import QUnit here or is it required to be grabbed from self for some reason?

Copy link
Author

@mwagz mwagz Nov 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just grabbed it from self because you did on line 12. Which also means we could probably swap it to testEnvironment instead. I'm without opinion, which would you prefer?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, great point. I think we should definitely swap it to testEnvironment to make sure that still works, and then maybe separate from this change we should consider whether we can just do

import * as QUnit from 'qunit';

I don't trust my past self to have made that decision wisely, but maybe there was a reason for it? XD

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened an issue to separate that question from this PR: #530

@elwayman02
Copy link
Owner

Are you sure this works? This open issue on the QUnit project seems to indicate that hooking up sinon.assert and QUnit.assert isn't this easy: qunitjs/qunit#1749

@mwagz
Copy link
Author

mwagz commented Nov 11, 2021

Are you sure this works? This open issue on the QUnit project seems to indicate that hooking up sinon.assert and QUnit.assert isn't this easy: qunitjs/qunit#1749

I can confirm that this fixed the QUnit assertion error outlined in the description by adding it to my local packages tests/test-helper file (as well as some of my team's apps). That particular issue outlines this as a "really easy way to hook it up to QUnit":

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 */ ) {
  // …
};

I think it depends on what you're wanting from the integration. The goal here for me is to not have QUnit yell at us for no assertions when we're using sinon.assert.

Thoughts?

@elwayman02
Copy link
Owner

elwayman02 commented Nov 11, 2021

I'm mostly concerned about the impact of the caveat in that ticket, though I'm not sure I fully understand the implications:

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

@mwagz
Copy link
Author

mwagz commented Nov 11, 2021

I'm mostly concerned about the impact of the caveat in that ticket, though I'm not sure I fully understand the implications:

That's reasonable. Let me do a little digging to better understand that and I'll comment back on this PR with what I can find. :)

@elwayman02
Copy link
Owner

Thanks! The TLDR here is that I fully support what you're trying to do, and I just want to make sure that we're doing it in a way that fully supports how people use Sinon and QUnit today.

@mwagz
Copy link
Author

mwagz commented Nov 12, 2021

Here's what I found out:

Sinon "Sandboxes" remove the need "to keep track of every fake thing created, which greatly simplifies cleanup". I've never looked at this code before, so take this with a grain of salt and a dash of skepticism, please.

https://github.com/sinonjs/sinon/blob/master/lib/sinon/sandbox.js

Sandboxes create a local sandbox.assert from sinonAssert.createAssertObject(), but the docs calls this a "convenience reference" (since [email protected]). If I'm not mistaken, this assert ends up being the exact same sinon.assert.fail and sinon.assert.pass that we are overriding in setupSinon, at least initially.

Krinkle initially linked to v.2.0.1 docs for Sinon, but according to the v11.1.2 docs, that sinon object is a default sandbox since [email protected]. This sort of supports that the solution here does work with the Restorable Sandbox Model, I think? Since if it's working with the version associated with ember-sinon-qunit, then it would be integrating with a sandbox. Lemme know if you think maybe I'm missing something there though (it's totally reasonable, I've only had one cup of coffee this morning 😅).

Lastly, I'm a little unsure about this particular bit of Krinkle's comment:

it would not be easy to connect it to a local assertion object

Not necessarily that it wouldn't be easy, but more about their definition of a "local assertion object". Thoughts on what they're referencing directly? Maybe QUnit.assert? Maybe sinon.assert?

I'm not totally sure where to dig from here, but I think this answers the initial question. Let me know if you want me to dig into something else. I'm learning a ton about sinon with this. 👀

@elwayman02
Copy link
Owner

Yea, local assertion object is what has me tripped up as well. I'm not exactly sure what that means!

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

Successfully merging this pull request may close these issues.

2 participants