reagent-project / reagent-template

A Leiningen template for projects using Reagent.
MIT License
394 stars 55 forks source link

fix issues when test cljs #98

Closed zjhmale closed 8 years ago

zjhmale commented 8 years ago

if we don't use the deprecated project clojurescript.test, it's will emit a error when lein cljsbuild test

Invalid :test-command, contains non-string value: [phantomjs :runner test/vendor/es5-shim.js test/vendor/es5-sham.js test/vendor/console-polyfill.js target/test.js]
yogthos commented 8 years ago

There should be a way to get this running without falling back to the deprecated project though.

zjhmale commented 8 years ago

ok let me find a way to running without falling back to the deprecated project

yogthos commented 8 years ago

great, thanks for spearheading this

waterlink commented 8 years ago

I have figured it out. I will create a gist and post a link here later today. It actually would involve vendoring a couple of JS shims and phantom runner script.

gadfly361 commented 8 years ago

FWIW, I've been using doo without complaints. I was previously using shims, but doo takes care of it for you can can run on multiple js environments.

For reference: https://github.com/reagent-project/reagent-cookbook/tree/master/recipes/test-example

waterlink commented 8 years ago

@gadfly361 Oh, that is very good example. In my honest opinion, this setup should be generated by +test here.

yogthos commented 8 years ago

that seems reasonable to me

zjhmale commented 8 years ago

using speclj maybe a good solution, i figured it out yestoday and used in my project, i will push codes to this pull request today lately.

zjhmale commented 8 years ago

@yogthos @waterlink @gadfly361 now use speclj to do the unit test, and i lein install in my local env and everything works fine.

2016-01-15 1 28 32
gadfly361 commented 8 years ago

@zjhmale This is sweet! After removing the clojurescript.test plugin, this ran for me (lein cljsbuild test unit).

@yogthos If you decide to invclude, I wonder if this should go in +test or if it should be a separate option like +spec (and keep +test for vanilla cljs.test)

yogthos commented 8 years ago

I think if this works more reliably then might make sense to default to specj then.

zjhmale commented 8 years ago

@gadfly361 remove the deprecated plugin away, and +spec option is a good idea, i will working on that today. @yogthos use speclj is reliable enough, add a +spec option and keep using cljs.test with +test option is more reasonable i think.

yogthos commented 8 years ago

sounds good to me, let's go with the +spec option

waterlink commented 8 years ago

:+1: for +spec version.

Though +test version will stay broken in this case. But that is OK, I will create a PR to fix it then later.

zjhmale commented 8 years ago

@yogthos @waterlink @gadfly361 +spec option is freeze.

waterlink commented 8 years ago

:+1:

yogthos commented 8 years ago

looks like everything's in order, I'll merge and push the new template out? :)

zjhmale commented 8 years ago

@yogthos yes it's ready for merge, i tested the new template and everything works fine :)

yogthos commented 8 years ago

nice work getting this working everybody :+1:

zjhmale commented 8 years ago

it's my pleasure :)

yogthos commented 8 years ago

and all the latest should be up on Clojars