Skip to content

Commit 6ea9fd3

Browse files
r2k1r2k1
committed
add compatibility tests and fix linter issues
- Add backward compatibility test: verifies old code can unmarshal current JSON - Add forward compatibility test: verifies current code can unmarshal old JSON - Extract setupWorktree helper to reduce duplication - Rename backward_compat_test.go to compatibility_test.go - Fix linter issues: - Add missing exhaustive switch cases - Convert if-else chain to switch statement - Remove unused depth parameter - Add nolint directives for safe integer conversions - Ignore cleanup error return value fix: run compatibility tests sequentially and use separate env vars - Remove t.Parallel() to avoid git lock conflicts in CI - Use BACKWARD_COMPAT_COMMIT and FORWARD_COMPAT_COMMIT env vars - Add git fetch to handle shallow clones in CI panic on unsupported field types in testutil Instead of silently returning empty values, panic with descriptive error messages when encountering: - Unknown field kinds in getDefaultValueForField - Invalid map key types in getDefaultKeyForKind - Message/group fields in getDefaultValueForField (should be handled in setFieldValue) This makes bugs more obvious during test development.
1 parent 878718c commit 6ea9fd3

File tree

6 files changed

+229
-184
lines changed

6 files changed

+229
-184
lines changed

‎aks-node-controller/pkg/nodeconfigutils/backward_compat_test.go‎

Lines changed: 0 additions & 88 deletions
This file was deleted.

‎aks-node-controller/pkg/nodeconfigutils/compatibility_test.go‎

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
package nodeconfigutils
2+
3+
import (
4+
"os"
5+
"os/exec"
6+
"path/filepath"
7+
"strings"
8+
"testing"
9+
10+
aksnodeconfigv1 "github.com/Azure/agentbaker/aks-node-controller/pkg/gen/aksnodeconfig/v1"
11+
"github.com/stretchr/testify/require"
12+
)
13+
14+
// setupWorktree creates a git worktree at the specified commit and returns its path.
15+
func setupWorktree(t *testing.T, targetCommit string) string {
16+
// Fetch the commit in case of shallow clone (common in CI)
17+
fetchCmd := exec.Command("git", "fetch", "--depth=1", "origin", targetCommit)
18+
if output, err := fetchCmd.CombinedOutput(); err != nil {
19+
t.Logf("Warning: Failed to fetch commit (might already exist): %s", output)
20+
}
21+
22+
worktreePath := t.TempDir()
23+
cmd := exec.Command("git", "worktree", "add", worktreePath, targetCommit)
24+
output, err := cmd.CombinedOutput()
25+
require.NoError(t, err, "Failed to create worktree: %s", output)
26+
27+
t.Cleanup(func() {
28+
_ = exec.Command("git", "worktree", "remove", "--force", worktreePath).Run()
29+
})
30+
31+
// Log commit info
32+
cmd = exec.Command("git", "log", "-1", "--oneline", targetCommit)
33+
if commitInfo, err := cmd.CombinedOutput(); err == nil {
34+
t.Logf("Testing against: %q", strings.TrimSpace(string(commitInfo)))
35+
}
36+
37+
return worktreePath
38+
}
39+
40+
// TestBackwardCompatibility tests that old code can unmarshal configs generated by current code.
41+
func TestBackwardCompatibility(t *testing.T) {
42+
targetCommit := os.Getenv("BACKWARD_COMPAT_COMMIT")
43+
if targetCommit == "" {
44+
// Commit from 2025-11-08 when the first version of Config was released.
45+
// CAREFUL: Test failure may indicate backward compatibility breakage.
46+
// This commit should be 6-8 months old to account for VHD release cycles and deployment timelines.
47+
// Investigate thoroughly before updating.
48+
targetCommit = "c4ddd55f7dd72bfb541313fe586f586dfa7bd114"
49+
}
50+
51+
t.Logf("Testing backward compatibility with commit: %s", targetCommit)
52+
53+
// Generate JSON with current code
54+
cfg := &aksnodeconfigv1.Configuration{}
55+
PopulateAllFields(cfg)
56+
57+
currentJSON, err := MarshalConfigurationV1(cfg)
58+
require.NoError(t, err)
59+
t.Logf("Generated %d bytes of JSON with current code", len(currentJSON))
60+
61+
// Create git worktree at target commit
62+
worktreePath := setupWorktree(t, targetCommit)
63+
64+
// Write JSON to worktree
65+
pkgPath := filepath.Join(worktreePath, "aks-node-controller", "pkg", "nodeconfigutils")
66+
jsonPath := filepath.Join(pkgPath, "test_compat.json")
67+
require.NoError(t, os.WriteFile(jsonPath, currentJSON, 0644))
68+
69+
// Create test file to unmarshal with old code
70+
testCode := `package nodeconfigutils
71+
import (
72+
"os"
73+
"testing"
74+
"github.com/stretchr/testify/require"
75+
)
76+
func TestUnmarshalCompat(t *testing.T) {
77+
data, err := os.ReadFile("test_compat.json")
78+
require.NoError(t, err)
79+
cfg, err := UnmarshalConfigurationV1(data)
80+
require.NoError(t, err, "Old code should unmarshal current JSON")
81+
require.NotNil(t, cfg)
82+
t.Logf("✓ Successfully unmarshaled %d bytes", len(data))
83+
}
84+
`
85+
testFile := filepath.Join(pkgPath, "compat_test.go")
86+
require.NoError(t, os.WriteFile(testFile, []byte(testCode), 0644))
87+
88+
// Run test with old code
89+
cmd := exec.Command("go", "test", "-v", "-run", "TestUnmarshalCompat", "./pkg/nodeconfigutils")
90+
cmd.Dir = filepath.Join(worktreePath, "aks-node-controller")
91+
output, err := cmd.CombinedOutput()
92+
93+
t.Logf("Old code output:\n%s", output)
94+
require.NoError(t, err, "❌ BACKWARD COMPATIBILITY BROKEN: Old code (%s) cannot unmarshal current JSON", targetCommit)
95+
}
96+
97+
// TestForwardCompatibility tests that current code can unmarshal configs generated by old code.
98+
func TestForwardCompatibility(t *testing.T) {
99+
targetCommit := os.Getenv("FORWARD_COMPAT_COMMIT")
100+
if targetCommit == "" {
101+
// Commit when PopulateAllFields was first introduced.
102+
// CAREFUL: Test failure may indicate forward compatibility breakage.
103+
// Only active RP releases (potentially multiple) need to be supported.
104+
// Investigate thoroughly before updating.
105+
targetCommit = "878718c80db175859a6d7cf377f7b7d40c438413"
106+
}
107+
108+
t.Logf("Testing forward compatibility with commit: %s", targetCommit)
109+
110+
// Create git worktree at target commit
111+
worktreePath := setupWorktree(t, targetCommit)
112+
pkgPath := filepath.Join(worktreePath, "aks-node-controller", "pkg", "nodeconfigutils")
113+
114+
// Try to generate JSON with old code using PopulateAllFields
115+
generateCode := `package nodeconfigutils
116+
import (
117+
"os"
118+
"testing"
119+
aksnodeconfigv1 "github.com/Azure/agentbaker/aks-node-controller/pkg/gen/aksnodeconfig/v1"
120+
"github.com/stretchr/testify/require"
121+
)
122+
func TestGenerateCompat(t *testing.T) {
123+
cfg := &aksnodeconfigv1.Configuration{}
124+
PopulateAllFields(cfg)
125+
data, err := MarshalConfigurationV1(cfg)
126+
require.NoError(t, err)
127+
require.NoError(t, os.WriteFile("old_config.json", data, 0644))
128+
t.Logf("Generated %d bytes", len(data))
129+
}
130+
`
131+
generateFile := filepath.Join(pkgPath, "generate_compat_test.go")
132+
require.NoError(t, os.WriteFile(generateFile, []byte(generateCode), 0644))
133+
134+
// Run the generation test
135+
cmd := exec.Command("go", "test", "-v", "-run", "TestGenerateCompat", "./pkg/nodeconfigutils")
136+
cmd.Dir = filepath.Join(worktreePath, "aks-node-controller")
137+
output, err := cmd.CombinedOutput()
138+
139+
t.Logf("Old code generation output:\n%s", output)
140+
require.NoError(t, err, "Old code failed to generate JSON")
141+
142+
// Read generated JSON
143+
jsonPath := filepath.Join(pkgPath, "old_config.json")
144+
oldJSON, err := os.ReadFile(jsonPath)
145+
require.NoError(t, err)
146+
t.Logf("Generated %d bytes of JSON with old code", len(oldJSON))
147+
148+
// Unmarshal with current code
149+
cfg, err := UnmarshalConfigurationV1(oldJSON)
150+
require.NoError(t, err, "❌ FORWARD COMPATIBILITY BROKEN: Current code cannot unmarshal old JSON (%s)", targetCommit)
151+
require.NotNil(t, cfg)
152+
}

‎aks-node-controller/pkg/nodeconfigutils/testutil.go‎

Lines changed: 27 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import (
88
)
99

1010
// PopulateAllFields recursively populates all fields in a protobuf message with non-zero test values.
11-
// This is useful for testing to ensure all fields can be marshaled/unmarshaled correctly.
11+
// This is used for testing to ensure all fields can be marshaled/unmarshaled correctly.
1212
func PopulateAllFields(msg proto.Message) {
1313
populateMessage(msg.ProtoReflect(), 0)
1414
}
@@ -28,7 +28,8 @@ func populateMessage(msg protoreflect.Message, depth int) {
2828
}
2929

3030
func setFieldValue(msg protoreflect.Message, fd protoreflect.FieldDescriptor, depth int) {
31-
if fd.IsList() {
31+
switch {
32+
case fd.IsList():
3233
// Handle repeated fields - add 2 elements
3334
list := msg.Mutable(fd).List()
3435
for j := 0; j < 2; j++ {
@@ -38,11 +39,11 @@ func setFieldValue(msg protoreflect.Message, fd protoreflect.FieldDescriptor, de
3839
populateMessage(elem.Message(), depth+1)
3940
list.Append(elem)
4041
} else {
41-
val := getDefaultValueForField(fd, fmt.Sprintf("item%d", j), depth)
42+
val := getDefaultValueForField(fd, fmt.Sprintf("item%d", j))
4243
list.Append(val)
4344
}
4445
}
45-
} else if fd.IsMap() {
46+
case fd.IsMap():
4647
// Handle map fields - add 2 entries
4748
mapVal := msg.Mutable(fd).Map()
4849
for j := 0; j < 2; j++ {
@@ -53,22 +54,22 @@ func setFieldValue(msg protoreflect.Message, fd protoreflect.FieldDescriptor, de
5354
populateMessage(val.Message(), depth+1)
5455
mapVal.Set(key, val)
5556
} else {
56-
val := getDefaultValueForMapValue(fd.MapValue(), j, depth)
57+
val := getDefaultValueForMapValue(fd.MapValue(), j)
5758
mapVal.Set(key, val)
5859
}
5960
}
60-
} else if fd.Kind() == protoreflect.MessageKind {
61+
case fd.Kind() == protoreflect.MessageKind:
6162
// Handle singular message fields - use Mutable to get/create the message
6263
nestedMsg := msg.Mutable(fd).Message()
6364
populateMessage(nestedMsg, depth+1)
64-
} else {
65+
default:
6566
// Handle singular primitive fields
66-
val := getDefaultValueForField(fd, "", depth)
67+
val := getDefaultValueForField(fd, "")
6768
msg.Set(fd, val)
6869
}
6970
}
7071

71-
func getDefaultValueForField(fd protoreflect.FieldDescriptor, suffix string, depth int) protoreflect.Value {
72+
func getDefaultValueForField(fd protoreflect.FieldDescriptor, suffix string) protoreflect.Value {
7273
switch fd.Kind() {
7374
case protoreflect.BoolKind:
7475
return protoreflect.ValueOfBool(true)
@@ -100,35 +101,39 @@ func getDefaultValueForField(fd protoreflect.FieldDescriptor, suffix string, dep
100101
return protoreflect.ValueOfEnum(enumDesc.Values().Get(lastIndex).Number())
101102
}
102103
return protoreflect.ValueOfEnum(0)
103-
case protoreflect.MessageKind:
104+
case protoreflect.MessageKind, protoreflect.GroupKind:
104105
// Message fields should be handled in setFieldValue using Mutable
105106
// This shouldn't be called for message fields
106-
return protoreflect.Value{}
107+
panic(fmt.Sprintf("getDefaultValueForField called for message/group field %q - this is a bug", fd.FullName()))
107108
default:
108-
return protoreflect.Value{}
109+
panic(fmt.Sprintf("getDefaultValueForField: unsupported field kind %v for field %q", fd.Kind(), fd.FullName()))
109110
}
110111
}
111112

112113
func getDefaultKeyForKind(kind protoreflect.Kind, index int) protoreflect.MapKey {
113114
switch kind {
114115
case protoreflect.StringKind:
115116
return protoreflect.ValueOfString(fmt.Sprintf("test-key-%d", index)).MapKey()
116-
case protoreflect.Int32Kind:
117-
return protoreflect.ValueOfInt32(int32(index + 1)).MapKey()
118-
case protoreflect.Int64Kind:
117+
case protoreflect.Int32Kind, protoreflect.Sint32Kind, protoreflect.Sfixed32Kind:
118+
return protoreflect.ValueOfInt32(int32(index + 1)).MapKey() //nolint:gosec // Index is always 0 or 1, no overflow risk
119+
case protoreflect.Int64Kind, protoreflect.Sint64Kind, protoreflect.Sfixed64Kind:
119120
return protoreflect.ValueOfInt64(int64(index + 1)).MapKey()
120-
case protoreflect.Uint32Kind:
121-
return protoreflect.ValueOfUint32(uint32(index + 1)).MapKey()
122-
case protoreflect.Uint64Kind:
123-
return protoreflect.ValueOfUint64(uint64(index + 1)).MapKey()
121+
case protoreflect.Uint32Kind, protoreflect.Fixed32Kind:
122+
return protoreflect.ValueOfUint32(uint32(index + 1)).MapKey() //nolint:gosec // Index is always 0 or 1, no overflow risk
123+
case protoreflect.Uint64Kind, protoreflect.Fixed64Kind:
124+
return protoreflect.ValueOfUint64(uint64(index + 1)).MapKey() //nolint:gosec // Index is always 0 or 1, no overflow risk
124125
case protoreflect.BoolKind:
125126
return protoreflect.ValueOfBool(index == 0).MapKey()
127+
case protoreflect.FloatKind, protoreflect.DoubleKind, protoreflect.BytesKind,
128+
protoreflect.EnumKind, protoreflect.MessageKind, protoreflect.GroupKind:
129+
// These types are not valid map key types in protobuf
130+
panic(fmt.Sprintf("getDefaultKeyForKind: invalid map key type %v", kind))
126131
default:
127-
return protoreflect.ValueOfString(fmt.Sprintf("key-%d", index)).MapKey()
132+
panic(fmt.Sprintf("getDefaultKeyForKind: unsupported kind %v", kind))
128133
}
129134
}
130135

131-
func getDefaultValueForMapValue(fd protoreflect.FieldDescriptor, index int, depth int) protoreflect.Value {
136+
func getDefaultValueForMapValue(fd protoreflect.FieldDescriptor, index int) protoreflect.Value {
132137
suffix := fmt.Sprintf("map%d", index)
133-
return getDefaultValueForField(fd, suffix, depth)
138+
return getDefaultValueForField(fd, suffix)
134139
}

0 commit comments

Comments
 (0)