Skip to content
This repository has been archived by the owner on Jun 9, 2020. It is now read-only.

Add support for testdouble-chai #21

Merged
merged 3 commits into from
Dec 20, 2016

Conversation

alexlafroscia
Copy link
Contributor

@alexlafroscia alexlafroscia commented Dec 15, 2016

TODO

  • Figure out how to test both testdouble-chai and sinon-chai, when both can't be enabled
  • Test in dummy app

Closes #19

@alexlafroscia
Copy link
Contributor Author

Thinking about the fact that they conflict... Do you think it would be possible to patch one/both of sinon- and testdouble-chai so that they support both being enabled? It feels like something that should be possible, but I don't know a ton about how either of them work.

@Turbo87
Copy link
Member

Turbo87 commented Dec 15, 2016

@alexlafroscia I've asked a very similar question in nathanboktae/chai-dom#16 and we've concluded that it doesn't seem possible with the current API that Chai has for addons.

As you can see in the lack of tests for chai-dom I haven't yet figured out how to test these two either. If you have some ideas on how to do it I'd like to hear them. For the time being we should test those mutually exclusive plugins manually and try to figure out a way to automate it.

Can you adjust the PR to only add support for the testdouble stuff for now but not include the failing test?

@alexlafroscia
Copy link
Contributor Author

Yup, absolutely.

@@ -49,12 +49,19 @@ var sinonPlugin = {
path: 'sinon-chai.js',
};

var testdoublePlugin = {
name: 'testdouble-chai',
constraint: '^0.5.0',
Copy link
Member

Choose a reason for hiding this comment

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

what about the releases below 0.5.0? they don't work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK they all "work" but 0.5.0 removed testdouble as a peer dependency, which caused some issues with ember-cli-testdouble-chai because testdouble was provided by ember-cli-testdouble.

@alexlafroscia
Copy link
Contributor Author

alexlafroscia commented Dec 15, 2016

I think I might need to add some code here to handle bootstrapping testdouble-chai

https://github.com/BaseCase/testdouble-chai#setup

ember-cli-testdouble-chai adds to the test-helper.js file, but it would be great to avoid the need for users to do that themselves, it should "just work".

@Turbo87
Copy link
Member

Turbo87 commented Dec 15, 2016

indeed, their UMD wrapper doesn't seem to call chai.use() automatically. I had to do something similar for chai-as-promised, but that also included using rollup and other things which might not be necessary here.

@alexlafroscia
Copy link
Contributor Author

Yeah, I'm thinking we can just include a file in the addon's vendor folder that gets pulled in when testdouble-chai is present.

@alexlafroscia
Copy link
Contributor Author

Just found out about the overwriteMethod option that plugins can use -- I'm surprised that doesn't work for this sinon/testdouble case.

@Turbo87
Copy link
Member

Turbo87 commented Dec 17, 2016

Yeah but you can only overwrite existing methods, if you overwrite an unexisting method it won't work :-/

@alexlafroscia
Copy link
Contributor Author

Not sure if you had something else in mind, but I ended up adding an additional, option property to the plugin format that can specify an additional file that needs to be loaded. I made a dummy app that was linked to my local version of ember-cli-chai and was able to get everything working in there through this setup.

Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

thanks :)

@Turbo87 Turbo87 merged commit bdd1e2c into ember-cli:master Dec 20, 2016
@Turbo87
Copy link
Member

Turbo87 commented Dec 20, 2016

released as v0.3.2 👍

@alexlafroscia
Copy link
Contributor Author

As a note, I added a notice to the ember-cli-testdouble-chai README that mentions the fact that it's now deprecated.

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

Successfully merging this pull request may close these issues.

2 participants