-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add coverage reports to e2e tests #443
Conversation
Had to wait a few minutes for Codecov to show the correct percentage in the branch of my fork. Master: This branch: Which I think is correct. There are no tests for |
Codecov Report
@@ Coverage Diff @@
## master #443 +/- ##
===========================================
+ Coverage 45.24% 56.35% +11.10%
===========================================
Files 37 50 +13
Lines 862 976 +114
Branches 67 75 +8
===========================================
+ Hits 390 550 +160
+ Misses 459 414 -45
+ Partials 13 12 -1
Continue to review full report at Codecov.
|
To review, just confirm GitHub workflow looks OK, look at the changes in the |
(Codecov seems confused as we have 2 reports uploaded. It's commenting only on the unit test report. But there is a second one for e2e that should increase the overall coverage.) |
Will fix conflicts and rebase tomorrow morning 👍 |
Note, when I created the PR, Codecov added a comment with the unit tests coverage (~40%). Only a few moments later it removed the old comment, and replaced it by a new one with the correct coverage (above, of ~50%). That makes sense I think. We have 2 jobs in the GitHub workflow. One for Ubuntu latest (default), and another with Ubuntu 16.04 due to a bug in Cypress. Codecov must be sending the first comment once the first job is done. Then there is a delay until the Cypress e2e job is done, and the coverage report is merged & updated in Codecov. |
Rebased and fixed conflicts. Now waiting for GitHub to tell whether that worked OK or not 😬 |
- name: Upload coverage to Codecov | ||
uses: codecov/codecov-action@v1 | ||
with: | ||
token: ${{ secrets.CODECOV_TOKEN }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might not kick in until merged as secrets are not exposed to forks.
Annoyingly GitHub doesn't raise an error if a template variable is unset, it just gives you an empty string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I thought it was working for the other coverage report (I copied this block from the existing line for unit tests coverage)
Will resolve conflicts once #446 is in. |
… new command coverage:e2e
…id calling command to instrument it)
Looks ok, how do we know if it's working? |
Ah, good question. Should have included how to review. Best way, IMO, would be looking at Codecov, and confirming it got a report from GitHub action. https://codecov.io/gh/kinow/cylc-ui/branch/coverage-e2e (rebased some hours ago) Or compare both: https://codecov.io/gh/kinow/cylc-ui/compare/master...coverage-e2e The reports should show files that are not covered by unit tests, e.g. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or compare both: https://codecov.io/gh/kinow/cylc-ui/compare/master...coverage-e2e
Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is a small change with no associated Issue.
Let's see if we can compare what's being tested in unit tests, and what's being tested in e2e tests (and what's not tested yet).
Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.