Introduce logverbose tag for test cases #1054

jessiezcc · 2018-06-05T00:16:40Z

update err msg with both expected and actual state

adrcunha · 2018-06-05T02:50:51Z

test/e2e/errorcondition_test.go

@@ -93,10 +97,10 @@ func TestContainerErrorMsg(t *testing.T) {
 		t.Fatalf("Failed to get logUrl from revision %s: %v", revisionName, err)
 	}

-	// TODO(jessiezcc@): actually validate the logURL, but requires kibana setup
+	// TODO(jessiezcc): actually validate the logURL, but requires kibana setup
 	log.Printf("LogURL: %s", logURL)


I mentioned about this log message causing noise in the output, please consider addressing it in this PR.

adrcunha · 2018-06-05T02:51:55Z

test/e2e/errorcondition_test.go

 	log.Printf("LogURL: %s", logURL)

-	// TODO(jessiezcc@): add the check to validate that Route is not marked as ready once https://github.com/elafros/elafros/issues/990 is fixed
+	// TODO(jessiezcc): add the check to validate that Route is not marked as ready once https://github.com/elafros/elafros/issues/990 is fixed
 }

 // Get revision name from configuration.


Please also consider fixing the comments of the functions in this PR.

Nevermind, this are not public method anymore.

knative-metrics-robot · 2018-06-05T02:54:39Z

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta

*TestCoverage feature is being tested, do not rely on any info here yet

adrcunha · 2018-06-05T20:03:57Z

/test pull-knative-serving-integration-tests

knative-metrics-robot · 2018-06-05T22:53:09Z

The following is the coverage report on pkg/. Say /test pull-knative-serving-go-coverage to run the coverage report again

File	Old Coverage	New Coverage	Delta

*TestCoverage feature is being tested, do not rely on any info here yet

adrcunha · 2018-06-06T16:21:03Z

test/e2e_flags.go

+}
+
+// VerboseGLog outputs verbose logging with defined logleve 10.
+func VerboseGLog(s string) {


Why not simply Debug?

would like to go with log level so that u can get all logs coming out of k8s libs as well.

I mean calling the method "Debug", just for simplicity sake.

ah, how about Verbose()? Debug is usually meant to be a build flavor, RTL vs. DBG

adrcunha · 2018-06-06T16:22:58Z

test/e2e_flags.go

+	if *boolPtr {
+		flag.Set("alsologtostderr", fmt.Sprintf("%t", true))
+		var logLevel string
+		flag.StringVar(&logLevel, "logLevel", "10", "verbose log level")


If you're going this path, I suggest defining a constant for the "verbose" output and sticking to it in all the related code.

@bobcatfish I didn't check glog, is it the best approach? I confess I was expecting something simpler to use...

that's the reason that I'm wrapping verbose log in a function to be called by TC.
started with constant, however, couldn't get gLog.V() to work, hence the function.

Sorry, I don't follow. I understand the initialization and the use of V(), but I fail to understand the relation to the wrapping (which I like) and not being able to use a constant for V().

will explain f2f when u are back to desk, easier that way

adrcunha · 2018-06-06T16:24:49Z

test/e2e_flags.go

@@ -58,3 +60,21 @@ func initializeFlags() *EnvironmentFlags {

 	return &f
 }
+
+func init() {


Any reason to not initialize these flags in initializeFlags()?

No particular reason, can merge into initializeFlags.

adrcunha · 2018-06-06T16:25:24Z

test/e2e_flags.go

@@ -58,3 +60,21 @@ func initializeFlags() *EnvironmentFlags {

 	return &f
 }
+
+func init() {
+	boolPtr := flag.Bool("logVerbose", false, "a bool")


I'm pretty sure Christie would love seeing "a bool" turned into a way more descriptive text about this flag. :D

yep, definitely

a) Can you do it here? :)

b) Flag usage in the other files use the "test.Flags." aproach, which is cleaner. Can't you use that?

[jessie] fix and pushed

adrcunha · 2018-06-06T16:27:50Z

test/e2e_flags.go

+	flag.Parse()
+
+	if *boolPtr {
+		flag.Set("alsologtostderr", fmt.Sprintf("%t", true))


flag.Set("alsologtostderr", "true")

Interesting, I thought glog would output directly to stdout/stderr by default.

Looks like I wasn't clear here: replace fmt.Sprintf() with just "true".

[jessie] fixed

adrcunha · 2018-06-06T16:28:56Z

test/e2e_flags.go

+		var logLevel string
+		flag.StringVar(&logLevel, "logLevel", "10", "verbose log level")
+		flag.Lookup("v").Value.Set(logLevel)
+		glog.Info("logVerbose %v", boolPtr)


boolPtr will be always true here. What about simply

glog.Info("Logging set to verbose mode")

?

adrcunha · 2018-06-06T16:35:58Z

I like the (simple) wrapping approach because it requires less boilerplate code, thus there're (fewer) excuses for not logging things appropriately.

jessiezcc · 2018-06-06T17:58:05Z

@bobcatfish, are you ok with function approach? Test case will call test.Verbose(...) .
If we are all in agreement with function, I will go ahead update the doc.

jessiezcc · 2018-06-06T18:01:38Z

also @adrcunha, @bobcatfish , are you fine with 2 PRs approach:
First PR to introduce verbose flag and function, call verbose function from error condition test case and relevant doc changes.
Second PR to switch all test logs from log to glog, and relevant doc changes.

Verbose func update

adrcunha · 2018-06-06T19:06:13Z

test/e2e_flags.go

 		flag.Lookup("v").Value.Set(logLevel)
-		glog.Info("logVerbose %v", boolPtr)
+		glog.Infof("Logging set to verbose mode with logleve %d", VerboseLogLevel)


typo: logLevel

Looks like you missed the typo. :)

[Jessie] also fixed

adrcunha · 2018-06-06T19:06:43Z

test/e2e_flags.go

 	boolPtr := flag.Bool("logVerbose", false, "a bool")
 	flag.Parse()

 	if *boolPtr {
 		flag.Set("alsologtostderr", fmt.Sprintf("%t", true))
 		var logLevel string
-		flag.StringVar(&logLevel, "logLevel", "10", "verbose log level")
+		flag.StringVar(&logLevel, "logLevel", fmt.Sprint(VerboseLogLevel), "verbose log level")


typo: Sprintf

Would string() work here?

tried string, didn't work

adrcunha · 2018-06-06T19:08:09Z

test/e2e_flags.go

-// VerboseGLog outputs verbose logging with defined logleve 10.
-func VerboseGLog(s string) {
-	glog.V(10).Infof(s)
+// Verbose outputs verbose logging with defined logleve 10.


typo: logLevel

But I would replace "logLevel 10" with "VerboseLogLevel".

…ster

adrcunha · 2018-06-06T20:54:22Z

test/e2e_flags.go

@@ -58,3 +60,21 @@ func initializeFlags() *EnvironmentFlags {

 	return &f
 }
+
+func init() {
+	boolPtr := flag.Bool("logVerbose", false, "a bool")


a) Can you do it here? :)

b) Flag usage in the other files use the "test.Flags." aproach, which is cleaner. Can't you use that?

[jessie] fix and pushed

adrcunha · 2018-06-06T20:59:24Z

test/e2e_flags.go

+	flag.Parse()
+
+	if *boolPtr {
+		flag.Set("alsologtostderr", fmt.Sprintf("%t", true))


Looks like I wasn't clear here: replace fmt.Sprintf() with just "true".

[jessie] fixed

adrcunha · 2018-06-06T21:00:10Z

test/e2e_flags.go

 		flag.Lookup("v").Value.Set(logLevel)
-		glog.Info("logVerbose %v", boolPtr)
+		glog.Infof("Logging set to verbose mode with logleve %d", VerboseLogLevel)


Looks like you missed the typo. :)

[Jessie] also fixed

jessiezcc · 2018-06-06T21:47:56Z

/retest

adrcunha · 2018-06-06T23:06:29Z

/lgtm
/approve

google-prow-robot · 2018-06-06T23:06:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrcunha, jessiezcc

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:

~~test/OWNERS~~ [adrcunha,jessiezcc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

bobcatfish

imo if you also update the e2e scripts so that they enable verbose logging, we can resolve #763 as well :D

bobcatfish · 2018-06-06T23:34:45Z

test/e2e_flags.go

+		var logLevel string
+		flag.StringVar(&logLevel, "logLevel", fmt.Sprint(VerboseLogLevel), "verbose log level")
+		flag.Lookup("v").Value.Set(logLevel)
+		glog.Infof("Logging set to verbose mode with logLevel %d", VerboseLogLevel)


I will need to update doc, will include changes for #763 as well.

bobcatfish · 2018-06-06T23:39:23Z

@bobcatfish, are you ok with function approach? Test case will call test.Verbose(...) .
If we are all in agreement with function, I will go ahead update the doc.

I still have a different opinion on this but I don't feel strongly! I say go ahead with it and we can see how it turns out, wouldn't be hard to change later if we change our minds.

jessiezcc · 2018-06-06T23:58:42Z

/retest

jessiezcc added 2 commits June 4, 2018 17:08

update err msg with both expected and actual state

ad25056

add back error output

f790a03

jessiezcc requested a review from adrcunha June 5, 2018 00:16

google-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 5, 2018

Fix username in TODO

739e2b8

adrcunha reviewed Jun 5, 2018

View reviewed changes

jessiezcc added 2 commits June 5, 2018 15:44

use glog to control verbose logging

0c280f2

Merge branch 'master' of github.com:jessiezcc/knative

62dbc0e

google-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 5, 2018

move verbose function to e2e_flags.go

2c2a0bc

adrcunha reviewed Jun 6, 2018

View reviewed changes

use constant for verbose loglevel

fa39fd3

Verbose func update

adrcunha suggested changes Jun 6, 2018

View reviewed changes

jessiezcc added 3 commits June 6, 2018 12:13

update comments, fix typo

93896cc

Merge branch 'master' of github.com:knative/serving into jessiezcc-ma…

48c7510

…ster

Merge branch 'jessiezcc-master'

5fbc83f

adrcunha suggested changes Jun 6, 2018

View reviewed changes

jessiezcc added 2 commits June 6, 2018 14:11

merged change broke error condition TC

dd04cf0

Add LogVerbose as part of EnvironmentFlags

55c6ccc

add more comments for setting gLog loglevel

08d3765

adrcunha approved these changes Jun 6, 2018

View reviewed changes

google-prow-robot assigned adrcunha Jun 6, 2018

google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 6, 2018

bobcatfish reviewed Jun 6, 2018

View reviewed changes

jessiezcc changed the title ~~update err msg with both expected and actual state~~ Introduce logverbose tag for test cases Jun 6, 2018

google-prow-robot merged commit 0957785 into knative:master Jun 7, 2018

Introduce logverbose tag for test cases #1054

Introduce logverbose tag for test cases #1054

Conversation

jessiezcc commented Jun 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-metrics-robot commented Jun 5, 2018

adrcunha commented Jun 5, 2018

knative-metrics-robot commented Jun 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrcunha Jun 6, 2018 • edited by jessiezcc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrcunha Jun 6, 2018 • edited by jessiezcc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrcunha commented Jun 6, 2018

jessiezcc commented Jun 6, 2018

jessiezcc commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrcunha Jun 6, 2018 • edited by jessiezcc Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrcunha Jun 6, 2018 • edited by jessiezcc Loading

Choose a reason for hiding this comment

adrcunha Jun 6, 2018 • edited by jessiezcc Loading

Choose a reason for hiding this comment

adrcunha Jun 6, 2018 • edited by jessiezcc Loading

Choose a reason for hiding this comment

jessiezcc commented Jun 6, 2018

adrcunha commented Jun 6, 2018

google-prow-robot commented Jun 6, 2018

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcatfish commented Jun 6, 2018

jessiezcc commented Jun 6, 2018

adrcunha Jun 6, 2018 •

edited by jessiezcc

Loading

adrcunha Jun 6, 2018 •

edited by jessiezcc

Loading

adrcunha Jun 6, 2018 •

edited by jessiezcc

Loading

adrcunha Jun 6, 2018 •

edited by jessiezcc

Loading

adrcunha Jun 6, 2018 •

edited by jessiezcc

Loading

adrcunha Jun 6, 2018 •

edited by jessiezcc

Loading