-
Notifications
You must be signed in to change notification settings - Fork 669
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
NOISSUE - Logger Fatal method returns no value #1728
Conversation
Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com>
Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com>
05d1a2e
to
1d10302
Compare
cmd/auth/main.go
Outdated
} | ||
|
||
logger, err := logger.New(os.Stdout, cfg.LogLevel) | ||
if err != nil { | ||
log.Fatalf(err.Error()) | ||
log.Fatal(fmt.Sprintf("failed to init logger: %s", err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this logger
or log
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a native Go log
because logger
was not initialized properly. But let me also rename it because we have:
logger, err := logger.New(os.Stdout, cfg.LogLevel)
Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com>
Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com>
Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
logger/logger_test.go
Outdated
res, err := writer.Read() | ||
require.Nil(t, err, "required successful buffer read") | ||
assert.Equal(t, e.ExitCode(), 1, fmt.Sprintf("%s: expected exit code %d, got %d", tc.desc, 1, e.ExitCode())) | ||
assert.Equal(t, res, tc.output, fmt.Sprintf("%s: expected output %s got %s", tc.desc, tc.output, res)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you assert, do you put first what you expect and then what you got as the params? As in these two lines this seems to be inverse and is a little bit confusing - because it is not consistent.
I will approve, but please change if you see fit.
Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #1728 +/- ##
=======================================
Coverage 70.35% 70.36%
=======================================
Files 146 146
Lines 11399 11398 -1
=======================================
Hits 8020 8020
+ Misses 2729 2728 -1
Partials 650 650
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What does this do?
This pull request makes Mainflux Logger Fatal method return no value.
Which issue(s) does this PR fix/relate to?
There is no such issue.
List any changes that modify/break current functionality
There are no changes in current functionalities.
Have you included tests for your changes?
Yes, tests for Fatal are added and other tests are refactored.
Did you document any new/modified functionality?
N/A
Notes
N/A