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 config #71

Merged
merged 3 commits into from
Oct 7, 2024
Merged

Test config #71

merged 3 commits into from
Oct 7, 2024

Conversation

lippserd
Copy link
Member

@lippserd lippserd commented Sep 18, 2024

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()

  1. 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.
  2. 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.

@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Sep 18, 2024
@lippserd lippserd requested a review from oxzi September 18, 2024 11:13
config/config_test.go Outdated Show resolved Hide resolved
config/config_test.go Outdated Show resolved Hide resolved
config/config_test.go Show resolved Hide resolved
config/config_test.go Outdated Show resolved Hide resolved
config/config_test.go Show resolved Hide resolved
config/config_test.go Show resolved Hide resolved
config/config_test.go Show resolved Hide resolved
err := ParseFlags(nil)
require.ErrorIs(t, err, ErrInvalidArgument)
})
}
Copy link
Member

Choose a reason for hiding this comment

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

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().

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

A simpler approach would have been to refactor ParseFlags(v any) to parseFlags(v any, exitOnHelp bool) and let ParseFlags call parseFlags(v, true), allowing to test parseFlags(…, false). But I really appreciate the effort.

config/tls_test.go Outdated Show resolved Hide resolved
config/tls_test.go Outdated Show resolved Hide resolved
@lippserd lippserd force-pushed the test-config branch 2 times, most recently from d7eae21 to b93c892 Compare September 27, 2024 19:31
@lippserd lippserd requested a review from oxzi September 30, 2024 07:12
@lippserd
Copy link
Member Author

@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.

@lippserd lippserd merged commit bb5b374 into main Oct 7, 2024
15 checks passed
@lippserd lippserd deleted the test-config branch October 7, 2024 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla/signed CLA is signed by all contributors of a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants