Use t.Logf instead of glog in tests
#248
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
approved
Codecov Report
@@ Coverage Diff @@
## master #248 +/- ##
======================================
Coverage 53.6% 53.6%
======================================
Files 16 16
Lines 735 735
======================================
Hits 394 394
Misses 289 289
Partials 52 52Continue to review full report at Codecov.
|
hack/verify-code-patterns.sh
Outdated
| @@ -34,3 +35,11 @@ if [[ -n "$out" ]]; then | |||
| echo >&2 "$out" | |||
| exit 1 | |||
| fi | |||
|
|
|||
| # Do not use glog in test code | |||
| out="$(grep --include '*_test.go' --exclude-dir 'vendor/' -EIrn 'github.com/golang/glog' || true)" | |||
There was a problem hiding this comment.
I recommend making the pattern based on invocation [kg]log\. as we might move to klog at some point (in fact we probably should). Similarly, import paths are more likely to change than the usage (e.g. this could change to golang.org/glog someday).
There was a problem hiding this comment.
My thought was to avoid false positives as glog is rather short. But for now there are no collisions and we can think of a better way if a collision should occur. 👍
integration_test/install_test.go
Outdated
| @@ -25,3 +34,42 @@ func TestKrewInstall(t *testing.T) { | |||
| test.WithIndex().Krew("install", validPlugin).RunOrFailOutput() | |||
| test.AssertExecutableInPATH("kubectl-" + validPlugin) | |||
| } | |||
|
|
|||
| func TestKrewInstall_Manifest(t *testing.T) { | |||
There was a problem hiding this comment.
I think the other PR got mixed in here :)
There was a problem hiding this comment.
gosh, how embarrassing 🙄
integration_test/testutil_test.go
Outdated
| @@ -166,7 +165,7 @@ func (it *ITest) WithIndex() *ITest { | |||
| // WithEnv sets an environment variable for the krew run. | |||
| func (it *ITest) WithEnv(key string, value interface{}) *ITest { | |||
| if key == "KREW_ROOT" { | |||
| glog.V(1).Infoln("Overriding KREW_ROOT in tests is forbidden") | |||
| it.t.Log("Overriding KREW_ROOT in tests is forbidden") | |||
There was a problem hiding this comment.
out of scope but I think this is more suitable for a t.Fatal
There was a problem hiding this comment.
It's a small but important thing, so I changed that.
corneliusweig
force-pushed
the
w/no-glog-in-tests
branch
from
fac4291
to
8c91c6e
Compare
k8s-ci-robot
added
size/S
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahmetb, corneliusweig 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 |
Fix #247