test: increase unit test coverage #23
Conversation
api/types_test.go
Outdated
| ) | ||
|
|
||
| var ( | ||
| OIDCNotEnabled = false |
There was a problem hiding this comment.
Can we move the package-level variables to the method they are used?
This will avoid polluting the package namespace.
In general variables should be declared with the minimum scope needed.
api/types_test.go
Outdated
| var ( | ||
| OIDCNotEnabled = false | ||
| OIDCIsEnabled = true | ||
| emptyFIlename = "" |
| OIDCConfigFilename = "./secrets/oidc-config.yaml.sample" | ||
| ) | ||
|
|
||
| func TestOIDCConfig_Validate(t *testing.T) { |
There was a problem hiding this comment.
As mentioned in https://github.com/bf2fc6cc711aee1a0c2a/observability-remote-write-proxy/pull/12#discussion_r1114172741 Validate should be reserved to validation and reading of the file should be decoupled from it to make it more testable
pkg/remotewrite/remotewrite_test.go
Outdated
| ) | ||
|
|
||
| var ( | ||
| rw = &prometheus.WriteRequest{ |
There was a problem hiding this comment.
pkg/remotewrite/validate_test.go
Outdated
| @@ -58,6 +59,13 @@ var ( | |||
| req = &prometheus.WriteRequest{ | |||
| Timeseries: []*prometheus.TimeSeries{}, | |||
| } | |||
| metadataReq = &prometheus.WriteRequest{ | |||
There was a problem hiding this comment.
Same comment as https://github.com/bf2fc6cc711aee1a0c2a/observability-remote-write-proxy/pull/23/files#r1132325720 with all the package global variables in this file
| }, | ||
| want: true, | ||
| }, | ||
| } |
There was a problem hiding this comment.
We should also test that if a remote write request has both timeseries data and metadata data then the request is not a metadata request
valeriiashapoval
changed the title
valeriiashapoval
force-pushed
the
add-unit-tests
branch
from
7048eac to
71b79f1
Compare
|
thanks @miguelsorianod @pb82 ! forgot to add replies - the comments have been addressed in the latest commits. |
| ) | ||
|
|
||
| func TestOIDCConfig_Validate(t *testing.T) { | ||
| OIDCNotEnabled := false |
There was a problem hiding this comment.
Instead of creating the OIDCNotEnabled, OIDCIsEnabled the boolean values can just be set directly in the fields struct directly changing the Enabled attribute to be a bool instead of *bool.
Then in the loop you can just pass the address of it, or create a copy in the loop itself and then passing the address to the method call in the loop
| OIDCNotEnabled := false | ||
| OIDCIsEnabled := true | ||
| emptyFilename := "" | ||
| filenameThatDoesntExist := "doesnt-exist.yaml" |
There was a problem hiding this comment.
A better alternative that doesn't require setting up the files all at the beginning is to add have an attribute in the fields struct that is of type func() string that returns the file name. In that function you would add the logic to create the file and such and then return the name of the filename.
Then on the loop you would call that function and from the returned result you would also perform a deferred call to os.Remove using that returned file name to perform the cleanup
There was a problem hiding this comment.
Also remember my #23 (comment) comment. The reason this test is this complicated is because the file reading is coupled to the validation. This should be improved, but that can be dealt with in another PR.
| ) | ||
|
|
||
| var ( | ||
| a byte = 116 |
There was a problem hiding this comment.
As mentioned in other comments we should avoid the use of global variables. Can we moved them where needed?
| wantErr bool | ||
| }{ | ||
| { | ||
| name: "should return an error", |
There was a problem hiding this comment.
Why should creating a new default token verifier return an error?
| want bool | ||
| }{ | ||
| { | ||
| name: "should return true", |
There was a problem hiding this comment.
The description should be more descriptive. For example: When the token verifier enabled option is true the token verifier is enabled
| want: true, | ||
| }, | ||
| { | ||
| name: "should return false", |
There was a problem hiding this comment.
| want string | ||
| }{ | ||
| { | ||
| name: "should return token", |
There was a problem hiding this comment.
See https://github.com/bf2fc6cc711aee1a0c2a/observability-remote-write-proxy/pull/23/files#r1165345194. Some of the test names should be more descriptive. Instead of telling what it returns it should explain in the form "when/if this happens this is the outcome" or similarly
| wantErr bool | ||
| }{ | ||
| { | ||
| name: "should return error", |
There was a problem hiding this comment.
| wantErr bool | ||
| }{ | ||
| { | ||
| name: "should return error", |
There was a problem hiding this comment.
This only tests an error. We should have a case for successful validation too
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "should return error", |
There was a problem hiding this comment.
| } | ||
|
|
||
| func TestIsMetadataRequest(t *testing.T) { | ||
| metadataReq := &prometheus.WriteRequest{ |
There was a problem hiding this comment.
Is there some reason the variables have been created first? Can we do the same but in each args value?
Those tests cover 75.2% of the proxy: api package (65.5%) and pkg (78.9%).