-
Notifications
You must be signed in to change notification settings - Fork 164
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 code coverage on the entire knative/pkg repo #110
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: steuhs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
As discussed offline, try to validate this locally and then make final changes instead of using a complex trial-and-error approach. |
@@ -1094,7 +1094,7 @@ presubmits: | |||
- "--postsubmit-job-name=post-knative-pkg-go-coverage" | |||
- "--artifacts=$(ARTIFACTS)" | |||
- "--profile-name=coverage_profile.txt" | |||
- "--cov-target=./apis/ ./configmap/" | |||
- "--cov-target=." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a) Would that automatically exclude //vendor
and //test
?
b) Why not applying this pattern to all coverage jobs, since it makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- it automatically exclude vendor folder, but not test
- let's confirm with @jessiezcc and see if we want to apply this pattern to all repos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can u apply gitattribute to a folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest that you be consistent with specifying excluded directories in the tool to avoid confusion and conflict, and thus not automatically exclude //vendor
and leave that (and //test
, in this case) for .gitattributes
just like https://github.com/knative/serving/blob/665012885601188d00e54f4da8fe539cd28765be/.gitattributes#L11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get the coverage and then prune it back :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jessiezcc yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adrcunha it's go test's default behavior to ignore vendor. Do you mean to specify it explicitly in .gitattributes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mattmoor the current behavior is to get a complete coverage profile and then ignore those files that are marked as coverage-excluded in .gitattributes when summarizing. I guess this is what you meant, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@steuhs yes
/lgtm |
@adrcunha , as discussed with @mattmoor , we want to try running code coverage on ./