CI - Reenable coverage and update workflow #3772
Conversation
samdoran
commented
Apr 9, 2024
samdoran
commented
- Rename the pull_request workflow to CI
- Run jobs on pushes to important branches not just on PRs
- Add codecov action
- Update test command to use jest flags
- Improve Markdown formatting
- Update missing links
- Change CI status badge to GitHub actions
- Add coverage badge
- Update docker compose commands
- Rename the pull_request workflow to CI - Run jobs on pushes to important branches not just on PRs - Add codecov action - Update test command to use jest flags
- Improve Markdown formatting - Update missing links - Change CI status badge to GitHub actions - Add coverage badge - Update docker compose commands
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3772 +/- ##
==========================================
- Coverage 65.12% 61.77% -3.36%
==========================================
Files 507 511 +4
Lines 9764 9918 +154
Branches 2090 2326 +236
==========================================
- Hits 6359 6127 -232
- Misses 3402 3545 +143
- Partials 3 246 +243 see 507 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
samdoran
force-pushed
the
ci/reenable-coverage
branch
from
855e7ab to
76d4575
Compare
dlabrecq
left a comment
dlabrecq
left a comment
There was a problem hiding this comment.
We're still relying on pull_request.xml to build PRs because the pipeline fails so often.
I was just expecting to re-add the "Upload coverage report" step at the bottom of that file.
| 1. Setup `/etc/hosts` entries listed above. | ||
| 1. Clone the repository, and open a terminal in the base of this project. | ||
| 1. Run the command `npm install` to install all the dependencies. |
There was a problem hiding this comment.
This is Markdown syntax for on ordered list. The correct numbers are used when the page is rendered. Makes reordering steps much less of a hassle.
There was a problem hiding this comment.
I'm happy to go back to using hard coded numbers, though.
| - name: Setup Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 18 |
There was a problem hiding this comment.
Why do we need separate jobs for lint and units? Each job runs many of the same steps twice
There was a problem hiding this comment.
Separate jobs lets them run concurrently, reducing overall runtime. There are some duplicate steps, but that's because the setup needs to happen on each runner.
There was a problem hiding this comment.
We could avoid the setup steps by using our own container image for CI runs. That's a much more reliable (and faster) way to run tests, but requires maintaining the CI image(s).
| branches: | ||
| - main | ||
| - prod-* | ||
| - stable-* |
There was a problem hiding this comment.
Do we need this to run on push? Can we just use pull_request.xml?
There was a problem hiding this comment.
Right now we don't have any CI runs on the code after it is merged into main or other branches. This would let us know the status of the code in those branches not just the PR branches.
There was a problem hiding this comment.
The other reason for this is integrity of the CI status badge. Without a test run against the stable branches, the CI status badge would only indicate that the last PR run was successful, not that the code in the stables branches was tested successfully.
| fail-fast: false | ||
| matrix: | ||
| node-version: | ||
| - '18' |
There was a problem hiding this comment.
Don't want to hard code this version, see ${{ matrix.node-version }}
There was a problem hiding this comment.
This is the definition of matrix.node-version. It looks funny because it is referenced in the name field, but that's how matrix works.
Do you mean the "ci.ext.devshift.net PR build" check fails often? I renamed The file can be named whatever we want. |
|
Speaking of ci.ext.devshift.net, that check is currently failing due to a bug (RedHatInsights/insights-frontend-builder-common#189). I'll have to change the branch name for this PR to work around it, which means this will get closed. I can open one PR per commit in this branch so it's easier to review. |
