🏃 e2e: replace fmt.Println with writes to ginko's writer #3065
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
There's a bunch of calls to |
k8s-ci-robot
added
size/L
There was a problem hiding this comment.
@cpanato thanks for this PR
I'm ok in getting rid of PrintLn to stdout, but I'm a little bit perplexed about the framework exposing a public Logf method. Is it possible to avoid this (e.g. move to an internal package)
test/framework/util.go
Outdated
| . "github.com/onsi/ginkgo" | ||
| ) | ||
|
|
||
| func Logf(format string, a ...interface{}) { |
There was a problem hiding this comment.
I'm debated on exposing this as a public method, first because logging is tangential to the goal to the framework, second because in the long term the goal is to make the framework to work without ginkgo & gomega: see
#2306 (comment)
#2955
There was a problem hiding this comment.
made the changes @fabianofranz let me know what do you think when you have time :)
cpanato
force-pushed
the
GH-2514
branch
3 times, most recently
from
13226e7
to
08c17de
Compare
test/e2e/common.go
Outdated
| @@ -46,6 +46,10 @@ func Byf(format string, a ...interface{}) { | |||
| By(fmt.Sprintf(format, a...)) | |||
| } | |||
|
|
|||
| func Logf(format string, a ...interface{}) { | |||
There was a problem hiding this comment.
Can we use framework/internal/log everywhere?
There was a problem hiding this comment.
@cpanato Any reason do we want to expose this as an API, or do we want to keep it internal?
There was a problem hiding this comment.
@sedefsavas the on in framework is internal and only accessible in the framework and we cant access from the e2e tests package
There was a problem hiding this comment.
i can change that to be internal @vincepri , no reasons, i just follow the same approach i saw somewhere in k/k
There was a problem hiding this comment.
@fabriziopandini @sedefsavas thoughts?
There was a problem hiding this comment.
Please change this to private
(also here, logging is tangential to the goal of test/e2e and we don't want to expose methods for it)
There was a problem hiding this comment.
done @fabriziopandini PTAL when you have time. thanks!
k8s-ci-robot
added
the
needs-rebase
k8s-ci-robot
removed
the
needs-rebase
|
/approve |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: benmoss, cpanato, fabriziopandini 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 |
k8s-ci-robot
added
the
approved
What this PR does / why we need it:
replace fmt.Println with writes to ginko's writer
Let me know if this is what you want :)
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):/cc @chuckha