Test failure output improvements #316

jrajahalme · 2021-06-14T19:51:09Z

Move pause from test to action so that new no test traffic will be generated after the failing action
Suppress logs for 0 flows received

connectivity/check/action.go

ti-mo · 2021-06-15T11:43:12Z

connectivity/check/action.go

 		}
 	} else {
 		if !a.shouldSucceed() {
 			a.Failf("command %q succeeded while it should have failed: %s", cmdStr, stdout.String())
 		}
 	}
+	if a.failed {
+		if stderr.Len() > 0 {
+			a.Warnf("%s stderr: %s", cmdName, stderr.String())


Warn is for bringing unexpected events to the user's attention that are not fatal to test execution, and they are logged separately at the end of the test run to make sure they're not missed in a flood of output.

I'd prefer to have Infof() (or even Failf()) here instead, which will be kept in line with the test results, and is more suited to multi-line output. (which is generally the case for stdout/err) Unexpected stderr is a failure, not a warning.

Failf() sounds good to me ✅

I was thinking the exit code of the execution would be sufficient to indicate the success/fail status of the command. No trouble to make this a Failf() though, will add a comment that we consider any stderr output to indicate a failing test as well.

Making both stderr and stdout logs as Failfs, we are not logging these only if the command failed, after all.

Was considering if we'd still need to log these at debug level if the action did not fail, though. Thoughts?

Changed this to log stderr and stdout at fail level if the action failed (i.e., command succeeded when it should not have, or failed when it should have succeeded), and at debug level otherwise (to always see the output, if any, with --debug).

ti-mo · 2021-06-15T12:11:29Z

connectivity/check/logging.go

@@ -345,11 +345,12 @@ func (a *Action) Infof(format string, s ...interface{}) {
 }

 // Warn must be called when a warning is detected performing the Action.
-//
+// Warnings are logged both inline and at the end of a test.


Could we keep the Warn*() family of functions as-is? See my comment above, unexpected stderr is not a warning, it's a failure.

@ti-mo I find it odd that one logging level is not logged inline while every other level is. Maybe my bad that I'm thinking of these logging functions in terms of "loglevels". I have nothing against highlighting specific log levels separately, but I think all logs should appear in the output in the order in which they were generated.

If we need warning functions that do not log inline we should call them something else instead to not cause confusion with usage of Warn() and Warnf() from other Go packages.

Sure, I can see how echoing them inline in the test's log would be useful as well, but we need to keep in mind they are also repeated at the end of the test run. They're not supposed to be used frequently and we should avoid using them for multi-line output.

Or maybe we should just remove Warn() altogether. :) Being indecisive about events ("it's a failure but not really") is a slippery slope because that's how things start rotting. If it doesn't fail, no one looks at it in CI. It was originally intended to be used for flagging deprecated things etc., but currently it's only used for the Unable to contact Hubble Relay... warning. If it causes more trouble/confusion than it's worth, let's remove it.

I'll separate this change to a PR of its own.

ti-mo

LGTM! Few nits.

connectivity/check/action.go

ti-mo · 2021-07-13T13:34:13Z

@jrajahalme btw, I already proposed something similar in #251 (0c690f9)

jrajahalme · 2021-07-20T09:55:59Z

@jrajahalme btw, I already proposed something similar in #251 (0c690f9)

Rebased due to this.

ti-mo

Few nits, but 🚀

connectivity/check/action.go

jrajahalme · 2021-08-11T11:51:28Z

Rebased

Move the pause on fail wait from test to action, so that nothing happens after the failed action. Signed-off-by: Jarno Rajahalme <[email protected]>

Suppress repeated output that occurs when no flow have been received yet, i.e., the first two lines of: Validating 0 flows against 2 requirements Validating 0 flows against 2 requirements Validating 23 flows against 2 requirements Signed-off-by: Jarno Rajahalme <[email protected]>

jrajahalme added the kind/feature New feature or request label Jun 14, 2021

jrajahalme requested review from a team and twpayne June 14, 2021 19:51

jrajahalme had a problem deploying to ci June 14, 2021 19:51 Error

jrajahalme force-pushed the pr/jrajahalme/test-failure-output-improvements branch from 9fd99f0 to ae91f98 Compare June 14, 2021 19:51

jrajahalme requested a review from ti-mo June 14, 2021 19:51

jrajahalme temporarily deployed to ci June 14, 2021 19:51 Inactive

michi-covalent reviewed Jun 14, 2021

View reviewed changes

connectivity/check/action.go Outdated Show resolved Hide resolved

ti-mo requested changes Jun 15, 2021

View reviewed changes

jrajahalme force-pushed the pr/jrajahalme/test-failure-output-improvements branch from ae91f98 to aba3152 Compare June 16, 2021 16:09

jrajahalme requested a review from a team as a code owner June 16, 2021 16:09

maintainer-s-little-helper bot assigned twpayne Jun 16, 2021

maintainer-s-little-helper bot requested review from ti-mo and removed request for a team June 16, 2021 16:09

maintainer-s-little-helper bot assigned ti-mo Jun 16, 2021

jrajahalme temporarily deployed to ci June 16, 2021 16:09 Inactive

jrajahalme requested a review from michi-covalent June 16, 2021 20:53

maintainer-s-little-helper bot assigned michi-covalent Jun 16, 2021

jrajahalme force-pushed the pr/jrajahalme/test-failure-output-improvements branch from aba3152 to e976e72 Compare June 18, 2021 15:20

jrajahalme temporarily deployed to ci June 18, 2021 15:20 Inactive

ti-mo approved these changes Jul 9, 2021

View reviewed changes

connectivity/check/action.go Outdated Show resolved Hide resolved

connectivity/check/action.go Outdated Show resolved Hide resolved

connectivity/check/action.go Outdated Show resolved Hide resolved

maintainer-s-little-helper bot unassigned ti-mo Jul 9, 2021

jrajahalme force-pushed the pr/jrajahalme/test-failure-output-improvements branch from e976e72 to 5cf1487 Compare July 20, 2021 09:53

jrajahalme temporarily deployed to ci July 20, 2021 09:53 Inactive

ti-mo approved these changes Jul 20, 2021

View reviewed changes

connectivity/check/action.go Outdated Show resolved Hide resolved

connectivity/check/action.go Outdated Show resolved Hide resolved

jrajahalme added the area/CI Continuous Integration testing issue or flake label Jul 21, 2021

jrajahalme force-pushed the pr/jrajahalme/test-failure-output-improvements branch from 5cf1487 to 42d454c Compare July 21, 2021 15:47

jrajahalme temporarily deployed to ci July 21, 2021 15:47 Inactive

ti-mo approved these changes Jul 21, 2021

View reviewed changes

michi-covalent approved these changes Jul 28, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned michi-covalent Jul 28, 2021

twpayne approved these changes Jul 28, 2021

View reviewed changes

maintainer-s-little-helper bot unassigned twpayne Jul 28, 2021

jrajahalme force-pushed the pr/jrajahalme/test-failure-output-improvements branch from 42d454c to 2e38c0b Compare August 11, 2021 11:51

jrajahalme temporarily deployed to ci August 11, 2021 11:51 Inactive

jrajahalme added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 1, 2021

jrajahalme added 2 commits September 1, 2021 13:36

test: Move pause from test to action

aa74cd0

Move the pause on fail wait from test to action, so that nothing happens after the failed action. Signed-off-by: Jarno Rajahalme <[email protected]>

tklauser force-pushed the pr/jrajahalme/test-failure-output-improvements branch from 2e38c0b to d9c6807 Compare September 1, 2021 11:36

tklauser temporarily deployed to ci September 1, 2021 11:36 Inactive

tklauser merged commit 4fdacae into master Sep 1, 2021

tklauser deleted the pr/jrajahalme/test-failure-output-improvements branch September 1, 2021 12:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test failure output improvements #316

Test failure output improvements #316

jrajahalme commented Jun 14, 2021 •

edited

Loading

ti-mo Jun 15, 2021

michi-covalent Jun 15, 2021

jrajahalme Jun 16, 2021

jrajahalme Jun 16, 2021

jrajahalme Jun 16, 2021

jrajahalme Jun 18, 2021

ti-mo Jun 15, 2021

jrajahalme Jun 16, 2021

ti-mo Jun 16, 2021 •

edited

Loading

jrajahalme Jun 16, 2021 •

edited

Loading

ti-mo left a comment

ti-mo commented Jul 13, 2021

jrajahalme commented Jul 20, 2021

ti-mo left a comment

jrajahalme commented Aug 11, 2021

Test failure output improvements #316

Test failure output improvements #316

Conversation

jrajahalme commented Jun 14, 2021 • edited 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-mo Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

jrajahalme Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

ti-mo left a comment

Choose a reason for hiding this comment

ti-mo commented Jul 13, 2021

jrajahalme commented Jul 20, 2021

ti-mo left a comment

Choose a reason for hiding this comment

jrajahalme commented Aug 11, 2021

jrajahalme commented Jun 14, 2021 •

edited

Loading

ti-mo Jun 16, 2021 •

edited

Loading

jrajahalme Jun 16, 2021 •

edited

Loading