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

Integrate sinon's assertion API into QUnit #223

Closed
wants to merge 2 commits into from

Conversation

sukima
Copy link

@sukima sukima commented Sep 11, 2019

Fixes issue #222

This is a big win as sinon's assertion API is far superior, more readable, and offers better messaging then doing so manually.

Fixes issue elwayman02#222

This is a big win as sinon's assertion API is far superior, more readable, and offers better messaging then doing so manually.
@elwayman02
Copy link
Owner

A couple issues we may want to revisit before we merge this:

  1. Is the current config correct? Is it a reasonable default? For example, with useFakeTimers set to false, could that break someone's tests as they're migrating to this addon? Is the properties array still correct?
  2. It seems reasonable that people may want to customize the config for their app. We would have to make setupSinon accept a config as a parameter.

Thinking this through even further, we may want to just let people configure sinon on their own, since one of the big pushes in this rewrite was to eliminate maintenance of a proxy API and let people use the sinon object directly. Can we extract the assertion integration into a separate file and only use that util in setupSinon, leaving the "default config" to the deprecated feature?

We should also update the README to reflect both the assertion integration (linking back to sinon's guide seems appropriate here) and the expectation of how people should set up their sinon config if they're migrating from the deprecated features where it was done for them.

@elwayman02
Copy link
Owner

@scalvert do you have any thoughts on this?

@elwayman02
Copy link
Owner

@sukima I'd love to move forward with this PR still, if we can come to a resolution on the open questions above.

@sukima
Copy link
Author

sukima commented Jan 23, 2020

I'm confused. You said the project wants people to Do It Themselves ™️. The lack of sinon-qunit support has been a long standing issue for many years now. I've relinquished to just doing this in all my projects:

import Application from '../app';
import QUnit from 'qunit';
import config from 'dummy/config/environment';
import sinon from 'sinon';
import { setApplication } from '@ember/test-helpers';
import { start } from 'ember-qunit';

sinon.assert.pass = (assertion) => QUnit.assert.ok(true, assertion);
sinon.assert.fail = (assertion) => QUnit.assert.ok(false, assertion);

setApplication(Application.create(config.APP));

start();

Four lines of code. Easily repeatable. Fixes every grievance for me. Maybe a README section explaining this.

I also have never ever needed any test wrapper, ever. Then again I don't go mocking globals willy-nilly and if I do I make it very intentional with an explicit afterEach restore. So maybe I'm a unique case.

Given the above and that ember comes with ember-auto-import by default there is no longer any need for me to have an specific addon. Sorry.

@elwayman02
Copy link
Owner

@sukima I'm not sure I understand how your last comment relates to the above discussion. Can you please clarify?

@sukima
Copy link
Author

sukima commented Feb 17, 2020

@elwayman02 After some clarity I think I was conflating a couple of separate conversations in the comments I had.

  1. ember-sinon-qunit was lacking a sinon.assert solution
  2. ember-sinon-qunit was focused on a test wrapper

My comments conflated the two. I have found that in all my use cases in many apps for quite some time never needed 2, And that all I needed was support for 1. Given that I can accomplish 1 in my apps with the above code snippet I was feeling kind of frustrated. For that I apologize.

As for this PR. After much thought I feel that sinon.asert integration is not a config concern. This PR uses it as such because as written the commonConfig has the side effect of integration but is not the main purpose. Honestly it has been so long and the fact that in all my apps I have yet to discover a need for a setupSinon especially given sinon's very flexible API. All I need is that small boilerplate code I referenced above. Basically if people want to use an addon fine for them but I assert that a sinon addon is unnecessary as sinon works out-of-the-box with the use of ember-auto-import (now default in Ember blueprints).

@sukima sukima closed this Feb 17, 2020
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