-
Notifications
You must be signed in to change notification settings - Fork 120
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
Included tests for internal/log/env #309
Conversation
- Included tests for internal/log/env - Included licenses for `env.go` and `config.go` Signed-off-by: nathannaveen <[email protected]>
internal/log/env_test.go
Outdated
} | ||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
got, _ := tt.e.MarshalText() // this function never returns an error |
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.
There should be a check that err is always nil
so the assertion is checked.
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.
Thanks, I forgot about that when writing the test! I have added the error check err != nil
.
internal/log/env_test.go
Outdated
t.Run(tt.name, func(t *testing.T) { | ||
got, _ := tt.e.MarshalText() // this function never returns an error | ||
|
||
if !reflect.DeepEqual(got, tt.want) { |
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.
bytes.Equal()
a better choice than DeepEqual
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.
I have changed it from reflect.DeepEqual()
to bytes.Equal()
.
internal/log/env_test.go
Outdated
}, | ||
} | ||
for _, tt := range tests { | ||
t.Setenv(string(tt.text), tt.value) |
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.
I don't think this call does anything and probably should be removed, along with tt.value
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.
I have removed the t.Setenv()
but I kept the tt.value
because I added a check test.e.String() != test.value
.
internal/log/env_test.go
Outdated
value: "dev", | ||
}, | ||
} | ||
for _, tt := range tests { |
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.
nit: test
is a better than tt
as tt
is very similar to t
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.
I have changed all the tt
's to test
.
Signed-off-by: nathannaveen <[email protected]>
* Included tests for internal/log/env - Included tests for internal/log/env - Included licenses for `env.go` and `config.go` Signed-off-by: nathannaveen <[email protected]> * Fixed based on code review Signed-off-by: nathannaveen <[email protected]> Signed-off-by: nathannaveen <[email protected]> Co-authored-by: Caleb Brown <[email protected]>
env.go
andconfig.go
Signed-off-by: nathannaveen [email protected]