add mock and os LookupEnv implementation + tests #2
Conversation
|
@adammck let me know if this does not correlate with the package intention ! |
os/os_test.go
Outdated
| assert.Equal(t, "GHI", val) | ||
| assert.True(t, ok) | ||
|
|
||
| val, ok = e.LookupEnv("nope") |
There was a problem hiding this comment.
This line will fail if a var named nope happens to be set. That seems unlikely, but could we change it to something like THIS-VAR-MUST-BE-UNSET and use os.Unsetenv to ensure that it has the expected (absent) value?
There was a problem hiding this comment.
all right good point, I've done both 👍
|
Thanks very much, looks like LookupEnv was added after this package was written! Patch looks good to merge with the suggested testing tweak. |
| @@ -28,6 +28,11 @@ func (e *MockEnv) Getenv(key string) string { | |||
| return e.data[key] | |||
| } | |||
|
|
|||
| func (e *MockEnv) LookupEnv(key string) (string, bool) { | |||
| val, ok := e.data[key] | |||
| return val, ok | |||
There was a problem hiding this comment.
Also, you can skip the variables here and return e.data[key] directly.
There was a problem hiding this comment.
hmm I tried it but I get a compilation error: not enough arguments to return. I think go does not recognize that I want to return the ok variable as well from map access, so I did this explicitly
There was a problem hiding this comment.
Huh, I thought that'd work. Thanks for checking.
|
@adammck thanks for the review, I've done your first suggestion, the second one did not work for me |
LookupEnvmethod