Skip to content

Add function for checking that code dirs have test#134

Closed
greghaynes wants to merge 1 commit into
knative:masterfrom
greghaynes:check-subdirs-no-tests
Closed

Add function for checking that code dirs have test#134
greghaynes wants to merge 1 commit into
knative:masterfrom
greghaynes:check-subdirs-no-tests

Conversation

@greghaynes
Copy link
Copy Markdown

There is a known issue where directories which contain code but no tests
are skipped in coverage reports. We work around this by adding a
stub_tests.go file in these directories.

@googlebot
Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@knative-prow-robot
Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: greghaynes
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: srinivashegde86

If they are not already assigned, you can assign the PR to them by writing /assign @srinivashegde86 in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

@knative-prow-robot knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 19, 2018
There is a known issue where directories which contain code but no tests
are skipped in coverage reports. We work around this by adding a
stub_tests.go file in these directories.
@greghaynes greghaynes force-pushed the check-subdirs-no-tests branch from 565537a to 6ea5528 Compare September 19, 2018 21:41
@googlebot
Copy link
Copy Markdown

CLAs look good, thanks!

@srinivashegde86
Copy link
Copy Markdown
Contributor

cc @steuhs
do we need a separate check here or this is a missing feature in the coverage tool?

Comment thread scripts/library.sh
# stub_tests.go with no tests to these directories.
function check_subdirs_without_tests() {
ret=0
for dir in $(find pkg -type d); do
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I assume you intended to replace pkg by $1 here?

Comment thread scripts/library.sh
# reports resulting in inaccurately high coverage results. Fix by adding a
# stub_tests.go with no tests to these directories.
function check_subdirs_without_tests() {
ret=0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

local please.

@adrcunha
Copy link
Copy Markdown
Contributor

/test pull-knative-test-infra-build-tests

@jessiezcc
Copy link
Copy Markdown
Contributor

@steuhs is solving the same problem with same solution AFAIK.

@steuhs
Copy link
Copy Markdown
Contributor

steuhs commented Oct 3, 2018

For the code coverage tool, I am planning on add _test.go to all .go file on the repo checked out locally. Once that is done, it will not only solve the issue mentioned here, but also cover all .go files. For now, this solution is a doable approach, but it will become redundant after the code coverage feature is updated accordingly.

@adrcunha
Copy link
Copy Markdown
Contributor

@steuhs can we close this PR now that your solution is implemented?

@steuhs
Copy link
Copy Markdown
Contributor

steuhs commented Oct 17, 2018

@adrcunha yes, we can close this
/close

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

7 participants