search

AmpersandJS / ampersand

Quickest way to get started with ampersand.
MIT License
812 stars 41 forks source link

add pretest script to warn (or cleanup) phantomjs browser-launcher #35

Closed bear closed 10 years ago

bear commented 10 years ago

Aria in IRC reported running into the same issue as reported here

https://github.com/AmpersandJS/ampersand-collection-view/issues/13

It seems like we should warn (or cleanup) about this before making folks think their tests are failing.

See the IRC log for http://static.andyet.com/irclogs/%26yet-2014-08-20.html

19:57:30 Hm. Trying to get the test suite to run for ampersand-view and it just exits with: no matches for phantom/* 19:57:30 Fail! 19:57:33 Any hints? 20:00:09 that's odd - phantom isn't used/referenced by that repo's test suite 20:01:43 That's what I said. 20:01:45 "that's odd" 20:01:49 lukekarrys ^ 20:01:52 I'm just running npm test 20:01:53 i'm pulling a fresh copy of the repo 20:02:20 tape-run defaults to phantom 20:02:49 one sec, I think theres a reference in an issue to that error 20:02:55 let me find it

bear commented 10 years ago

@lukekarrys has found that a bug exists for browser-launcher:

https://github.com/substack/browser-launcher/issues/28

lukekarrys commented 10 years ago

Ideally that browser-launcher bug could get fixed. But even in that case we should have a message stating that phantomjs is required globally to run the tests.

kamilogorek commented 10 years ago

Here Michael https://github.com/AmpersandJS/ampersand-collection-view/issues/13 :)

On Wed, Aug 20, 2014 at 11:32 PM, Michael Garvin notifications@github.com wrote:

I made an issue very similar to this somewhere but couldn't find it.

— Reply to this email directly or view it on GitHub https://github.com/AmpersandJS/ampersand/issues/35#issuecomment-52847686 .

Kamil Ogórek

http://kamilogorek.pl kamil.ogorek@gmail.com @kamilogorek

wraithgar commented 10 years ago

FWIW I installed phantomjs locally instead of globally (this was after an nvm upgrade so my global packages were definitely empty) and tests passed.

lukekarrys commented 10 years ago

I made a PR to browser-launcher for this issue.

lukekarrys commented 10 years ago

https://github.com/substack/browser-launcher/pull/32 was merged and I just confirmed that the following steps no longer reproduce the error:

  1. remove global phantomjs
  2. clone an ampersand module
  3. npm install
  4. npm test exits with no matches for phantom/*
  5. install phantomjs
  6. npm test runs the tests