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

fix: better detect merge-commits #397

Merged
merged 1 commit into from
Mar 6, 2024
Merged

Conversation

giovanni-guidini
Copy link
Contributor

Apparently a common thing for CIs to do is create a merge-commit for changes
in a branch before running tests and stuff.
This means that - especially for not-directly-supported CIs - we would maybe
return a SHA for a commit that didn't exist in the branch.

These changes fix that by checking to see if the current commit is a merge commit.
If it is we return the second parent, the most recent commit before the current one.

Q: What if the current latest commit in the branch is a merge commit?
Well in this case the parent, which is also part of the branch, will have the coverage.

Users can still provide a commit sha value to override this behavior.

closes #372

Apparently a common thing for CIs to do is create a merge-commit for changes
in a branch before running tests and stuff.
This means that - especially for not-directly-supported CIs - we would maybe
return a SHA for a commit that didn't exist in the branch.

These changes fix that by checking to see if the current commit is a merge commit.
If it is we return the second parent, the most recent commit before the current one.

Q: What if the current latest commit in the branch is a merge commit?
Well in this case the parent, which is also part of the branch, will have the coverage.

Users can still provide a commit sha value to override this behavior.

closes #372
Copy link

codecov bot commented Mar 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.02%. Comparing base (92d7e89) to head (e8f8f77).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #397   +/-   ##
=======================================
  Coverage   96.02%   96.02%           
=======================================
  Files          81       81           
  Lines        2817     2821    +4     
=======================================
+ Hits         2705     2709    +4     
  Misses        112      112           
Flag Coverage Δ
python3.10 96.31% <100.00%> (+<0.01%) ⬆️
python3.11 96.31% <100.00%> (+<0.01%) ⬆️
python3.8 96.28% <100.00%> (+<0.01%) ⬆️
python3.9 96.28% <100.00%> (+<0.01%) ⬆️
smart-labels 96.02% <100.00%> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@matt-codecov matt-codecov left a comment

Choose a reason for hiding this comment

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

do we know which CIs do this? how do we tend to handle testing behaviors we add for specific CIs? do we have demo accounts on all of them?

@giovanni-guidini
Copy link
Contributor Author

do we know which CIs do this?

No, but I've been told "it's a common thing"

how do we tend to handle testing behaviors we add for specific CIs?

Specific CIs are typically the ones we directly support. We do unit tests, mostly.
The fallback values come from the CI's docs. The tests are here

do we have demo accounts on all of them?

I highly doubt that.

@giovanni-guidini giovanni-guidini merged commit ff1c300 into main Mar 6, 2024
18 checks passed
@giovanni-guidini giovanni-guidini deleted the gio/merge-commit branch March 6, 2024 19:15
@thomasrockhu-codecov
Copy link
Contributor

💯 thanks @giovanni-guidini!

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.

Correct SHA may not being chosen when used in a pre-merge commit
3 participants