Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enabling karma-coverage support #125

Merged
merged 2 commits into from
Mar 24, 2018
Merged

Enabling karma-coverage support #125

merged 2 commits into from
Mar 24, 2018

Conversation

alun
Copy link
Contributor

@alun alun commented Jan 23, 2017

This PR contains modifications which are adding karma-coverage support for karma runner.

In the end it's possible to measure the coverage, add minimum expectations and generate coverage report on the compiled .cljs files

Also karma-config is changed to have keywords as keys while working with it in clojure.

@bensu
Copy link
Owner

bensu commented Feb 9, 2017

Thank you for the work! I'm a little swamped at work but I'll review it soon.

[(:output-to compiler-opts)
{:pattern (->out-dir "/**") :included false}
{:pattern (->out-dir "/**/*.js") :included false}
{:pattern (->out-dir "/*.js") :included false}])
Copy link
Collaborator

Choose a reason for hiding this comment

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

This undoes the fix done in #153.

@miikka
Copy link
Collaborator

miikka commented Mar 13, 2018

In master, there's now support for custom Karma configuration, so in principle you can configure coverage by hand. I think that support for packages would still be useful, but because master has changed so much, this patch needs some rework to be mergeable.

@alun
Copy link
Contributor Author

alun commented Mar 13, 2018

@miikka thanks for looking into this, I've rewritten the feature on top of the latest master

Copy link
Collaborator

@miikka miikka left a comment

Choose a reason for hiding this comment

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

I've suggested a bunch of small changes and fixes. If you can do those, then I'd be happy to merge this.

README.md Outdated

Add coverage seetings to your `project.clj`

:doo {:coverage {:packages [:my-app.module]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small change suggested: to match the way e.g. cljsbuild :main is configured, it'd be nice to have packages be symbols instead of keywords. Your code already supports it, but it could be shown here: :packages [my-app.module].

"Adds preprocessors for sources from a given packages"
[->out-dir package]
(let [package-path (-> (name package)
(clojure.string/replace #"\." "/"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another replacement needed here is from - to _. E.g. if your package is my-app, the generated source will be in out-dir/my_app/ and not in out-dir/my-app/.

(do
(utils/debug-log "Karma config:" (pr-str karma-opts))
(utils/debug-log "Created karma conf file:" (.getAbsolutePath f)))
(.deleteOnExit f))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Heh, thanks, not deleting the config makes debugging much easier.

@@ -93,7 +94,9 @@
"client" {"args" ["doo.runner.run_BANG_"]}
"singleRun" true
"plugins" (into ["karma-cljs-test"] launcher-plugins)}
(get-in opts [:karma :config]))))
(get-in opts [:karma :config])
(coverage/settings ->out-dir opts)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you swap the order of these two lines? It'd be best to have user's [:karma :config] last so that they can override anything they want, if needed.

@@ -1,4 +1,4 @@
(defproject lein-doo-example "0.1.10-SNAPSHOT"
(defproject lein-doo-example "0.2.1-SNAPSHOT"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer it if feature pull requests didn't change the version numbers – I'd like to do it separately when I do a new release and the next version is not going to be 0.2.1 anyway (it's either 0.1.10 or 0.2.0). Could you remove these changes from your commit?

@alun alun force-pushed the master branch 2 times, most recently from 24ae942 to 461d026 Compare March 19, 2018 15:12
@alun
Copy link
Contributor Author

alun commented Mar 19, 2018

@miikka thanks, all of your remarks are fixed

@miikka miikka merged commit 0039857 into bensu:master Mar 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants