creating tests for files in the cmd directory #433
Conversation
Signed-off-by: Philip-21 <[email protected]>
|
|
|
Hi , I am Philip from Nigeria, its My first time contributing to Pomeruim, i decided to start with creating unit tests in the cli codebase to help understand and get along with pomerium technologies and its codebases, I would like to contribute more and tackle issues as they arise . |
|
i will be adding more unit tests soon before this pr can be merged |
There was a problem hiding this comment.
Hi @Philip-21, we appreciate your willingness to contribute. Unfortunately I don't think we can merge this PR without some substantial revisions.
cmd/pomerium-cli/cache_test.go
Outdated
| mockCache := new(MockCache) | ||
|
|
||
| //set up dir expectation for ExecCredentialsPath | ||
| mockCache.On(" ExecCredentialsPath").Return(filePath+"/tmp/cache", nil) |
There was a problem hiding this comment.
You've correctly identified that ExecCredentialsPath() makes this behavior difficult to test.
However I don't think this mock object is effective at swapping out a different path. There are some early returns below, but if I change those, I see that clearAllCachedCredentials() uses the real ExecCredentialsPath() rather than this mock path.
cmd/pomerium-cli/cache_test.go
Outdated
| if err != nil { | ||
| t.Log(err) | ||
| return | ||
| } |
There was a problem hiding this comment.
By ignoring this error, the test can pass without ever exercising the behavior of clearAllCachedCredentials(). In general it would probably be better to do something like require.NoError(t, err) instead (adding an import for "github.com/stretchr/testify/assert").
When running this test I found that file1.json could not be created because the tmp and cache directories did not exist.
There was a problem hiding this comment.
i decided to use a tempdir so we wouldnt have unwanted test files generated.
If you want the files to be created i can implement that
cmd/pomerium-cli/cache_test.go
Outdated
|
|
||
| func TestCachedCredentialPath(t *testing.T) { | ||
| //machine filepath name declared when running tests | ||
| pcPath := "/home/dell/.cache" |
There was a problem hiding this comment.
This test has the same problem as above: the behavior under test depends on the return value of ExecCredentialsPath(). We would need to make sure that this test can pass when run by GitHub Actions and when run by a developer locally.
There was a problem hiding this comment.
thats true, the ci checks won't pass .
|
Hi @kenjenkins i will make adjustments where necessary, thanks for the reviews |
Signed-off-by: Philip-21 <[email protected]>
Summary
Related issues
The existing implementation here contains unit test codes for different files in the
cmddirectoryChecklist
improvement/bug/ etc)