Clojurians Log v2

That’s a pretty interesting picture. Let’s add it and people can use it if they want to.

@brandon.ringe Just a note: the above change only does coverage for Typescript files

Oh gotcha. I also want to point out that some (maybe a lot / most) of the Typescript code has no unit tests because the modules import vscode, and as far as I know / remember, we can't write units tests for any module that does that. It has something to do with how vscode is built / provided. It's a run-time thing or something (@pez might be able to explain it more), and isn't available in a unit test run.

However, I learned that we can pass vscode into a function as an argument and use it that way, which is what the clojurescript code for the new output view does. I haven't tried that in TS, but in cljs it works fine. At extension startup now, we pass vscode from the TS to the cljs and https://github.com/BetterThanTomorrow/calva/blob/8ead76b6ff65ebb90c6d3a9e1bd03b512a8655c5/src/cljs-lib/src/calva/util.cljs#L33, and code that needs it uses it from that atom.

Using this method of passing vscode as a function arg, we can test the code that uses it now in unit tests, and we can mock it however we want.

@brandon.ringe Yeah, I was wondering about that. Testing those modules might not be feasible, at least without the inversion/dependency injection that you mention, or maybe bringing in some type of mocking framework like Jest or Sinon. At least for the paredit stuff, I've been pleasantly surprised at how well separated the logic is from the vscode APIs!

Wow that's great. Does it make it possible to test "pressing keys", so a test could verify the cursor 'when' detectors?

Are you referring to the mocking of vscode? I'm not sure. Haven't tried something like that.

I'm far from an expert on VSCode, never mind VSCode extensions (!). But my understanding is that Microsoft learns towards integration tests (presumably that have a headless VSCode running). From a quick look, it looks the integration test APIs are used in the Calva tests already, like in format-test.ts (among other files). I poked around, but didn't see any way to test keybindings (and the when clause along with them) with integration tests. That might be by design, since the tests would differ between platforms (eg Mac and Linux) and tests that run differently on different platforms can be painful. There's an existing set of VSCode mocks that I noticed: https://github.com/streetsidesoftware/jest-mock-vscode/blob/main/README.md But that sort of thing is for simpler tests, like unit tests.

Just my experience from work - coverage helps me note that my unit tests are at least hitting all the lines. If I mess up and completely miss something, the coverage will alert me to it. This varies, but the general consensus is that requiring a coverage target in PRs backfires. People end up creating all sorts of useless tests, or tests that actually don't have any asserts, just to hit the numbers. It's super useful as a tool for those who care though. Note that coverage is pretty anemic in the Calva codebase now, so it may only be useful for those who really want to use it. I'll attach a screenshot of the coverage numbers so you can get an idea.

I don’t see a reason not to add it if you want to. I’ve used a code coverage tool with JS or TS years ago at a job and I liked it. Haven’t used one with Clojure yet.

I threw together a quick change to do code coverage with Istanbul. Is this worth an issue/pull request? Changes are simple:

diff --git a/package.json b/package.json
index 97a4c4180..d3e332f01 100644
--- a/package.json
+++ b/package.json
@@ -3394,10 +3394,21 @@
     "prettier-format-watch": "onchange \"./**/*.{ts,js,json}\" -- prettier --write {{changed}}",
     "preprettier-format-watch": "npm run prettier-format",
     "eslint": "npx eslint . --ext .js,.jsx,.ts,.tsx",
+    "test-coverage": "nyc --reporter=html npm run unit-test",
     "eslint-watch": "npx esw . --ext .js,.jsx,.ts,.tsx --watch",
     "install-ys": "curl -sS  | PREFIX=/tmp/yamlscript VERSION=0.1.56 bash",
     "publish": "bb scripts/publish.clj"
   },
+  "nyc": {
+    "extends": "@istanbuljs/nyc-config-typescript",
+    "all": true,
+    "include": [
+      "src/**/*.ts"
+    ],
+    "exclude": [
+      "src/extension-test/**"
+    ]
+  },
   "dependencies": {
     "@vscode/debugadapter": "^1.64.0",
     "ansi-regex": "^5.0.1",
@@ -3426,6 +3437,7 @@
     "vsls": "^1.0.4753"
   },
   "devDependencies": {
+    "@istanbuljs/nyc-config-typescript": "^1.0.2",
     "@types/chai": "^4.2.6",
     "@types/follow-redirects": "^1.13.0",
     "@types/glob": "^7.1.1",
@@ -3454,6 +3466,7 @@
     "mocha-junit-reporter": "^2.0.2",
     "mocha-multi-reporters": "^1.5.1",
     "node-gyp": "^8.4.1",
+    "nyc": "^17.1.0",
     "onchange": "^7.1.0",
     "ovsx": "^0.8.3",
     "prettier": "2.5.1",

Thanks! I am not sure what code coverage would bring to our table. WDYT, @brandon.ringe?

I don’t think it would hurt, and might only help, so long as developers are actually using it to make decisions about test writing

It might only really help if we were to set a goal / standard for test coverage in Calva and ensure that PRs meet it.

There’s nothing for code coverage.

Cloverage (or lein-cloverage) makes Clojure coverage reports based on a run of clojure.test tests (or of course whatever you configure)

| Cloverage Ah yes, I use it for Clojure. But AFAIK it doesn't do Typescript, right?

For the record. I didn’t understand that the question was about testing Calva’s source code. There’s no coverage implemented there either, but there should be option’s out there for doing it.