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 code coverage report #1932

Merged
merged 1 commit into from
Feb 15, 2024
Merged

Conversation

torredil
Copy link
Member

What is this PR about? / Why do we need it?

This PR makes modifications to make test to generate a test coverage report that explicitly excludes all generated mock files, giving contributors and maintainers more insight into code coverage.

In place of using the grep tech to filter out mock files, I initially attempted to split out mocked code into its own separate package. However, I quickly discovered that this is not possible due to a circular dependencies issue. To prevent the import cycle we'd have to bring in the tests over to the mock package as well - this is undesirable as a) it would effectively break the code coverage measurement altogether and b) not a standard Go practice.

What testing is done?

make test
Screenshot 2024-02-13 at 6 24 20 PM

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 13, 2024
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 13, 2024
Copy link

Code Coverage Diff

This PR does not change the code coverage

@@ -74,7 +74,10 @@ clean:

.PHONY: test
test:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer a separate target (perhaps make test/coverage?) for generating the test coverage profile. I believe make test should be reserved for just running the unit tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added new target as requested: make test/coverage

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I just realized while test/coverage is in line with what we use to separate sub-commands of other make targets (e.g. e2e/cluster, the test-sanity target now sticks out. I think this is fine though (if anything we should consider changing test-sanity to test/sanity in a separate PR).

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 14, 2024
Copy link
Contributor

@AndrewSirenko AndrewSirenko left a comment

Choose a reason for hiding this comment

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

Successfully ran on Linux. Left one more comment about adding a section to our makefile documentation. Thanks for this!

@@ -9,8 +9,9 @@ bin
# Test binary, build with `go test -c`
Copy link
Contributor

Choose a reason for hiding this comment

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

Top level comment: Consider adding a section to local-development section of our makefile documentation. Perhaps something like:

### `make test/coverage`

Outputs a filtered version of the each package's unit test coverage profiling via go's coverage tool to a local `coverage.html` file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks - added ✅

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2024
Makefile Outdated
@@ -76,6 +76,13 @@ clean:
test:
go test -v -race ./cmd/... ./pkg/...

.PHONY: test/coverage
test/coverage:
go test -v -race -coverprofile=cover.out ./cmd/... ./pkg/...
Copy link
Contributor

Choose a reason for hiding this comment

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

np: could probably ditch the -v and -race as this is separate from make test
/lgtm

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks - changed ✅

Signed-off-by: Eddie Torres <[email protected]>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2024
@torredil
Copy link
Member Author

/retest

3 similar comments
@ConnorJC3
Copy link
Contributor

/retest

@ConnorJC3
Copy link
Contributor

/retest

@ConnorJC3
Copy link
Contributor

/retest

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2024
@ConnorJC3
Copy link
Contributor

/retest

@AndrewSirenko
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AndrewSirenko

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 15, 2024
@k8s-ci-robot k8s-ci-robot merged commit 67f3920 into kubernetes-sigs:master Feb 15, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants