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

Add codecov badge #69

Merged
merged 3 commits into from Oct 12, 2022
Merged

Add codecov badge #69

merged 3 commits into from Oct 12, 2022

Conversation

dfed
Copy link
Owner

@dfed dfed commented Oct 11, 2022

As title

@dfed dfed requested review from fdiaz and bachand October 11, 2022 20:24
@dfed dfed marked this pull request as ready for review October 11, 2022 20:29
Copy link
Collaborator

@bachand bachand left a comment

Choose a reason for hiding this comment

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

Approving though it sounds like we're still having some issues with codecov (see #67 (comment)) so it may be best if we feel more confident before we add the badge.

Separately, would it be expected that this PR itself has a codecov comment?

@dfed
Copy link
Owner Author

dfed commented Oct 12, 2022

Approving though it sounds like we're still having some issues with codecov (see #67 (comment)) so it may be best if we feel more confident before we add the badge.

Yup. Going to try to get the other PR working before I merge this.

Separately, would it be expected that this PR itself has a codecov comment?

That we would! Working on it in #68

@dfed
Copy link
Owner Author

dfed commented Oct 12, 2022

Now that #68 is merged, I'll get this in once CI goes green again.

@codecov
Copy link

codecov bot commented Oct 12, 2022

Codecov Report

Merging #69 (ea12b06) into main (16e6563) will not change coverage.
The diff coverage is n/a.

❗ Current head ea12b06 differs from pull request most recent head bb99592. Consider uploading reports for the commit bb99592 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #69   +/-   ##
=======================================
  Coverage   96.26%   96.26%           
=======================================
  Files          14       14           
  Lines         562      562           
=======================================
  Hits          541      541           
  Misses         21       21           

@dfed dfed merged commit fd03b6d into main Oct 12, 2022
@dfed dfed deleted the dfed--codecov-badge branch October 12, 2022 18:43
@bachand
Copy link
Collaborator

bachand commented Oct 12, 2022

@dfed I'm wondering how we can avoid this codecov warning?
Screen Shot 2022-10-12 at 2 29 46 PM

It seems like when you pushed bb99592 we somehow didn't update codecov with the necessary information to recompute code coverage?

I came to that conclusion since that I see that codecov added this warning later, and the original message is comparing ea12b06, the previous commit, to the base branch.

Screen Shot 2022-10-12 at 2 30 18 PM

@bachand
Copy link
Collaborator

bachand commented Oct 12, 2022

If you'd prefer I can file an issue on this too. Not trying to add more work. You've already done some amazing KTLO work on this repo this week :)

@dfed
Copy link
Owner Author

dfed commented Oct 12, 2022

Oh I know what happened here @bachand. I merged main into this branch, then merged before CI completed since I was only updating the README.

My guess is that codecov doesn't update posts on closed/merged PRs, so we never got to see the update for the latest commit.

I guess that teaches me not to take shortcuts 😅

@bachand
Copy link
Collaborator

bachand commented Oct 12, 2022

Ah OK I'm relieved we have a good theory 👍

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.

None yet

2 participants