Skip to content
This repository has been archived by the owner on Dec 27, 2018. It is now read-only.

Conflict with dispatch:mocha-phantomjs? #23

Closed
gadicc opened this issue Mar 31, 2016 · 20 comments · Fixed by #50
Closed

Conflict with dispatch:mocha-phantomjs? #23

gadicc opened this issue Mar 31, 2016 · 20 comments · Fixed by #50

Comments

@gadicc
Copy link

gadicc commented Mar 31, 2016

I'm running like this:

$ meteor test --driver-package practicalmeteor:mocha

And everything works. But if I add dispatch:mocha-phantomjs to my .meteor/packages, with no other changes, server tests no longer run, I get:

passes: 0  failures: 0  duration: 0s

In case this doesn't affect all cases, there's a repro at gadicc/meteor-blaze-react-component@6a0e529.

@rbabayoff
Copy link
Member

@gadicc, yes, we don't co-exist nicely, since both of us are adding mocha independently. I suggest you wait a couple of days until we get support for 1.3 in spacejam and practicalmeteor:mocha-console-runner.

Long term we hope to create a pr into mocha with the meteor specific support of having multiple html reporters to eliminate those things altogether, so you could just npm install mocha independently of any of the two packages.

@aldeed
Copy link

aldeed commented Apr 18, 2016

@rbabayoff I now have all the dispatch:mocha-* packages depending on dispatch:mocha-core, which loads the client side file and exports a single instance of mocha on the server for them all to share. This allows them all to be added without conflicts. I haven't look into it yet, but I believe practicalmeteor:mocha could also be updated to depend on dispatch:mocha-core and then everything would not conflict. I can give you access to dispatch:mocha-core. There isn't much there except that it has the mocha npm dependency, so we would have to bump that version occasionally.

trajano added a commit to trajano/guide that referenced this issue May 3, 2016
If `practicalmeteor:chai` is used we are locking into the `practicalmeteor` ecosystem.  However if we keep to npm chai we can use other test providers.  See practicalmeteor/meteor-mocha#23 and https://github.com/xolvio/meteor-jasmine/issues/334#issuecomment-208725856 for other reasons.
trajano added a commit to trajano/guide that referenced this issue May 3, 2016
Rather than practicalmeteor:chai, use the npm versions.  This allows us to
ensure we can get the latest versions possible of the libraries and allows
us to do tests using other test packages.

See practicalmeteor/meteor-mocha#23 and https://github.com/xolvio/meteor-jasmine/issues/334#issuecomment-208725856 for other reasons.
@tmeasday
Copy link
Collaborator

tmeasday commented May 4, 2016

@rbabayoff what do you think about trying to settle on a single mocha core package?

@rbabayoff
Copy link
Member

@tmeasday I agree, but I'd prefer to use ours. PRs are welcome, 2 options:

  1. Create the mocha instance in our mocha-core and add dependency on npm.
  2. Have dispatch depend on practicalmeteor:mocha and use the exported mocha instance from our package. Preferred.

@aldeed and @tmeasday what say you?

@tmeasday
Copy link
Collaborator

tmeasday commented May 5, 2016

I'm really not sure what the difference between the core packages are? Why would you prefer to use yours @rbabayoff?

@aldeed
Copy link

aldeed commented May 5, 2016

@rbabayoff Our core pkg already depends on your mocha-core to do the binding, so if your mocha-core just exported the mocha stuff already bound on the server and did init mocha on the client, that's my preference. In other words, we can merge ours into yours. I can do a PR but probably not until next week.

@aldeed
Copy link

aldeed commented May 19, 2016

@rbabayoff @tmeasday Here is a PR for combining the pkgs: practicalmeteor/meteor-mocha-core#6

@jsep
Copy link

jsep commented Jun 9, 2016

Hello @tmeasday @aldeed @gadicc

I just release a release candidate version (2.4.5-rc3.1) of the package using practicalmeteor:mocha-core with the changes of @aldeed

To use it just run meteor add practicalmeteor:mocha@2.4.5-rc3.1

Please feel free to test the changes and let us know any problem.

Thanks

@jsep
Copy link

jsep commented Jun 9, 2016

@aldeed which version of your package it's using the same practicalmeteor:mocha-core package?

@aldeed
Copy link

aldeed commented Jun 9, 2016

Our packages dispatch:mocha, dispatch:mocha-phantomjs, and dispatch:mocha-browser all use our dispatch:mocha-core, which uses practicalmeteor:mocha-core to do the server wrapping. I will update those three packages to use practicalmeteor:mocha@2.4.5-rc3.1 directly, and then dispatch:mocha-core will be deprecated. I will try to get our own RCs out for that so that we can verify that everything plays nicely together.

@jsep
Copy link

jsep commented Jun 9, 2016

@aldeed the version of practicalmeteor:mocha-core that I'm using is 1.0.0-rc.2.

@Ben305
Copy link

Ben305 commented Jun 9, 2016

@aldeed Can you post here again when the RCs are out?

@rbabayoff
Copy link
Member

@gadicc, @aldeed and @tmeasday, as soon as you confirm it's working, we'll publish this as a release version.

@abhiaiyer91
Copy link

When is this being resolved? @tmeasday

@tmeasday
Copy link
Collaborator

Just waiting to hear from @aldeed .

You can just use the dispatch reporter in the meantime if it's slowing you down @abhiaiyer91

@gadicc
Copy link
Author

gadicc commented Jun 16, 2016

Sorry for my late replies to this... I'm a bit behind and also have some weird new issue with my testing setup that I've run out of time to fix. In any event, I don't think I can test this without an updated dispatch:mocha-phantomjs, and I think as soon as they give the final OK you can consider this resolved. Many thanks! Will be awesome to have both meteor test and CI working with the same config.

@jsep jsep mentioned this issue Jun 17, 2016
@jsep jsep closed this as completed in #50 Jun 18, 2016
jsep pushed a commit that referenced this issue Jun 18, 2016
Version 2.4.5_3. Various improvements and bug fixes. Fixes #23 #42 #44 #45.

See new Changelog for details.
@maciej-trebacz
Copy link

@tmeasday If this is fixed you may want to update the guide to remove this line:

NOTE: Currently tests may not run properly if you add both a practicalmeteor:* package and a dispatch:* package to your app. This is being fixed. See this issue.

@rbabayoff
Copy link
Member

@aldeed let us know if this works for you and if not, we'll reopen this.

@aldeed
Copy link

aldeed commented Jun 23, 2016

@rbabayoff @jsep I published new releases of all the dispatch:mocha* packages using practicalmeteor:mocha-core@1.0.0. As far as I can tell, everything is working well and there are no longer conflicts between the packages.

@tmeasday Here is a PR updating todos app: meteor/todos#153 And the note in the guide should be updated as mentioned by @m4v3r above. If the latest release of all mocha packages is used, there is no conflict.

Thanks all!

@jsep
Copy link

jsep commented Jun 23, 2016

Those are great news, thank you so much @aldeed !

trajano added a commit to trajano/guide that referenced this issue Mar 12, 2017
Rather than practicalmeteor:chai, use the npm versions.  This allows us to
ensure we can get the latest versions possible of the libraries and allows
us to do tests using other test packages.

See practicalmeteor/meteor-mocha#23 and https://github.com/xolvio/meteor-jasmine/issues/334#issuecomment-208725856 for other reasons.
NatashaKSS pushed a commit to nus-mtp/nus-oracle that referenced this issue Apr 7, 2017
* mocha-phantomjs and practicalmeteor:mocha do not play well with each
other, refer to existing issue
practicalmeteor/meteor-mocha#23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants