Skip to content

Commit de561dc

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 de561dc

File tree

4 files changed

+225
-184
lines changed

4 files changed

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

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