testing: Improve logging from TestAgent #7546
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously the log output included the test name twice and a long date format. The test output is already grouped by test, so adding the test name did not add any new information. The date and time are only useful to understand elapsed time, so using a short format should provide succident detail. Also fixed a bug in NewTestAgentWithFields where nil was returned instead of the test agent.
dnephin
added
the
theme/internal-cleanup
dnephin
commented
Mar 30, 2020
| // default value. Tests which set a custom name for multiple agents will | ||
| // use the provided name. | ||
| // TODO: remove TestAgent.Name and accept a name arg on Start, to remove | ||
| // this workaround. |
There was a problem hiding this comment.
I can make this change after the PR merges. It should be easy to automate, but will change ~800 lines. I did not want all of those changes distracting from the logging changes in this PR.
mkeeler
reviewed
Mar 30, 2020
dnephin
force-pushed
the
dnephin/logging-in-tests
branch
from
47e2ec5
to
1ea8456
Compare
And re-add the name to the logger so that log messages from different agents in a single can be identified.
dnephin
force-pushed
the
dnephin/logging-in-tests
branch
from
1ea8456
to
ad7c78f
Compare
This was referenced Mar 31, 2020
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I find the log output in tests somewhat difficult to work with.
As an example, here is a log line before this change:
There are 112 characters before the relevant log line text starts. The name is duplicated, and is unnecessary because
go testalready aggregates the output by test. The full date/time is most likely not relevant, either CI will give us the time, or for a local run we know the time. The time is really only relevant to understand the elapsed time between log lines.writer.go:29is the same on every log line.After this change:
The time is still there, so we can see elapsed time between statements. For the few tests which run multiple agents
TestAgentwill be replaced with the name provided by the test, so we can tell which agent produced the log message.