Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions pkg/reconciler/instance/utils/instance_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,12 +136,29 @@ func GetPodComponentName(pod *v1.Pod) string {
func GetPodComponentID(pod *v1.Pod) int32 {
componentId := pod.Labels[v1alpha1.InstanceComponentIDKey]
if len(componentId) != 0 {
id, _ := strconv.Atoi(componentId)
id, err := strconv.ParseInt(componentId, 10, 32)
if err != nil {
return 0
}
return int32(id)
}
list := strings.Split(pod.Name, "-")
componentId = list[len(list)-1]
id, _ := strconv.Atoi(componentId)
lastIdx := len(list) - 1
for lastIdx >= 0 && list[lastIdx] == "" {
lastIdx--
}
if lastIdx < 0 {
return 0
}
if lastIdx > 0 && list[lastIdx-1] == "" {
componentId = "-" + list[lastIdx]
} else {
componentId = list[lastIdx]
}
id, err := strconv.ParseInt(componentId, 10, 32)
if err != nil {
return 0
}
return int32(id)
Comment on lines 137 to 162

Choose a reason for hiding this comment

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

medium

The logic for parsing the component ID string and handling errors is duplicated for both the label and pod name cases. You can refactor the function to first determine the componentId string (from either the label or the pod name), and then perform the parsing once at the end. This will make the code cleaner and adhere to the DRY (Don't Repeat Yourself) principle.

    componentId := pod.Labels[v1alpha1.InstanceComponentIDKey]
	if len(componentId) == 0 {
		list := strings.Split(pod.Name, "-")
		lastIdx := len(list) - 1
		for lastIdx >= 0 && list[lastIdx] == "" {
			lastIdx--
		}
		if lastIdx < 0 {
			return 0
		}
		if lastIdx > 0 && list[lastIdx-1] == "" {
			componentId = "-" + list[lastIdx]
		} else {
			componentId = list[lastIdx]
		}
	}
	id, err := strconv.ParseInt(componentId, 10, 32)
	if err != nil {
		return 0
	}
	return int32(id)

}

Expand Down
176 changes: 176 additions & 0 deletions pkg/reconciler/instance/utils/instance_utils_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,176 @@
package utils

import (
"math"
"testing"

"github.com/stretchr/testify/assert"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/rbgs/api/workloads/v1alpha1"
)

// TestGetPodComponentID 测试修复后的GetPodComponentID函数
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add comments in English.

func TestGetPodComponentID(t *testing.T) {
// 测试用例:覆盖标签存在/不存在、合法值、边界值、超出范围、非数字等场景
tests := []struct {
name string // 测试用例名称
pod *corev1.Pod // 输入的Pod对象
expected int32 // 预期返回的ComponentID
}{
// 场景1:通过标签获取ID(正常情况)
{
name: "valid id from label (positive)",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
v1alpha1.InstanceComponentIDKey: "123",
},
},
},
expected: 123,
},
{
name: "valid id from label (negative)",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
v1alpha1.InstanceComponentIDKey: "-456",
},
},
},
expected: -456,
},

// 场景2:标签存在但值不合法(非数字、超出范围)
{
name: "invalid non-numeric id in label",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
v1alpha1.InstanceComponentIDKey: "abc",
},
},
},
expected: 0, // 错误时返回默认值0
},
{
name: "id in label exceeds int32 max",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
v1alpha1.InstanceComponentIDKey: "2147483648", // 比MaxInt32大1
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

Chinese comment should be in English for consistency with the rest of the codebase.

Copilot uses AI. Check for mistakes.
},
},
},
expected: 0,
},
{
name: "id in label less than int32 min",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
v1alpha1.InstanceComponentIDKey: "-2147483649", // 比MinInt32小1
},
},
},
expected: 0,
},

// 场景3:标签不存在,从Pod名称解析ID(名称格式:前缀-组件名-ID)
{
name: "valid id from pod name (positive)",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "instance-web-789", // 最后一段为ID
Labels: map[string]string{}, // 无ComponentID标签
},
},
expected: 789,
},
{
name: "valid id from pod name (negative)",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "instance-db--987", // 最后一段为负ID
Labels: map[string]string{},
},
},
expected: -987,
},

// 场景4:标签不存在,名称解析不合法
{
name: "invalid non-numeric id in pod name",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "instance-cache-xyz", // 最后一段非数字
Labels: map[string]string{},
},
},
expected: 0,
},
{
name: "id in pod name exceeds int32 max",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "instance-store-2147483648",
Labels: map[string]string{},
},
},
expected: 0,
},
{
name: "id in pod name less than int32 min",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "instance-queue--2147483649",
Labels: map[string]string{},
},
},
expected: 0,
},

// 场景5:边界值测试(int32最大/最小值)
{
name: "int32 max value in label",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
v1alpha1.InstanceComponentIDKey: "2147483647", // math.MaxInt32
},
},
},
expected: math.MaxInt32,
},
{
name: "int32 min value in label",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{
v1alpha1.InstanceComponentIDKey: "-2147483648", // math.MinInt32
},
},
},
expected: math.MinInt32,
},
{
name: "int32 max value in pod name",
pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "instance-max-2147483647",
Labels: map[string]string{},
},
},
expected: math.MaxInt32,
},
}
Comment on lines 16 to 167

Choose a reason for hiding this comment

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

medium

The test suite is very comprehensive and covers a wide range of scenarios. To make it even more robust, I suggest adding a test case for the minimum int32 value parsed from a pod name. This would complete the boundary value testing for pod names, providing symmetry with the existing test for the maximum int32 value (int32 max value in pod name).

Here is a suggested test case to add:

{
	name: "int32 min value in pod name",
	pod: 	&corev1.Pod{
		ObjectMeta: metav1.ObjectMeta{
			Name:   "instance-min--2147483648", // math.MinInt32
			Labels: map[string]string{},
		},
	},
	expected: math.MinInt32,
},

Comment on lines +166 to +167

Choose a reason for hiding this comment

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

medium

Great test coverage! To make it even more robust, consider adding a boundary test case for the minimum int32 value parsed from a pod name. You have a test for the max value from a pod name, and min value from a label, but the math.MinInt32 case from a pod name is missing.

			expected: math.MaxInt32,
		},
		{
			name: "int32 min value in pod name",
			pod: &corev1.Pod{
				ObjectMeta: metav1.ObjectMeta{
					Name:   "instance-min--2147483648", // math.MinInt32
					Labels: map[string]string{},
				},
			},
			expected: math.MinInt32,
		},
	}


// 执行测试用例
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := GetPodComponentID(tt.pod)
assert.Equal(t, tt.expected, result, "测试用例[%s]失败", tt.name)
})
}
}
Loading