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

maintenance: send coverage (jacoco) to codecov #1094

Merged
merged 10 commits into from
Mar 20, 2023

Conversation

jeromevdl
Copy link
Contributor

Issue #, if available: #1080

Description of changes:

  • removed cobertura as we have also jacoco
  • merge results in parent target folder

Checklist

Breaking change checklist

RFC issue #:

  • Migration process documented
  • Implement warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov-commenter
Copy link

codecov-commenter commented Mar 17, 2023

Codecov Report

❗ No coverage uploaded for pull request base (master@b062a76). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #1094   +/-   ##
=========================================
  Coverage          ?   71.83%           
  Complexity        ?      523           
=========================================
  Files             ?       68           
  Lines             ?     2166           
  Branches          ?      230           
=========================================
  Hits              ?     1556           
  Misses            ?      503           
  Partials          ?      107           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jeromevdl
Copy link
Contributor Author

It works: https://app.codecov.io/github/awslabs/aws-lambda-powertools-java/tree/unittest_coverage_report

@@ -50,7 +50,7 @@ jobs:
name: Java ${{ matrix.java }}
env:
OS: ${{ matrix.os }}
JAVA: ${{ matrix.java-version }}
JAVA: ${{ matrix.java }}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me what the previous version of this was doing ..... shouldn't matrix.java-version have always been null? :|

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so. Not sure the environment variable is even used actually...

@@ -50,7 +50,7 @@ jobs:
name: Java ${{ matrix.java }}
env:
OS: ${{ matrix.os }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Only related to change insofar as matrix.java-version was also not defined in the matrix context. In this case I think matrix.os has been taken from the matrix examples but is being misused here because it is never set.

I suggest removing this. I can see no usages of OS within the codebase, and this is actively confusing. Adding the filter on the matrix to ensure codecov runs once adds to this - if we actually had a 2D matrix with OS as well, which this implies, the filter would not be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, will try to remove the os to see if it still works.

Copy link
Contributor

@scottgerring scottgerring left a comment

Choose a reason for hiding this comment

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

A couple comments inline that are tenuously related about the way we're using the build matrix

@jeromevdl
Copy link
Contributor Author

@scottgerring anything else?

Copy link
Contributor

@scottgerring scottgerring left a comment

Choose a reason for hiding this comment

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

LGTM!

@jeromevdl jeromevdl merged commit f0c9562 into master Mar 20, 2023
@jeromevdl jeromevdl deleted the unittest_coverage_report branch March 20, 2023 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants