feat: add code coverage #93

smorrisj · 2023-01-18T15:59:21Z

Summary
Draft proposal to integrate c8 coverage tool for tests

Testing
-close all running vs code instances
-from terminal run "npm run coverage"
-verify that the expected extension tests run and that counters are output at the end

smorrisj · 2023-01-18T16:02:32Z

Draft proposal of what integrating c8 would look like. I think for now it's probably sufficient to report on the metrics so that we can establish a baseline to build from. Perhaps later on we can begin gating the PRs based on falling below thresholds after we've hit a targeted minimum threshhold. Ive set the value to 75% but I also believe that number should be discussed as well.

scottdover · 2023-01-18T16:26:02Z

package.json

@@ -332,13 +332,15 @@
    "format": "prettier --write .",
    "pretest": "npm run compile && tsc -p ./client",
    "test": "node ./client/out/test/runTest.js",
+    "coverage": "c8 --clean --check-coverage --lines 75 npm run test",


We're currently using jest for testing, right? Is there anything c8 gives us that jest doesn't? It looks like they're both using istanbul under the hood

Looks like jest also supports thresholds in a configuration: https://jestjs.io/docs/configuration#coveragethreshold-object

I dont have a strong opinion on which one we choose. If we already have jest as a dependency (looks like we do) then we can reuse what we have.

It looks like the extension test api uses mocha. Jest-Worker is being pulled in transitively, but the dev dependencies also support the mocha theory:

"devDependencies": { "@types/mocha": "^10.0.1", "@types/node": "^18.11.18", "@typescript-eslint/eslint-plugin": "^5.48.1", "@typescript-eslint/parser": "^5.48.2", "c8": "^7.12.0", "chai": "^4.3.6", "esbuild": "^0.13.15", "eslint": "^8.31.0", "media-typer": "^1.1.0", "mocha": "^10.2.0", "path-browserify": "^1.0.1", "prettier": "^2.7.1", "ts-loader": "^9.4.2", "typescript": "^4.4.3", "webpack": "^5.75.0", "webpack-cli": "^5.0.1" }

Based on the original stackoverflow post from the issue, it looks like when using mocha that an additional library is needed.

Another interesting data point to look at use trends for the 3 popular ones, nyc, c8, and istanbul:

Specifically with respect to downloads and the frequency for which the libraries are being updated.

@2TomLi asked me to quick take a look at this since I've done a good amount of work on jest in other projects. I'm not familiar with this project though so if I miss something, sorry.

As @smorrisj was saying, this project, as best as I can see, is using mocha and I don't see any support for coverage directly from mocha like we get with jest. Using either c8 or nyc seems like an easy setup. istanbul doc says nyc is basically the istanbul CLI so you're left with just c8 or nyc. Both c8 and nyc seem popular enough to use and find support if needed. Both integrate the same way. So if y'all decide one isn't good, it should be an easy swap to the other.

scnwwu · 2023-01-19T07:53:21Z

It's great. I looked at the build log
https://github.com/sassoftware/vscode-sas-extension/actions/runs/3951890140/jobs/6766307438#step:6:198
BTW., I can't find an overall coverage from current log. I guess it would be better to set the reporter to HTML for the task so that developers can easily view the details on local.
I guess we need further config for the files. Currently it lists dist, test and tools files which don't need to be counted.

smorrisj · 2023-01-19T14:06:08Z

It's great. I looked at the build log https://github.com/sassoftware/vscode-sas-extension/actions/runs/3951890140/jobs/6766307438#step:6:198 BTW., I can't find an overall coverage from current log. I guess it would be better to set the reporter to HTML for the task so that developers can easily view the details on local. I guess we need further config for the files. Currently it lists dist, test and tools files which don't need to be counted.

Great points. I started out taking the defaults to see where we'd want to tailor the config. I'll add a .c8rc to frame further discussion around configs.

smorrisj · 2023-01-19T14:37:06Z

.c8rc

@@ -0,0 +1,6 @@
+{
+  "all": true,
+  "include": ["client/out/**"],


@scnwwu Which directories from lsp server should be included here? Is src sufficient? I suspect data and pubsdata are not needed.

Yes, I think src should be good.
BTW., for client, why not "client/src" but "out"?

The coverage counters act upon the transpiled output from what I've experimented with. When I changed it to only include client/src/** and server/src/**, c8 didnt output any coverage data in the report. Based on this is it still appropriate to exclude dist from server directory?

Thank you for the information. I don't have much knowledge about it. I'm confused about why it can count the files in "client/out". The tests run on VS Code which will run the "client/dist/node/extension.js" file. The files in "client/out" is not used except test files. Perhaps only including "dist" directory should be enough?
Also, I guess the files in "connection/rest/api" can be excluded as they're generated.

OK, I see. Looking at the coverage report, the "node/extension.ts" file shows 0. So it only counts the files that directly imported from the tests. So I guess both "client/out" and "client/dist" should be included, hopefully it can aggregate the coverage. And for server "server/dist" should be good.

@scnwwu thanks, will update the includes to add these.

scnwwu · 2023-01-20T01:12:48Z

A reminder, this change will add some new files to the workspace. Before merge, please update the .vscodeignore file if needed to make sure only needed files will be packaged to VSIX. Thanks.

smorrisj · 2023-01-20T13:06:07Z

A reminder, this change will add some new files to the workspace. Before merge, please update the .vscodeignore file if needed to make sure only needed files will be packaged to VSIX. Thanks.

Will update the .vscodeignore to reflect recently added files that should not be part of the vsix.

scnwwu · 2023-01-28T02:48:41Z

.c8rc

@@ -0,0 +1,6 @@
+{
+  "all": true,
+  "include": ["client/out/**"],


Thank you for the information. I don't have much knowledge about it. I'm confused about why it can count the files in "client/out". The tests run on VS Code which will run the "client/dist/node/extension.js" file. The files in "client/out" is not used except test files. Perhaps only including "dist" directory should be enough?
Also, I guess the files in "connection/rest/api" can be excluded as they're generated.

scnwwu · 2023-01-28T02:49:20Z

.vscodeignore

@@ -15,3 +15,6 @@ prettier.config.js
 contributing.md
 ContributorAgreement.txt
 .travis.yml
+.eslintignore
+.eslintrc.js
+.c8rc


does coverage/ need to be added here too?

@scnwwu agreed, will add that to the list here.

scnwwu · 2023-01-28T02:50:42Z

package.json

@@ -332,13 +332,15 @@
    "format": "prettier --write .",
    "pretest": "npm run compile && tsc -p ./client",
    "test": "node ./client/out/test/runTest.js",
+    "coverage": "c8 --clean --lines 75 npm run test",


as there's a config file now, do we want to move --lines config into the config file?

@scnwwu thanks. Agreed. will add that to the .c8rc

scnwwu · 2023-01-29T00:57:33Z

package.json

@@ -332,13 +332,15 @@
    "format": "prettier --write .",
    "pretest": "npm run compile && tsc -p ./client",
    "test": "node ./client/out/test/runTest.js",
+    "coverage": "c8 --clean --lines 75 npm run test",


Oh, another question, looking at current log
https://github.com/sassoftware/vscode-sas-extension/actions/runs/3997580900/jobs/6859100928
It shows 42.67% lines for "All files". Why the build didn't fail as it requires 75%?

@scnwwu I am not using the check-coverage flag. Without the check-coverage flag it will just list the coverage numbers in information mode and not fail the build. I thought for the first iteration this would be best, so that we can make targeted efforts to bring the numbers up. After the numbers are up we would enable the check-coverage option so that new changes that cause the threshold to fall below 75 percent would fail?

smorrisj · 2023-04-10T14:02:36Z

@scnwwu the "all" flag will use the "src" setting to load said files into the report. See updated c8rc config file. C8 doc. I also have the html report showing. Wondering what else we might need to implement so that we can get this merged?

scnwwu · 2023-04-11T04:59:40Z

.c8rc

@@ -0,0 +1,7 @@
+{
+  "all": true,
+  "src": ["client/src", "server/src"],


I'm a little confused. Does this src setting really take effect? If so, sounds like "client/out/test/" doesn't need to be excluded as it won't be included? I looked at latest build log, tools/build.mjs is also in the report.

scnwwu · 2023-04-11T05:08:23Z

@scnwwu the "all" flag will use the "src" setting to load said files into the report. See updated c8rc config file. C8 doc. I also have the html report showing. Wondering what else we might need to implement so that we can get this merged?

I looked at the latest build log. It looks still confusing. It shows both dist and src. Some count on src some not. For example client/src/node/extension.ts is the entry file but shows 0. So the total percentage looks not reliable. We probably would want to look at how to map the dist coverage to src. But I'm good with merge it and investigate it separately. Thank you.

scottdover · 2024-05-01T13:54:20Z

Hey @smorrisj / @scnwwu . This PR has been sitting for more than a year now, and I'm wondering if it should remain open?

It seems like this would be a good thing to pursue longer term. If no one objects, I can take a todo to see what's needed to wrap this up during reset week?

smorrisj · 2024-05-01T14:02:59Z

I think it's still important and should be implemented. Other priorities have pulled me away from looking into it further. See @scnwwu's latest comment. I think if we could get answers to those, maybe it would be enough to make small adjustments to what's there to finish it up.

scnwwu · 2024-05-06T09:27:34Z

VS Code's test-cli already integrates c8, maybe worth a look when anyone pick this up
https://github.com/microsoft/vscode-test-cli/blob/main/src/config.cts#L173

smorrisj requested review from scnwwu, 2TomLi, clangsmith and scottdover January 18, 2023 15:59

smorrisj linked an issue Jan 18, 2023 that may be closed by this pull request

Add code coverage task to scripts to measure test coverage #92

Open

smorrisj changed the title ~~feature: add c8 code coverage (92)~~ feature: add c8 code coverage Jan 18, 2023

scottdover reviewed Jan 18, 2023

View reviewed changes

smorrisj changed the title ~~feature: add c8 code coverage~~ feature: add code coverage Jan 18, 2023

smorrisj self-assigned this Jan 18, 2023

smorrisj commented Jan 19, 2023

View reviewed changes

scnwwu reviewed Jan 28, 2023

View reviewed changes

scnwwu reviewed Jan 29, 2023

View reviewed changes

smorrisj changed the title ~~feature: add code coverage~~ feat: add code coverage Jan 31, 2023

2TomLi added the code quality Code quality related issues label Feb 1, 2023

smorrisj added 7 commits April 10, 2023 08:45

feature: add c8 code coverage (92)

2a8f0c4

fix: use global npmjs

4b58b10

chore: add npmrc to define global registry

876f710

fix: move .npmrc to root

11e47fb

fix: add coverage info to pr check

df7454a

chore: add .c8rc to configure c8

58bdb1c

chore: add recently created dev configs to .vscodeignore

5d1c3fa

smorrisj force-pushed the feature/code-coverage branch from 0ae3e0e to 5d1c3fa Compare April 10, 2023 12:47

smorrisj added 2 commits April 10, 2023 08:49

fix: botched merge

e6fa075

chore: load source files into metrics

b0fe8ce

smorrisj added 4 commits April 10, 2023 10:04

fix: add new files to vscode ignore

6533212

fix: add coverage dir to vscode ignore

7fdaa28

fix: prettier ignore coverage

e3d6913

fix: move lines to config

bdfe42a

smorrisj marked this pull request as ready for review April 10, 2023 15:00

scnwwu reviewed Apr 11, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add code coverage #93

feat: add code coverage #93

smorrisj commented Jan 18, 2023

smorrisj commented Jan 18, 2023 •

edited

Loading

scottdover Jan 18, 2023 •

edited

Loading

smorrisj Jan 18, 2023

smorrisj Jan 18, 2023 •

edited

Loading

smorrisj Jan 18, 2023 •

edited

Loading

nick-roth-sas Jan 18, 2023

scnwwu commented Jan 19, 2023

smorrisj commented Jan 19, 2023

smorrisj Jan 19, 2023 •

edited

Loading

scnwwu Jan 20, 2023

smorrisj Jan 20, 2023

scnwwu Jan 28, 2023

scnwwu Jan 29, 2023

smorrisj Feb 6, 2023

scnwwu commented Jan 20, 2023

smorrisj commented Jan 20, 2023

scnwwu Jan 28, 2023

scnwwu Jan 28, 2023

smorrisj Feb 6, 2023

scnwwu Jan 28, 2023

smorrisj Feb 6, 2023

scnwwu Jan 29, 2023

smorrisj Feb 6, 2023

smorrisj commented Apr 10, 2023

scnwwu Apr 11, 2023

scnwwu commented Apr 11, 2023

scottdover commented May 1, 2024

smorrisj commented May 1, 2024

scnwwu commented May 6, 2024

feat: add code coverage #93

Are you sure you want to change the base?

feat: add code coverage #93

Conversation

smorrisj commented Jan 18, 2023

smorrisj commented Jan 18, 2023 • edited Loading

scottdover Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smorrisj Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

smorrisj Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scnwwu commented Jan 19, 2023

smorrisj commented Jan 19, 2023

smorrisj Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scnwwu commented Jan 20, 2023

smorrisj commented Jan 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smorrisj commented Apr 10, 2023

Choose a reason for hiding this comment

scnwwu commented Apr 11, 2023

scottdover commented May 1, 2024

smorrisj commented May 1, 2024

scnwwu commented May 6, 2024

smorrisj commented Jan 18, 2023 •

edited

Loading

scottdover Jan 18, 2023 •

edited

Loading

smorrisj Jan 18, 2023 •

edited

Loading

smorrisj Jan 18, 2023 •

edited

Loading

smorrisj Jan 19, 2023 •

edited

Loading