Skip to content
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

test: increase unit test coverage #23

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 143 additions & 0 deletions api/types_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
package api

import (
"os"
"testing"
)

func TestOIDCConfig_Validate(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

OIDCNotEnabled := false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

OIDCIsEnabled := true
emptyFilename := ""
filenameThatDoesntExist := "doesnt-exist.yaml"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fullConfigFile, _ := os.CreateTemp("", "")
defer func(name string) {
err := os.Remove(name)
if err != nil {
t.Fatal(err)
}
}(fullConfigFile.Name())
filenameComplete := fullConfigFile.Name()
yamlDataComplete := `
issuer_url: https://example.com
client_id: client-id
client_secret: client-secret
`
_, _ = fullConfigFile.Write([]byte(yamlDataComplete))
err := fullConfigFile.Close()
if err != nil {
return
}
emptyConfigFile, _ := os.CreateTemp("", "")
defer func(name string) {
err := os.Remove(name)
if err != nil {
t.Fatal(err)
}
}(emptyConfigFile.Name())
filenameNotComplete := emptyConfigFile.Name()
yamlDataNotComplete := ``
_, _ = emptyConfigFile.Write([]byte(yamlDataNotComplete))
err = emptyConfigFile.Close()
if err != nil {
return
}
type fields struct {
Enabled *bool
Filename *string
Attributes OIDCAttributes
}
tests := []struct {
name string
fields fields
wantErr bool
}{
{
name: "should return nil if oidc is not enabled",
fields: fields{
Enabled: &OIDCNotEnabled,
},
wantErr: false,
},
{
name: "should return error if oidc is enabled but no filename is provided",
fields: fields{
Enabled: &OIDCIsEnabled,
Filename: &emptyFilename,
},
wantErr: true,
},
{
name: "should return err if oidc is enabled and filename is provided, but file does not exist",
fields: fields{
Enabled: &OIDCIsEnabled,
Filename: &filenameThatDoesntExist,
},
wantErr: true,
},
{
name: "should return an error if oidc is enabled but the client id is not provided",
fields: fields{
Enabled: &OIDCIsEnabled,
Filename: &filenameNotComplete,
Attributes: OIDCAttributes{
IssuerURL: "https://test.com",
ClientID: "",
},
},
wantErr: true,
},
{
name: "should return an error if oidc is enabled but the client secret is not provided",
fields: fields{
Enabled: &OIDCIsEnabled,
Filename: &filenameNotComplete,
Attributes: OIDCAttributes{
IssuerURL: "https://test.com",
ClientID: "test",
ClientSecret: "",
},
},
wantErr: true,
},
{
name: "should return nil if oidc is enabled and all required fields are provided",
fields: fields{
Enabled: &OIDCIsEnabled,
Filename: &filenameComplete,
Attributes: OIDCAttributes{
IssuerURL: "https://test.com",
ClientID: "test",
ClientSecret: "test",
},
},
wantErr: false,
},
{
name: "should return nil if oidc is enabled and all required fields are provided along with optional fields",
fields: fields{
Enabled: &OIDCIsEnabled,
Filename: &filenameComplete,
Attributes: OIDCAttributes{
IssuerURL: "https://test.com",
ClientID: "test",
ClientSecret: "test",
Audience: "test",
},
},
wantErr: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
c := &OIDCConfig{
Enabled: tt.fields.Enabled,
Filename: tt.fields.Filename,
Attributes: tt.fields.Attributes,
}
if err := c.Validate(); (err != nil) != tt.wantErr {
t.Errorf("OIDCConfig.Validate() error = %v, wantErr %v", err, tt.wantErr)
}
})
}
}
Loading