Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

many: add logger.MockLogger() and use it in the tests #3911

Merged
merged 3 commits into from
Sep 13, 2017

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Sep 13, 2017

We have two noisy tests in the unittest runs right now. Messages
get logged to stdout which really should not be visible. With the use
of the new logger.MockLogger() those are now no longer visible.

I also tweaked the existing pattern of:

var logbuf bytes.Buffer
l, err := logger.New(&logbuf, logger.DefaultFlags)
c.Assert(err, IsNil)
logger.SetLogger(l)

to just use:

logbuf, restore := logger.MockLogger()
defer restore()

Please let me know what you think, if you think this simple setup should not have a MockLogger() helper I'm happy to adjust the PR to just switch to a different logger in the two noisy tests we currently have.

We have some noise in the unittest runs right now because messages
get logged to stdout which really should not be visible. With the
new logger.MockLogger() they are now no longer visible.
@codecov-io
Copy link

codecov-io commented Sep 13, 2017

Codecov Report

Merging #3911 into master will increase coverage by 0.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3911      +/-   ##
==========================================
+ Coverage    75.9%   75.91%   +0.01%     
==========================================
  Files         416      416              
  Lines       35986    35994       +8     
==========================================
+ Hits        27316    27326      +10     
+ Misses       6752     6751       -1     
+ Partials     1918     1917       -1
Impacted Files Coverage Δ
testutil/dbustest.go 0% <0%> (ø) ⬆️
logger/logger.go 92.45% <80%> (-2.9%) ⬇️
asserts/asserts.go 91.22% <0%> (+0.04%) ⬆️
overlord/snapstate/snapmgr.go 81.7% <0%> (+1.13%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ecbeea9...9cf23db. Read the comment docs.

Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks reasonable

logger/logger.go Outdated
}
SetLogger(l)
return buf, func() {
logger = oldLogger
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not needed

logger/logger.go Outdated
// MockLogger replaces the exiting logger with a buffer and returns
// the log buffer and a restore function.
func MockLogger() (buf *bytes.Buffer, restore func()) {
buf = bytes.NewBuffer(nil)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is just: &bytes.Buffer{}

@pedronis pedronis merged commit ee9452f into canonical:master Sep 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants