test: add TestLevelStringUnknown and TestParseLevelVariations in level_test.go - #1581
test: add TestLevelStringUnknown and TestParseLevelVariations in level_test.go#1581aoright wants to merge 1 commit into
Conversation
…l_test.go Signed-off-by: aoright <102943475+aoright@users.noreply.github.com>
thaJeztah
left a comment
There was a problem hiding this comment.
Thanks! I had a look and it looks like most lines are already covered by tests, except for these 3;
Lines 88 to 91 in 134c80f
Left suggestions to replace the existing tests with your table approach; let me know if you need more info.
When updating, please amend / squash your commit 😅
|
|
||
| func TestLevelStringUnknown(t *testing.T) { | ||
| unknownLevel := logrus.Level(99) | ||
| require.Equal(t, "unknown", unknownLevel.String()) | ||
| } | ||
|
|
||
| func TestParseLevelVariations(t *testing.T) { | ||
| tests := []struct { | ||
| input string | ||
| expected logrus.Level | ||
| hasErr bool | ||
| }{ | ||
| {"panic", logrus.PanicLevel, false}, | ||
| {"PANIC", logrus.PanicLevel, false}, | ||
| {"fatal", logrus.FatalLevel, false}, | ||
| {"error", logrus.ErrorLevel, false}, | ||
| {"warn", logrus.WarnLevel, false}, | ||
| {"WARN", logrus.WarnLevel, false}, | ||
| {"warning", logrus.WarnLevel, false}, |
There was a problem hiding this comment.
I like the table-test, but we should instead make this replace the existing test(s) (and move it to that file, because ParseLevel is defined in logrus.go; I actually wanted to update those to use a table-test, so perhaps you can update those so that we don't duplicate the exact tests?
Possibly the same test could cover text -> level and level text within the same table so that we don't end up with 2 table tests, but I haven't tried.
Lines 530 to 613 in 134c80f
Oh! One thing worth mentioning; that logrus_test.go uses dot-imports for logrus; that's not consistent with other tests-files we have, but changing it would cause too much code-churn, so I left that for now (and we probably should .. for now);
Lines 18 to 19 in 134c80f
| {"debug", logrus.DebugLevel, false}, | ||
| {"trace", logrus.TraceLevel, false}, | ||
| {"TRACE", logrus.TraceLevel, false}, |
There was a problem hiding this comment.
When updating, could you avoid implicit fields here?
{input: "debug", expected: logrus.DebugLevel, hasErr: false},| {"invalid", logrus.Level(0), true}, | ||
| } | ||
|
|
||
| for _, tt := range tests { |
There was a problem hiding this comment.
Minor nit; we mostly converged on using tc (for "test case") for these tables; can you update it to use the same for consistency?
for _, tc := range tests {
Summary
level_test.go, add unit testTestLevelStringUnknownto verifyLevel.String()for unknown level values.TestParseLevelVariationsto verify case-insensitive matching and alias handling (e.g.,warn/warning, uppercase, invalid input).