Clojurians Log v2

Hi @thheller, I've spent a good amount of days trying and searching for alternatives, in vain. How feasible would it be to have the :browser-test target generating 2 js files? - one with only the namespaces from the sources - one with the tests and all the rest Having this, it would be extremely easy to run test coverage.

sorry I don't understand what that would accomplish

No worries, let me explain: The code that needs to be analysed, in terms of coverage, has to be in a separate file from the rest of the code (tests, libraries etc.) so we can specify it in Karma's configuration (`preprocessors: 'path/to/code/to/analyse/*.js': ['coverage']`) (see https://karma-runner.github.io/0.8/config/coverage.html). This way Karma's coverage "knows" what js files are to analyse v.s. what js files are testing that code. Currently, the :karma target dumps all the js code into a single file which makes it impossible to run the coverage module with any accuracy.

I don't understand the last argument

how does it get more accurate with multiple files? I presume it uses source map to map the code which will point back to the original sources?

why does it matter that there multiple and not one file?

please point me to technical docs explaining why if there are any?

I simply do not know enough about karma to make any technical decisions but what you are asking does not make sense to me in a build sense. I told you before to use :npm-module if you want one file per namespace. that is the best I can offer?

you can create a reproducible example repo with something that doesn't work so I can take a look

but so far I don't have enough information to make more suggestsions

https://karma-runner.github.io/0.8/config/coverage.html

In the section "Preprocessor"

There is a BAD example

> In this example also JASMINE and JASMINE_ADAPTER get included but they shouldn't as these file are only for the test setup used and not for your program.

> If you include these files there can occur side effects like the following, > - a part of the code coverage report will be output in the installation directory of Karma. > - the code coverage rate is reduced unfairly.

I understand you must have bigger fish to fry and I'd love to be able to propose a PR.

I cloned your repo locally but I hadn't enough time to put myself in the code in order to understand where to tweak it.

Please, if you could point me into the right direction I would appreciate it!

the only thing I can say is that I need a reproducible repo to actually see what is happening before making any decicions

the JASMINE line makes so sense to me since that is not something shadow-cljs uses or includes

I took it as an example, it's valid with any non-functional/production code

I can't point you in any direction since I still don't have the slightest clue which problem you are actually trying to solve

OK. Let me create a repo to illustrate the issue

I'll get back to you in a few days

It was faster to demonstrate than I thought: https://github.com/ClemRz/shadow-cljs-karma-coverage-issue Let me know if I can help on anything 🙂

ok from what I can gather you are expecting to get 100% coverage from running one simple test?

but since the code includes all of cljs.core and so on that skews your numbers and you only want coverage for you actual ns

can't think of a decent way to make that happen

but I can point you to https://github.com/thheller/shadow-cljs/blob/master/src/main/shadow/build/targets/karma.clj#L106-L112

thats the code controlling the output. so you can try and modify it to match your needs.

Thank you Thomas! I'll look into it.

Ok, I found a way to make it work! I know it is a bit ugly... 😁 I created a remediation branch on the repo to show how I made it work (https://github.com/ClemRz/shadow-cljs-karma-coverage-issue/tree/remediation). @thheller I'd be very happy to see this change included in the next release of shadow-cljs, if possible. Please let me know how can I help with that.

you can rename the karma.clj to karma2.clj or move it to an entirely different namespace

you can then publish it as a separate library

as a quick test I made

----------------------|---------|----------|---------|---------|--------------------
File                  | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
----------------------|---------|----------|---------|---------|--------------------
All files             |   71.43 |     62.5 |      50 |      72 |                   
 coverage.app.js      |     100 |      100 |     100 |     100 |                   
 coverage.app_test.js |   65.22 |       50 |   33.33 |      65 | 3,9,14-15,22,26-27
----------------------|---------|----------|---------|---------|--------------------

that seems more along the lines of what you want? actually split by ns? but for some reason it just reports coverage and no longer failing tests

anyways I don't really use karma myself so someone that does should really work on this more

Good morning Thomas. TY for looking into it. For your test: have you pulled the remediation branch or did you just put the override of karma.clj? (I'm asking because it looks more like a configuration thing)

https://clojurians.slack.com/archives/C6N245JGG/p1605690861061100?thread_ts=1604691976.359900&cid=C6N245JGG do you have an example of a separate library that I could use please?

https://gist.github.com/thheller/bb6e190c6559a849cf239ea6dcfe8288

basically the idea was to generate a files.json file so karma knows how to load the separate files in order

https://gist.github.com/thheller/bb6e190c6559a849cf239ea6dcfe8288#file-kama-conf-js

so the files are truly seperate

seems to work but probably requires tuning a lot more

TY, I like this approach. I'll dig into it and let you know if I can tune it.

Would it be a good idea to reference karma.clj's function that hasn't been changed instead of redefining them? I'm thinking that it would be good to have both targets evolving along but at the same time it would make it harder to maintain.

no clue. depends on how many other changes make sense I guess.

I built the karma stuff because someone asked how to do it ... without ever having used it myself there are probably a lot of things that could be done better

How did you configure the target in your shadow.edn please?

no change from yours

except :target :karma2 of course

If you change this in your karma.conf.js you'll get a better result:

preprocessors: {
            'target/files/coverage.!(*_test).js': ['coverage']
        },

Basically it prevents coverage from analyzing the test code.

I noticed that the / are escaped in files.json:

"target\/files\/shadow.test.karma.js"

found it: (json/write-str filenames :escape-slash false)

I variabilized the "target" since it comes from the configuration: https://github.com/ClemRz/shadow-cljs-karma-coverage-issue/blob/remediation/src/main/shadow/build/targets/karma_coverage.clj

I am getting back to your comment: https://clojurians.slack.com/archives/C6N245JGG/p1605630730053000?thread_ts=1604691976.359900&cid=C6N245JGG

Currently there is no mapping produced by the :karma2 target. How difficult would it be to include it?

you can probably call this function https://github.com/thheller/shadow-cljs/blob/fced62bfafcdedaa30817bd81181c8a2be1f2d26/src/main/shadow/build/output.clj#L252

that flushes the sources with source map

nice! thanks!

Am I missing something? The "file" entry is omitted from the source map:

{
  "version": 3,
  "sources": [
    "coverage/app.cljs"
  ],
  "mappings": ";AAEA,AAAA,AAAMA,AAAeC;AAArB,AACM,AAAI,AAAA,AAAMA;AAAV;;AAAA",
  "names": [
    "<http://coverage.app/some-function|coverage.app/some-function>",
    "arg"
  ],
  "sourcesContent": [
    "(ns <http://coverage.app|coverage.app>)\n\n(defn some-function [arg]\n      (if (nil? arg) \"foo\" \"bar\"))"
  ]
}

what would that do? the content is right there?

You are right, this field seems to be optional (https://sourcemaps.info/spec.html). I think there is something I missed with the config of remap-istanbul plugin. FYI this plugin allows a mapping between the js files and the cljs sources.

Yeah, that's it, I have an outdated version, this has been fixed: https://github.com/SitePen/remap-istanbul/pull/170

After a bit of fighting with this abandon-wares I finally got it to work:

-----------|----------|----------|----------|----------|----------------|
File       |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
-----------|----------|----------|----------|----------|----------------|
 coverage/ |      100 |      100 |      100 |      100 |                |
  app.cljs |      100 |      100 |      100 |      100 |                |
-----------|----------|----------|----------|----------|----------------|
All files  |      100 |      100 |      100 |      100 |                |
-----------|----------|----------|----------|----------|----------------|

Thanks a lot for your help @thheller!!