Improve code coverage reporting #7961
Conversation
|
@nwmac Did you make a test to the Codecov report to be sure it's actually working, before we merge this? |
|
@nwmac I also cannot see anymore the Codecov delta comment here in the PR, has this been removed from the settings? |
|
@cnotv Okay - once PSA stuff is done - you and I can look at the PR stuff - maybe I missed a change for that. |
| cp -r coverage/* coverage-artifacts/coverage/ | ||
| ls -al coverage-artifacts | ||
|
|
||
| - name: Upload updated coverage |
There was a problem hiding this comment.
Why do we have 4 uploads now?
cypress.config.ts
Outdated
| @@ -72,7 +72,7 @@ export default defineConfig({ | |||
| }, | |||
| env: { | |||
| baseUrl, | |||
| coverage: false, | |||
| coverage: true, | |||
There was a problem hiding this comment.
Once again this is turned to true. Doing so it wastes time while developing E2E tests.
Please try to understand this point.
package.json
Outdated
| @@ -30,6 +30,7 @@ | |||
| "docker:local:start": "docker run -d --restart=unless-stopped -p 80:80 -p 443:443 -e CATTLE_BOOTSTRAP_PASSWORD=password -e CATTLE_PASSWORD_MIN_LENGTH=3 --name cypress --privileged rancher/rancher:v2.7-head", | |||
| "docker:local:stop": "docker kill cypress || docker rm cypress || true", | |||
| "build": "./node_modules/.bin/nuxt build --devtools", | |||
| "build:ci": "node --max-old-space-size=8192 ./node_modules/.bin/nuxt build", | |||
There was a problem hiding this comment.
Why is --max-old-space-size=8192 required?
There was a problem hiding this comment.
We do not use nuxt anymore as well, so it should be something like
"build: ci": "NODE_OPTIONS=--max_old_space_size=8192 ./node_modules/.bin/vue-cli-service build",
Although we had the same issue in build, so maybe we just stick to the current value
| @@ -72,7 +72,7 @@ export default defineConfig({ | |||
| }, | |||
| env: { | |||
| baseUrl, | |||
| coverage: false, | |||
| coverage: process.env.TEST_COVERAGE === 'true', | |||
There was a problem hiding this comment.
Do not forget to mention it in the documentation, sorry about mentioning it 😬
| @@ -2931,6 +2931,16 @@ | |||
| resolved "https://registry.yarnpkg.com/@types/stack-utils/-/stack-utils-2.0.1.tgz#20f18294f797f2209b5f65c8e3b5c8e8261d127c" | |||
| integrity sha512-Hl219/BT5fLAaz6NDkSuhzasy49dwQS/DSdu4MdggFB8zcXv7vflBI3xp7FEmkmdDkBUI2bPUNeMttp2knYdxw== | |||
|
|
|||
| "@types/strip-bom@^3.0.0": | |||
There was a problem hiding this comment.
I think we need to generate a new yarn.lock as some changes seems should not be there? 🤔
| @@ -1 +1,10 @@ | |||
| module.exports = require('./shell/babel.config.js'); | |||
There was a problem hiding this comment.
Careful with this change, since in the shell we have some configuration changed, including the one where we allow the use of require, otherwise it creates error:
TypeError: require.context is not a function
|
I've been looking into the mismatch between the coverage reports generated by jest and cypress - looks like the cypress files ignore import and export statements - did a search and found this - istanbuljs/babel-plugin-istanbul#135 |
Fixes #7142
This improves the code coverage reports.
This PR includes the important changes from - #7166 so that can be closed.
I gave up on trying to get the unit and e2e coverage reports to report over the same set of files and instead went for a post-process approach - we post-process the reports and ensure that both reports contain coverage data for the files included in the individual report.
The artifact uploaded to GitHub has been updated - this now contains unit, e2e and combined results and these now make sense - i.e. the combined result is at least as high as the lowest of the two.
Note that codecov still reports a slightly different coverage value that the reports above - but it is at least now approximately correct - e.g. the reports generated via Istanbul might show 27.8% and codecov will report 27.1% - it appears codecov uses a different (or newer version of) a library for processing these - you can see it identified more lines etc than the istanbul reports - but I think the numbers are now close enough that this is ok.
FYI: @cnotv