Test `config` #71

lippserd · 2024-09-18T09:49:28Z

This PR introduces tests for the ParseFlags(), FromYAMLFile() and TLS.MakeConfig() functions:

ParseFlags

Verifies that command-line flags are correctly parsed into a struct.
Error propagation from defaults.Set().
Nil pointer argument: Tests calling FromYAMLFile with a nil pointer argument.
Nil argument: Tests calling FromYAMLFile with a nil argument.

FromYAMLFile

Simple YAML: Tests parsing a simple YAML file.
Inlined YAML: Tests parsing a YAML file with inlined fields.
Embedded YAML: Tests parsing a YAML file with embedded fields.
Defaults: Tests parsing a YAML file with default values.
Overriding defaults: Tests parsing a YAML file overriding default values.
Empty YAML: Tests parsing an empty YAML file.
Empty YAML with directive separator: Tests parsing an empty YAML file with a directive separator.
Faulty YAML: Tests parsing a faulty YAML file.
Invalid YAML: Tests parsing a YAML file that is expected to fail validation.
Nil pointer argument: Tests calling FromYAMLFile with a nil pointer argument.
Nil argument: Tests calling FromYAMLFile with a nil argument.
Non-existent file: Tests calling FromYAMLFile with a non-existent file.
Permission denied: Tests calling FromYAMLFile with a file that has no read permissions.

TLS.MakeConfig()

Basic Configuration
- Verifies that the function returns nil when TLS is disabled.
- Ensures the server name is correctly set in the TLS configuration.
- Checks that InsecureSkipVerify is set to true when specified.
- Ensures an error is returned when the client certificate is missing.
- Ensures an error is returned when the private key is missing.
x509 Certificate Handling
- Valid certificate and key: Verifies successful configuration with valid certificate and key.
- Mismatched certificate and key: Ensures an error is returned for mismatched certificate and key.
- Invalid certificate path: Ensures an error is returned for a non-existent certificate file.
- Invalid certificate permissions: Ensures an error is returned for a certificate file with no read permissions.
- Corrupt certificate: Ensures an error is returned for a corrupt certificate file.
- Invalid key path: Ensures an error is returned for a non-existent key file.
- Invalid key permissions: Ensures an error is returned for a key file with no read permissions.
- Corrupt key: Ensures an error is returned for a corrupt key file.
- Valid CA: Verifies successful configuration with a valid CA certificate.
- Invalid CA path: Ensures an error is returned for a non-existent CA file.
- Invalid CA permissions: Ensures an error is returned for a CA file with no read permissions.
- Corrupt CA: Ensures an error is returned for a corrupt CA file.

oxzi · 2024-09-27T09:22:35Z

config/config_test.go

+	t.Run("Nil pointer argument", func(t *testing.T) {
+		var flags interface{}
+
+		err := ParseFlags(flags)
+		require.ErrorIs(t, err, ErrInvalidArgument)
+	})
+
+	t.Run("Nil argument", func(t *testing.T) {
+		err := ParseFlags(nil)
+		require.ErrorIs(t, err, ErrInvalidArgument)
+	})
+}


These two test cases seem to be identical. Furthermore, the first one does not check a nil pointer, but nil.

If, however, one changes it to use a real nil pointer by setting ParseFlags(&flags), it panics.

Yep, I forgot to add *, i.e. var flags *any.

oxzi · 2024-09-27T09:25:59Z

config/config_test.go

+	})
+}
+
+// simpleConfig is an always valid test configuration struct with only one key.


This is just my personal taste, but I would expect the used types to be on the top of the file. When reading the tests top to bottom, I first asked myself where the used types are being declared.

I just always put the exported stuff first. But you're right that it's better for tests to have them first.

oxzi · 2024-09-27T09:29:59Z

config/config_test.go

+			defer func(name string) {
+				_ = os.Remove(name)
+			}(yamlFile.Name())


Is there a reason to introduce the name variable for the deferred function? yamlFile.Name() is also used on all other places.

Suggested change

defer func(name string) {

_ = os.Remove(name)

}(yamlFile.Name())

defer func() { _ = os.Remove(yamlFile.Name()) }()

In such scenarios, I would always capture the argument values at the time the defer is set up, as opposed to relying on the value not having changed by the time the defer is executed.

oxzi · 2024-09-27T09:35:20Z

config/config_test.go

+	t.Run("Nil pointer argument", func(t *testing.T) {
+		var config Validator
+
+		err := FromYAMLFile("", config)
+		require.ErrorIs(t, err, ErrInvalidArgument)
+	})
+
+	t.Run("Nil argument", func(t *testing.T) {
+		err := FromYAMLFile("", nil)
+		require.ErrorIs(t, err, ErrInvalidArgument)
+	})


Again, both these tests boil down to perform the same FromYAMLFile call.

True, and it's a bit more difficult here. But I think something like var config *struct{ Validator } could work.

oxzi · 2024-09-27T09:36:45Z

config/config_test.go

+		var config struct{ validateValid }
+		var pathError *fs.PathError
+
+		err := FromYAMLFile("nonexistent.yaml", &config)


One should enforce that nonexistent.yaml does not suddenly exists. Maybe create another new temporary file, delete it afterwards and use its file path? I mean, technically this could be raced, but it should be fine for tests.

I will add require.ErrorIs(t, pathError.Err, fs.ErrNotExist). If nonexistent.yaml really gets created, the test fails.

oxzi · 2024-09-27T09:41:04Z

config/config_test.go

+		defer func(name string) {
+			_ = os.Remove(name)
+		}(yamlFile.Name())


Again, why are you wrapping the file name?

Btw, I have verified that the os.Remove works. First, I was a bit uncertain due to the 000 permissions.

Btw, I have verified that the os.Remove works. First, I was a bit uncertain due to the 000 permissions.

Yes, the ability to delete a file is determined by the permissions of the directory containing the file.

oxzi · 2024-09-27T09:52:14Z

config/config_test.go

+				Key: "value",
+			},
+			invalid: true,
+		},


A test case for failed defaults is missing. Maybe create some test type with faulty default struct types.

icinga-go-library/config/config.go

Lines 37 to 38 in d8bd22a

if err := defaults.Set(v); err != nil {

return errors.Wrap(err, "can't set config defaults")

Sounds good.

oxzi · 2024-09-27T09:54:25Z

config/config_test.go

+		err := ParseFlags(nil)
+		require.ErrorIs(t, err, ErrInvalidArgument)
+	})
+}


There are no tests for a parser.Parse() error. However, this may require some code changes as otherwise one has to recover from os.Exit().

icinga-go-library/config/config.go

Lines 70 to 78 in d8bd22a

if _, err := parser.Parse(); err != nil {

var flagErr *flags.Error

if errors.As(err, &flagErr) && flagErr.Type == flags.ErrHelp {

fmt.Fprintln(os.Stdout, flagErr)

os.Exit(0)

}

return errors.Wrap(err, "can't parse CLI flags")

}

However, covering this is not completely necessary, imo.

Recover from os.Exit() is impossible using the stdlib, at least to my knowledge. But we could test that using a subprocess. I'll give it a try.

oxzi · 2024-09-27T10:49:15Z

config/tls_test.go

+	var issuerKey crypto.PrivateKey
+	if options.issuer != nil {
+		if options.issuerKey == nil {
+			panic("issuerKey required if issuer set")


This helper function returns an error in most cases, but panics here. When generateCert is called, the error must always be empty. Thus, to have a nicer test output, maybe return an error here as well.

Yeah, why not.

oxzi · 2024-09-27T10:50:57Z

config/tls_test.go

+		issuerKey = privateKey
+	}
+
+	derBytes, err := x509.CreateCertificate(rand.Reader, template, issuer, privateKey.Public(), issuerKey)


Well, x509.CreateCertificate() returns the DER encoded certificate as bytes -> derBytes 😆.

I would rename it to der as x509.ParseCertificate() uses that as argument name.

lippserd · 2024-09-30T07:13:34Z

@oxzi I have adjusted the code based on your preview. Please take another look at it. I have already moved code unfortunately, so the comments are no longer aligned correctly.

cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Sep 18, 2024

lippserd requested a review from oxzi September 18, 2024 11:13

lippserd mentioned this pull request Sep 18, 2024

Introduce config.FromEnv() #41

Open

lippserd force-pushed the test-config branch from f232daa to 2d3bad2 Compare September 20, 2024 07:07

oxzi reviewed Sep 27, 2024

View reviewed changes

lippserd force-pushed the test-config branch from 2d3bad2 to d7eae21 Compare September 27, 2024 15:49

lippserd added 3 commits September 27, 2024 21:31

Test config.ParseFlags()

ad3b00d

Test config.FromYAMLFile()

a70036d

Test config#TLS.MakeConfig()

b93c892

lippserd force-pushed the test-config branch from d7eae21 to b93c892 Compare September 27, 2024 19:31

lippserd requested a review from oxzi September 30, 2024 07:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test `config` #71

Test `config` #71

lippserd commented Sep 18, 2024 •

edited

Loading

oxzi Sep 27, 2024

lippserd Sep 27, 2024

oxzi Sep 27, 2024

lippserd Sep 27, 2024

oxzi Sep 27, 2024

lippserd Sep 27, 2024

oxzi Sep 27, 2024

lippserd Sep 27, 2024

oxzi Sep 27, 2024

lippserd Sep 27, 2024

oxzi Sep 27, 2024

lippserd Sep 27, 2024

oxzi Sep 27, 2024

lippserd Sep 27, 2024

oxzi Sep 27, 2024

lippserd Sep 27, 2024

oxzi Sep 27, 2024

lippserd Sep 27, 2024

oxzi Sep 27, 2024

lippserd Sep 27, 2024

lippserd Sep 27, 2024 •

edited

Loading

lippserd commented Sep 30, 2024

	if err := defaults.Set(v); err != nil {
	return errors.Wrap(err, "can't set config defaults")

	if _, err := parser.Parse(); err != nil {
	var flagErr *flags.Error
	if errors.As(err, &flagErr) && flagErr.Type == flags.ErrHelp {
	fmt.Fprintln(os.Stdout, flagErr)
	os.Exit(0)
	}

	return errors.Wrap(err, "can't parse CLI flags")
	}

Test config #71

Are you sure you want to change the base?

Test config #71

Conversation

lippserd commented Sep 18, 2024 • edited Loading

ParseFlags

FromYAMLFile

TLS.MakeConfig()

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lippserd Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

lippserd commented Sep 30, 2024

Test `config` #71

Test `config` #71

lippserd commented Sep 18, 2024 •

edited

Loading

lippserd Sep 27, 2024 •

edited

Loading