Skip to content
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

feat: [M3-7128] - Coverage Report - Part 1 #9772

Closed
wants to merge 62 commits into from

Conversation

abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Oct 9, 2023

Description 📝

This PR is Part 1 of 2.

The way GH actions work, we need the base job ("Code Coverage") to be in the development branch for the depending job ("Coverage Comment") to trigger. The reason we have two different jobs is because of being able to comment on forked PRs and secrets are stripped from jobs triggered by forked pull_request.
I assume tuning will be needed once this is merged and it may create some noise for a couple days. No big deal, none of this will stop a PR from bring merged and will announce the changes/disturbance in our channel. No pain no gain.

Note: changeset will be added in next PR!
Second Note: Please pardon the many commits and notices, it was an experimental branch that got prematurely put in review.

Changes 🔄

  • fix the coverage generation
  • implements two github action for reporting on open PRs
    • one to generate the coverage
    • one to comment on the PR since we can't comment on forked PRs with on: [pull_request]
  • make sure the CI runs the coverage report

Preview 📷

Screenshot 2023-11-14 at 20 08 21

How to test 🧪

  • run yarn to update your dependencies
  • pull code locally and run yarn coverage

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@abailly-akamai abailly-akamai self-assigned this Oct 10, 2023
@abailly-akamai abailly-akamai force-pushed the M3-7128 branch 2 times, most recently from 3344459 to 3b6e039 Compare October 16, 2023 15:43
@abailly-akamai abailly-akamai changed the title feat: [M3-7128] - Coverage Report feat: [M3-7128] - Coverage Report - Part 1 Oct 16, 2023
@@ -53,7 +53,6 @@
"lodash": "^4.17.21",
"glob-parent": "^5.1.2",
"hosted-git-info": "^5.0.0",
"minimatch": "^9.0.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need a resolution here. This was part of the reason the coverage script was not running - this was last changed when I worked on the perfectionist linting job and we had a conflict. Instead of a resolution, we should allow both versions to be available.

@abailly-akamai abailly-akamai marked this pull request as ready for review October 16, 2023 19:21
@abailly-akamai abailly-akamai requested a review from a team as a code owner October 16, 2023 19:21
@abailly-akamai abailly-akamai requested review from jdamore-linode, jaalah-akamai, bnussman-akamai and coliu-akamai and removed request for a team October 16, 2023 19:21
@abailly-akamai abailly-akamai marked this pull request as draft October 18, 2023 19:51
@abailly-akamai abailly-akamai dismissed bnussman-akamai’s stale review October 18, 2023 19:56

Put back into draft cause I need to rework a couple things

@linode linode deleted a comment from linode-gh-bot Oct 19, 2023
@linode linode deleted a comment from linode-gh-bot Oct 19, 2023
@linode linode deleted a comment from linode-gh-bot Oct 20, 2023
@linode linode deleted a comment from linode-gh-bot Nov 9, 2023
@linode linode deleted a comment from linode-gh-bot Nov 13, 2023
@linode linode deleted a comment from linode-gh-bot Nov 13, 2023
uses: actions/upload-artifact@v3
with:
name: ref_code_coverage
path: ref_code_coverage.txt
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will fail right now cause code coverage isn't working on develop. Once merged, coverage will start running again and this will succeed

repo-token: ${{ secrets.GITHUB_TOKEN }}
message: |
$(cat updated_comment.txt)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this action won't run till merged to develop. I am expecting to iterate on it on part 2, or maybe it'll just work!

<p align="center">
<img alt="Linode/Manager Coverage" src="https://img.shields.io/badge/%40linode%2Fmanager_coverage-N%2FA%25-yellow" />
</p>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're adding a badge to our front page. This should work once merged, although i may need to add our token to the job.

"!src/**/*.hooks.{js,jsx,ts,tsx}",
"!src/**/*.stories.{js,jsx,ts,tsx}",
"!src/**/index.{js,jsx,ts,tsx}",
"!src/**/*.styles.{js,jsx,ts,tsx}"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may want to iterate on those as we go. Basically we want to collect coverage from the most relevant code paths. For instance, it does not make sense to include all features in the coverage however we want to include the utilities from there.

@abailly-akamai abailly-akamai marked this pull request as ready for review November 15, 2023 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants