feat: add test coverage #1606
feat: add test coverage #1606
Conversation
Signed-off-by: Scott Dover <[email protected]>
Signed-off-by: Scott Dover <[email protected]>
Signed-off-by: Scott Dover <[email protected]>
| "client/src/connection/rest/api", | ||
| "server/dist", | ||
| "tools", | ||
| "server/src/python" |
There was a problem hiding this comment.
server/src/python is copied from node_modules as part of the build process
There was a problem hiding this comment.
sorry this directory looks not copied from node_modules to me. where did you see the copy?
| "**/node_modules/**", | ||
| "**/*.test.*", | ||
| "client/dist", | ||
| "client/src/connection/rest/api", |
There was a problem hiding this comment.
These are files auto-generated by OpenAPI
| "checkCoverage": true, | ||
| "lines": 52, | ||
| "statements": 52, | ||
| "branches": 71, | ||
| "functions": 43 |
There was a problem hiding this comment.
What do you think about adding minimum levels now just to make sure things don't get worse (the numbers above are right below the current values), with the eventual goal being having code coverage at 80%.
And, do we care about these individual numbers, or do we really only care about lines (I'm inclined to believe this should probably only be "lines": 80)?
Last, is 80% code coverage reasonable in an open source repository, or would that be asking too much from contributors?
There was a problem hiding this comment.
I would vote to not fail the build until we're fully ready. I guess we're not setting up testing infrastructure to the webviews. So if lines added in webviews and make the numbers worse it's not straightforward to fix.
There was a problem hiding this comment.
A main concern holding the original #93 is that the numbers are not reflecting the real coverage.
For example, I looked at the coverage report from the build
https://github.com/sassoftware/vscode-sas-extension/actions/runs/17080242102/job/48431996872?pr=1606
Looking at client/src/node/extension.ts, most of lines are uncovered, 55-283,285-290.
It doesn't make sense because this file is the entry point. As long as the extension activates, most of the lines should be executed.
Similar to the server/src/server.ts file.
The coverage only counts files directly imported from tests. But some tests require the vscode environment.
| "client/src/connection/rest/api", | ||
| "server/dist", | ||
| "tools", | ||
| "server/src/python" |
There was a problem hiding this comment.
sorry this directory looks not copied from node_modules to me. where did you see the copy?
| "src": ["client/src", "server/src"], | ||
| "exclude": [ | ||
| "**/node_modules/**", | ||
| "**/*.test.*", |
There was a problem hiding this comment.
I see client/test in the coverage report. I guess we should exclude it.
| "checkCoverage": true, | ||
| "lines": 52, | ||
| "statements": 52, | ||
| "branches": 71, | ||
| "functions": 43 |
There was a problem hiding this comment.
I would vote to not fail the build until we're fully ready. I guess we're not setting up testing infrastructure to the webviews. So if lines added in webviews and make the numbers worse it's not straightforward to fix.
Summary
This revives @smorrisj's previous work around using c8 for test coverage. This is still a wip
Testing