Description
Problem
The current TestSettings
in pkg/settings/settings_test.go
has a testing reliability gap: when new fields are added to the Settings
struct, the test doesn't automatically fail if those fields aren't included in the test's expectedSettings
.
This was highlighted when adding the WeightedRoutePrecedence
and IngressUseWaypoints
fields to the Settings struct. The test would have passed even if these fields were omitted from the test expectations, because Go's struct comparison with cmp.Diff()
only compares fields that are explicitly set in the expected struct.
Current Behavior
// TODO: this test case does not fail when a new field is added to Settings
// but not updated here. should it?
name: "defaults to empty or default values",
expectedSettings: &settings.Settings{
DnsLookupFamily: settings.DnsLookupFamilyV4Preferred,
EnableIstioIntegration: false,
// ... other fields
// If a new field is added to Settings but forgotten here,
// the test will still pass silently
},
Impact
- Silent test failures: New settings fields might not be properly tested for their default values
- Incomplete test coverage: Developers might unknowingly introduce untested configuration paths
- Maintenance burden: Manual vigilance required to ensure all fields are tested
Related
- Original TODO comment in
pkg/settings/settings_test.go:31
- Pull request where this gap was identified: test(settings): improve error handling and assertions in TestSettings #11518
Metadata
Metadata
Assignees
Labels
No labels