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

Run Codecov Workflow only when PR is ready #1114

Merged

Conversation

jiji14
Copy link
Contributor

@jiji14 jiji14 commented Dec 5, 2023

Why Change the Workflow?

Sending a coverage report to Codecov with GitHub Action without a token encounters a rate limit issue. For more details, please refer to the comments on issue #1085.

In the current flow, the Codecov workflow runs whenever we push code to PR. To address the rate limit issue, we have decided to run the workflow only when we are ready for review.

When PR is draft

Screenshot 2023-12-05 at 2 27 51 PM

@jiji14 jiji14 marked this pull request as draft December 5, 2023 22:06
@jiji14 jiji14 marked this pull request as ready for review December 5, 2023 22:28
Copy link

codecov bot commented Dec 5, 2023

Codecov Report

Merging #1114 (1901f4c) into service_rewrite_2023 (1006165) will not change coverage.
Report is 43 commits behind head on service_rewrite_2023.
The diff coverage is n/a.

Additional details and impacted files
@@                  Coverage Diff                  @@
##           service_rewrite_2023    #1114   +/-   ##
=====================================================
  Coverage                 58.80%   58.80%           
=====================================================
  Files                        26       26           
  Lines                      1420     1420           
  Branches                    320      320           
=====================================================
  Hits                        835      835           
  Misses                      585      585           
Flag Coverage Δ
unit 58.80% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@jiji14
Copy link
Contributor Author

jiji14 commented Dec 5, 2023

@JGreenlee
Jest runs on both the osx-serve-install and codecov workflows. I think it's enough to execute Jest when the PR is ready, specifically in the code coverage workflow. What are your thoughts? If you agree, I will delete Jest execution in osx-serve-install

@JGreenlee
Copy link
Collaborator

JGreenlee commented Dec 5, 2023

@jiji14 I think as long as the expectation is for contributors to run the tests locally before they mark a PR as ready, this makes sense.

The plan was to extract Jest testing out of the osx-serve-install workflow at some point anyway

@jiji14
Copy link
Contributor Author

jiji14 commented Dec 5, 2023

@JGreenlee
I believe most contributors run the tests locally, but we can't guarantee that every contributor does so. I guess the safest approach is to keep Jest in osx-serve-install workflow

@JGreenlee
Copy link
Collaborator

If a contributor forgets to run tests locally, they would mark it ready for review causing the workflow to run, and checks might fail.

In this scenario, do they have a way to re-trigger the workflow after making fixes? Can they mark it ready for review again?
If so, I think it's fine to remove Jest from the osx-serve-install workflow.

@jiji14
Copy link
Contributor Author

jiji14 commented Dec 7, 2023

@JGreenlee

I've just verified that Codecov is executed whenever the PR status transitions from Draft to Ready to Review in this workflow.

In your scenario,

  1. If a contributor forgets to run tests and the initial attempt fails,
  2. They can switch the PR to draft status to address the issues.
  3. When the necessary fixes are made, they can mark the PR as Ready to Review, triggering Codecov.

Does this clarify the process?

@JGreenlee
Copy link
Collaborator

Does this clarify the process?

Yes it does!

If so, I think it's fine to remove Jest from the osx-serve-install workflow.

Then I think we can remove Jest from the osx-serve-install workflow to avoid running the tests twice.

Copy link
Collaborator

@JGreenlee JGreenlee left a comment

Choose a reason for hiding this comment

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

LGTM!

@shankari shankari merged commit 8070e76 into e-mission:service_rewrite_2023 Dec 20, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants