Skip to content

Require all Settings are tested #11582

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

sheidkamp
Copy link
Contributor

@sheidkamp sheidkamp commented Jul 3, 2025

Description

Resolves #11556

Update the settings test to ensure that all settings are tested by moving the ENV out of the test case and into a separate data structure that can be validated against the Settings struct.

When a new field is added to the Settings struct, the "all settings are tested" test will fail unless an ENV var with a non-default is added to the tests.

Also ordered fields in the test to mirror the order in the Settings struct and removed the KGW_STS_CLUSTER_NAME and KGW_STS_URI env vars which do not correspond to any Settings fields

Some code has been borrowed and modified from the kelseyhightower/envconfig package to determine what ENV vars should be set for the test.

Change Type

/kind cleanup

Changelog

NONE

Additional Notes

@Copilot Copilot AI review requested due to automatic review settings July 3, 2025 13:21
@github-actions github-actions bot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note-none labels Jul 3, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds a safeguard to ensure every field in the Settings struct is covered by a corresponding environment‐variable test by extracting the hardcoded map into allEnvVarsSet() and comparing it against a reflection‐derived list.

  • Extracted the full ENV map into allEnvVarsSet() for reuse
  • Added expectedEnvVars() to reflectively generate expected env-var names and defaults
  • Introduced two new subtests to verify (1) every setting is tested, and (2) expectedEnvVars() behaves as intended

Reviewed Changes

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

File Description
pkg/settings/settings_test.go Moved hardcoded env var map into allEnvVarsSet(), added expectedEnvVars() helper
pkg/settings/settings_test.go Added “all settings are tested” and “expectedEnvVars” subtests
Comments suppressed due to low confidence (2)

pkg/settings/settings_test.go:214

  • [nitpick] The subtest name expectedEnvVars matches the helper function name and could be confused. Consider renaming the subtest to validateExpectedEnvVars or similar.
	t.Run("expectedEnvVars", func(t *testing.T) {

pkg/settings/settings_test.go:24

  • The STS-related fields (KGW_STS_CLUSTER_NAME and KGW_STS_URI) were removed from allEnvVarsSet(), but if Settings still defines these fields, the new all settings are tested subtest will fail. Add entries for those env vars or remove the fields from Settings.
		"KGW_ISTIO_NAMESPACE":               "my-istio-namespace",

sheidkamp and others added 5 commits July 3, 2025 09:35
Co-authored-by: Copilot <[email protected]>
Signed-off-by: Seth Heidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Signed-off-by: sheidkamp <[email protected]>
Copy link
Member

@timflannagan timflannagan left a comment

Choose a reason for hiding this comment

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

Seems reasonable at a high-level - a couple of comments on the approach.

@@ -11,6 +15,32 @@ import (
"github.com/kgateway-dev/kgateway/v2/pkg/settings"
)

func allEnvVarsSet() map[string]string {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think maintaining a canonical map is the right approach.

Comment on lines +242 to +247
var gatherRegexp = regexp.MustCompile("([^A-Z]+|[A-Z]+[^A-Z]+|[A-Z]+)")
var acronymRegexp = regexp.MustCompile("([A-Z]+)([A-Z][^A-Z]+)")

// expectedEnvVars returns a map of all the env vars that should be set for the given settings value.
// The value of the map is the default value of the field.
func expectedEnvVars(settingsValue reflect.Value) map[string]interface{} {
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be simplified quite a bit. Using reflection still makes sense to me. Sketching this out inline:

	want := map[string]struct{}{}
	st := reflect.TypeOf(settings.Settings{})
	for i := 0; i < st.NumField(); i++ {
		f := st.Field(i)
		want["KGW_"+camelToSnake(f.Name)] = struct{}{}
	}
        ...

And we define a helper function to do the camel case conversion:

func camelToSnake(s string) string {
	var b strings.Builder
	for i, r := range s {
		if unicode.IsUpper(r) && i > 0 {
			b.WriteByte('_')
		}
		b.WriteRune(unicode.ToUpper(r))
	}
	return b.String()
}

Not sure if strings.Builder is correct here, but should do the job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started down a similar path, but the reason I went the complicated route was to match the logic used by envconfig.

Your suggestion works with all the current fields, but doesn't work if a field is created with an "alt" name or uses an acronym, or doesn't use "split_words:true". (If we want to standardize that you can't use an "alt" and "split_words" must be true, we could validate/enforce that as well)

Also, by parsing out the default value we can check that the test is configured properly by validating the ENV var value used in the test is not the default. WDYT?

@@ -184,6 +194,40 @@ func TestSettings(t *testing.T) {
require.Emptyf(t, diff, "Settings do not match expected values (-expected +got):\n%s", diff)
})
}

t.Run("all settings are tested", func(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we move this into it's own standalone test focused on flagging settings w/o test coverage, duplicate settings, or handle the scenario where settings that live in the allEnvVarsSet have been removed.

func TestEnvVarCoverage(t *testing.T) {
	want := map[string]struct{}{}
	st := reflect.TypeOf(settings.Settings{})
	for i := 0; i < st.NumField(); i++ {
		f := st.Field(i)
		want["KGW_"+camelToSnake(f.Name)] = struct{}{}
	}

	have := allValuesSetEnvVars()
	var missing, extra []string
	for k := range want {
		if _, ok := have[k]; !ok {
			missing = append(missing, k)
		}
	}
	for k := range have {
		if _, ok := want[k]; !ok {
			extra = append(extra, k)
		}
	}
	if len(missing) > 0 || len(extra) > 0 {
		t.Fatalf("env var coverage out of date:\nmissing: %v\nextra: %v", missing, extra)
	}
}

And the settings_test.go is now responsible for checking the defaulting logic, providing negative test cases, etc. WYDT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I will setup a separate test (contents pending response to the other comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test gap: Adding new Settings fields doesn't break tests when fields are missing from expectations"
2 participants