bensu / doo

doo is a library and lein plugin to run cljs.test on different js environments.
Eclipse Public License 1.0
324 stars 63 forks source link

Support for custom Karma launchers #108

Closed cdorrat closed 6 years ago

cdorrat commented 8 years ago

I'm using lein-doo to test a clojurescript chrome plugin and I needed to be able to launch chrome with the "--load-extension=target/chrome" command line argument.

This pull request allows you to add new karma launch configurations in the project.clj and run them from lein doo. There's an example in readme.md and the sample project

bensu commented 8 years ago

Hi @cdorrat,

Thanks for your work! It is a big PR with many changes, so I'll take some time to review it (I first need to wrap up and release 0.1.7).

kamituel commented 8 years ago

@cdorrat Have you published your doo fork anywhere for the time being (before this gets merged)? I'd like to start running our unit tests in CI and this would be very helpful. If not, I'll publish it temporarily.

kamituel commented 8 years ago

Also, on this PR's branch, the following config causes an exception to be thrown when running doo:

  :doo {:paths {:karma "node_modules/.bin/karma"}
        :karma {:config {"reporters" ["progress", "junit"]
                         "plugins" ["karma-junit-reporter"]
                         "junitReporter" {"outputDir" "test-output"}}}}

It can be solved by adding :alias {} to the map above. I guess it could be handled gracefully though.

cdorrat commented 8 years ago

Hi @kamituel, I had noticed the alias issue, I think it was in the 1.7-SNAPSHOT branch before my changes, we should probably raise an issue or fix it in master before the 1.7 release.

I'm happy for you to temporarily push a build of my fork to clojars until the pr is accepted, I'd meant to do it myself but hadn't had time yet.

kamituel commented 8 years ago

I published a Doo version with this PR as [kamituel/lein-doo "0.1.8-PR-108-1-SNAPSHOT"]. Hope to get rid of it soon, though, once this PR gets merged.

bensu commented 8 years ago

@cdorrat can you give me an example of a config that caused problems in master?

@kamituel thank you for pushing to Clojars!

cdorrat commented 8 years ago

Hi @bensu, commenting out the :doo :alias section in the example project will give the following error when running lein doo phantomjs on a checkout of the current master (0.1.7-SNAPSHOT)

Exception in thread "main" java.lang.AssertionError: Assert failed: (map? alias-table)
        at doo.core$resolve_alias.invoke(core.clj:23)
        at leiningen.doo$cli__GT_js_envs.invoke(doo.clj:130)
        at leiningen.doo$doo.doInvoke(doo.clj:201)
        at clojure.lang.RestFn.invoke(RestFn.java:423)
        at clojure.lang.Var.invoke(Var.java:383)
        at clojure.lang.AFn.applyToHelper(AFn.java:156)
        at clojure.lang.Var.applyTo(Var.java:700)
        at clojure.core$apply.invoke(core.clj:632)
        at leiningen.core.main$partial_task$fn__5953.doInvoke(main.clj:261)
        at clojure.lang.RestFn.applyTo(RestFn.java:139)
        at clojure.lang.AFunction$1.doInvoke(AFunction.java:29)
        at clojure.lang.RestFn.applyTo(RestFn.java:137)
        at clojure.core$apply.invoke(core.clj:632)
        at leiningen.core.main$apply_task.invoke(main.clj:311)
        at leiningen.core.main$resolve_and_apply.invoke(main.clj:317)
        at leiningen.core.main$_main$fn__6019.invoke(main.clj:390)
        at leiningen.core.main$_main.doInvoke(main.clj:383)
        at clojure.lang.RestFn.invoke(RestFn.java:421)
        at clojure.lang.Var.invoke(Var.java:383)
        at clojure.lang.AFn.applyToHelper(AFn.java:156)
        at clojure.lang.Var.applyTo(Var.java:700)
        at clojure.core$apply.invoke(core.clj:630)
        at clojure.main$main_opt.invoke(main.clj:316)
        at clojure.main$main.doInvoke(main.clj:421)
        at clojure.lang.RestFn.invoke(RestFn.java:457)
        at clojure.lang.Var.invoke(Var.java:394)
        at clojure.lang.AFn.applyToHelper(AFn.java:165)
        at clojure.lang.Var.applyTo(Var.java:700)
        at clojure.main.main(main.java:37)
bensu commented 8 years ago

@cdorrat thanks for the quick response.

@arichiardi also found it and pointed me to the solution in #113 and it is now fixed.

smogg commented 7 years ago

We encountered the same problem, is this PR still a work-in-progress?

mulchy commented 7 years ago

Is this still being worked on?

miikka commented 6 years ago

🎉 Merged! 🎉

Thanks for the patch @cdorrat. I have a further improvement proposal in #171 and I hope to create a release soon that will include this change.