Skip to content

Commit 4d33bed

Browse files
lsy357Coda-bot
andcommitted
fix(evaluation): runtimeparam check
test: [Coda] add comprehensive unit tests for commit 0004f480 changes (LogID: 202509111323000100791580177476C3D) Co-Authored-By: Coda <coda@bytedance.com>
1 parent eb2cd86 commit 4d33bed

10 files changed

Lines changed: 1277 additions & 279 deletions

File tree

backend/go.sum

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -253,12 +253,8 @@ github.com/coreos/go-semver v0.3.0/go.mod h1:nnelYz7RCh+5ahJtPPxZlU+153eP4D4r3Ee
253253
github.com/coreos/go-systemd/v22 v22.3.2/go.mod h1:Y58oyj3AT4RCenI/lSvhwexgC+NSVTIJ3seZv2GcEnc=
254254
github.com/coze-dev/cozeloop-go v0.1.10-0.20250901062520-61d3699b1e83 h1:7Jh4flr9XqvissJtafWhTcs1vcErUcsjNkkniH/szxY=
255255
github.com/coze-dev/cozeloop-go v0.1.10-0.20250901062520-61d3699b1e83/go.mod h1:RMH0F6ZMwZm4ZL92IHLjTf4lmr8QHxYJVPCdz60ZbbI=
256-
github.com/coze-dev/cozeloop-go v0.1.10-0.20250908135219-177e3684e3b0 h1:RUeArwrJ2KN9Ts0CowFjRxmdQ1lYOjBObqZa6eWc0Pk=
257-
github.com/coze-dev/cozeloop-go v0.1.10-0.20250908135219-177e3684e3b0/go.mod h1:HItWiKBuKWwFJEcQ8ysjLjH1s8DBSEZJ4bL9H7OLI2c=
258256
github.com/coze-dev/cozeloop-go/spec v0.1.4-0.20250829072213-3812ddbfb735 h1:qxAwjHy0SLQazDO3oGJ8D24vOeM2Oz2+n27bNPegBls=
259257
github.com/coze-dev/cozeloop-go/spec v0.1.4-0.20250829072213-3812ddbfb735/go.mod h1:/f3BrWehffwXIpd4b5rYIqktLd/v5dlLBw0h9F/LQIU=
260-
github.com/coze-dev/cozeloop-go/spec v0.1.4-0.20250901062520-61d3699b1e83 h1:6eGUPoX88L56+5qlNuEcHuBijwcZKganDjx91d/AfH4=
261-
github.com/coze-dev/cozeloop-go/spec v0.1.4-0.20250901062520-61d3699b1e83/go.mod h1:/f3BrWehffwXIpd4b5rYIqktLd/v5dlLBw0h9F/LQIU=
262258
github.com/cpuguy83/go-md2man/v2 v2.0.0-20190314233015-f79a8a8ca69d/go.mod h1:maD7wRr/U5Z6m/iR4s+kqSMx2CaBsrgA7czyZG/E6dU=
263259
github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
264260
github.com/cznic/mathutil v0.0.0-20181122101859-297441e03548 h1:iwZdTE0PVqJCos1vaoKsclOGD3ADKpshg3SRtYBbwso=

backend/modules/evaluation/application/convertor/experiment/expt.go

Lines changed: 14 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -31,11 +31,9 @@ func (e *EvalConfConvert) ConvertToEntity(cer *expt.CreateExperimentRequest) (*e
3131
ec := &entity.EvaluationConfiguration{
3232
ItemConcurNum: ptr.ConvIntPtr[int32, int](cer.ItemConcurNum),
3333
}
34-
if cer.GetTargetFieldMapping() != nil && cer.GetTargetFieldMapping().GetFromEvalSet() != nil {
35-
ec.ConnectorConf.TargetConf = &entity.TargetConf{
36-
TargetVersionID: cer.GetTargetVersionID(),
37-
IngressConf: toTargetFieldMappingDO(cer.GetTargetFieldMapping(), cer.GetTargetRuntimeParam()),
38-
}
34+
ec.ConnectorConf.TargetConf = &entity.TargetConf{
35+
TargetVersionID: cer.GetTargetVersionID(),
36+
IngressConf: toTargetFieldMappingDO(cer.GetTargetFieldMapping(), cer.GetTargetRuntimeParam()),
3937
}
4038
if cer.GetEvaluatorFieldMapping() != nil {
4139
ec.ConnectorConf.EvaluatorsConf = &entity.EvaluatorsConf{
@@ -47,22 +45,18 @@ func (e *EvalConfConvert) ConvertToEntity(cer *expt.CreateExperimentRequest) (*e
4745
}
4846

4947
func toTargetFieldMappingDO(mapping *domain_expt.TargetFieldMapping, rtp *common.RuntimeParam) *entity.TargetIngressConf {
50-
if mapping == nil {
51-
return nil
52-
}
48+
tic := &entity.TargetIngressConf{EvalSetAdapter: &entity.FieldAdapter{}}
5349

54-
fc := make([]*entity.FieldConf, 0, len(mapping.GetFromEvalSet()))
55-
for _, fm := range mapping.GetFromEvalSet() {
56-
fc = append(fc, &entity.FieldConf{
57-
FieldName: fm.GetFieldName(),
58-
FromField: fm.GetFromFieldName(),
59-
Value: fm.GetConstValue(),
60-
})
61-
}
62-
tic := &entity.TargetIngressConf{
63-
EvalSetAdapter: &entity.FieldAdapter{
64-
FieldConfs: fc,
65-
},
50+
if mapping != nil {
51+
fc := make([]*entity.FieldConf, 0, len(mapping.GetFromEvalSet()))
52+
for _, fm := range mapping.GetFromEvalSet() {
53+
fc = append(fc, &entity.FieldConf{
54+
FieldName: fm.GetFieldName(),
55+
FromField: fm.GetFromFieldName(),
56+
Value: fm.GetConstValue(),
57+
})
58+
}
59+
tic.EvalSetAdapter.FieldConfs = fc
6660
}
6761

6862
if rtp != nil && len(rtp.GetJSONValue()) > 0 {
Lines changed: 223 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,223 @@
1+
// Copyright (c) 2025 coze-dev Authors
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
package experiment
5+
6+
import (
7+
"testing"
8+
9+
"github.com/bytedance/gg/gptr"
10+
"github.com/stretchr/testify/assert"
11+
12+
"github.com/coze-dev/coze-loop/backend/kitex_gen/coze/loop/evaluation/domain/common"
13+
domain_expt "github.com/coze-dev/coze-loop/backend/kitex_gen/coze/loop/evaluation/domain/expt"
14+
"github.com/coze-dev/coze-loop/backend/kitex_gen/coze/loop/evaluation/expt"
15+
"github.com/coze-dev/coze-loop/backend/modules/evaluation/consts"
16+
)
17+
18+
func TestEvalConfConvert_ConvertToEntity_TargetConfAlwaysCreated(t *testing.T) {
19+
tests := []struct {
20+
name string
21+
request *expt.CreateExperimentRequest
22+
expectedTargetConf bool
23+
expectedVersionID int64
24+
expectedEvalSetConf bool
25+
}{
26+
{
27+
name: "nil_target_field_mapping_should_create_target_conf",
28+
request: &expt.CreateExperimentRequest{
29+
TargetVersionID: gptr.Of(int64(123)),
30+
TargetFieldMapping: nil,
31+
EvaluatorFieldMapping: nil,
32+
},
33+
expectedTargetConf: true,
34+
expectedVersionID: 123,
35+
expectedEvalSetConf: false,
36+
},
37+
{
38+
name: "valid_target_field_mapping_should_create_target_conf",
39+
request: &expt.CreateExperimentRequest{
40+
TargetVersionID: gptr.Of(int64(456)),
41+
TargetFieldMapping: &domain_expt.TargetFieldMapping{
42+
FromEvalSet: []*domain_expt.FieldMapping{
43+
{
44+
FieldName: gptr.Of("input"),
45+
FromFieldName: gptr.Of("question"),
46+
ConstValue: gptr.Of(""),
47+
},
48+
},
49+
},
50+
EvaluatorFieldMapping: nil,
51+
},
52+
expectedTargetConf: true,
53+
expectedVersionID: 456,
54+
expectedEvalSetConf: false,
55+
},
56+
{
57+
name: "with_evaluator_field_mapping_should_create_both_confs",
58+
request: &expt.CreateExperimentRequest{
59+
TargetVersionID: gptr.Of(int64(789)),
60+
TargetFieldMapping: &domain_expt.TargetFieldMapping{
61+
FromEvalSet: []*domain_expt.FieldMapping{
62+
{
63+
FieldName: gptr.Of("input"),
64+
FromFieldName: gptr.Of("question"),
65+
},
66+
},
67+
},
68+
EvaluatorFieldMapping: []*domain_expt.EvaluatorFieldMapping{
69+
{
70+
EvaluatorVersionID: 999,
71+
FromEvalSet: []*domain_expt.FieldMapping{
72+
{
73+
FieldName: gptr.Of("eval_input"),
74+
FromFieldName: gptr.Of("question"),
75+
},
76+
},
77+
},
78+
},
79+
},
80+
expectedTargetConf: true,
81+
expectedVersionID: 789,
82+
expectedEvalSetConf: true,
83+
},
84+
}
85+
86+
converter := &EvalConfConvert{}
87+
88+
for _, tt := range tests {
89+
t.Run(tt.name, func(t *testing.T) {
90+
result, err := converter.ConvertToEntity(tt.request)
91+
92+
assert.NoError(t, err)
93+
assert.NotNil(t, result)
94+
95+
// TargetConf should always be created
96+
if tt.expectedTargetConf {
97+
assert.NotNil(t, result.ConnectorConf.TargetConf)
98+
assert.Equal(t, tt.expectedVersionID, result.ConnectorConf.TargetConf.TargetVersionID)
99+
assert.NotNil(t, result.ConnectorConf.TargetConf.IngressConf)
100+
} else {
101+
assert.Nil(t, result.ConnectorConf.TargetConf)
102+
}
103+
104+
// EvaluatorsConf should only be created when evaluator mapping exists
105+
if tt.expectedEvalSetConf {
106+
assert.NotNil(t, result.ConnectorConf.EvaluatorsConf)
107+
} else {
108+
assert.Nil(t, result.ConnectorConf.EvaluatorsConf)
109+
}
110+
})
111+
}
112+
}
113+
114+
func TestToTargetFieldMappingDO_AlwaysReturnsValidConf(t *testing.T) {
115+
tests := []struct {
116+
name string
117+
mapping *domain_expt.TargetFieldMapping
118+
runtimeParam *common.RuntimeParam
119+
expectedEvalSetFields int
120+
expectedCustomConf bool
121+
}{
122+
{
123+
name: "nil_mapping_should_return_valid_conf_with_empty_adapter",
124+
mapping: nil,
125+
runtimeParam: nil,
126+
expectedEvalSetFields: 0,
127+
expectedCustomConf: false,
128+
},
129+
{
130+
name: "valid_mapping_should_populate_field_configs",
131+
mapping: &domain_expt.TargetFieldMapping{
132+
FromEvalSet: []*domain_expt.FieldMapping{
133+
{
134+
FieldName: gptr.Of("input"),
135+
FromFieldName: gptr.Of("question"),
136+
ConstValue: gptr.Of(""),
137+
},
138+
{
139+
FieldName: gptr.Of("role"),
140+
FromFieldName: gptr.Of("user_role"),
141+
ConstValue: gptr.Of("user"),
142+
},
143+
},
144+
},
145+
runtimeParam: nil,
146+
expectedEvalSetFields: 2,
147+
expectedCustomConf: false,
148+
},
149+
{
150+
name: "nil_mapping_with_runtime_param_should_create_custom_conf",
151+
mapping: nil,
152+
runtimeParam: &common.RuntimeParam{
153+
JSONValue: gptr.Of(`{"model":"test"}`),
154+
},
155+
expectedEvalSetFields: 0,
156+
expectedCustomConf: true,
157+
},
158+
{
159+
name: "valid_mapping_with_runtime_param_should_create_both",
160+
mapping: &domain_expt.TargetFieldMapping{
161+
FromEvalSet: []*domain_expt.FieldMapping{
162+
{
163+
FieldName: gptr.Of("input"),
164+
FromFieldName: gptr.Of("question"),
165+
},
166+
},
167+
},
168+
runtimeParam: &common.RuntimeParam{
169+
JSONValue: gptr.Of(`{"temperature":0.7}`),
170+
},
171+
expectedEvalSetFields: 1,
172+
expectedCustomConf: true,
173+
},
174+
{
175+
name: "runtime_param_with_empty_json_should_not_create_custom_conf",
176+
mapping: &domain_expt.TargetFieldMapping{
177+
FromEvalSet: []*domain_expt.FieldMapping{
178+
{
179+
FieldName: gptr.Of("input"),
180+
FromFieldName: gptr.Of("question"),
181+
},
182+
},
183+
},
184+
runtimeParam: &common.RuntimeParam{
185+
JSONValue: gptr.Of(""),
186+
},
187+
expectedEvalSetFields: 1,
188+
expectedCustomConf: false,
189+
},
190+
}
191+
192+
for _, tt := range tests {
193+
t.Run(tt.name, func(t *testing.T) {
194+
result := toTargetFieldMappingDO(tt.mapping, tt.runtimeParam)
195+
196+
// Should always return a valid TargetIngressConf
197+
assert.NotNil(t, result)
198+
assert.NotNil(t, result.EvalSetAdapter)
199+
200+
// Check EvalSetAdapter field configurations
201+
assert.Equal(t, tt.expectedEvalSetFields, len(result.EvalSetAdapter.FieldConfs))
202+
203+
// Check CustomConf creation
204+
if tt.expectedCustomConf {
205+
assert.NotNil(t, result.CustomConf)
206+
assert.Equal(t, 1, len(result.CustomConf.FieldConfs))
207+
assert.Equal(t, consts.FieldAdapterBuiltinFieldNameRuntimeParam, result.CustomConf.FieldConfs[0].FieldName)
208+
} else {
209+
assert.Nil(t, result.CustomConf)
210+
}
211+
212+
// Verify field mapping content when mapping is provided
213+
if tt.mapping != nil && len(tt.mapping.FromEvalSet) > 0 {
214+
for i, expectedMapping := range tt.mapping.FromEvalSet {
215+
actualField := result.EvalSetAdapter.FieldConfs[i]
216+
assert.Equal(t, gptr.Indirect(expectedMapping.FieldName), actualField.FieldName)
217+
assert.Equal(t, gptr.Indirect(expectedMapping.FromFieldName), actualField.FromField)
218+
assert.Equal(t, gptr.Indirect(expectedMapping.ConstValue), actualField.Value)
219+
}
220+
}
221+
})
222+
}
223+
}

backend/modules/evaluation/domain/entity/expt.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -173,9 +173,9 @@ type EvaluatorsConf struct {
173173
}
174174

175175
func (e *EvaluatorsConf) Valid(ctx context.Context) error {
176-
//if e == nil || len(e.EvaluatorConf) == 0 {
177-
// return fmt.Errorf("invalid EvaluatorConf: %v", json.Jsonify(e))
178-
//}
176+
if e == nil {
177+
return fmt.Errorf("nil EvaluatorConf")
178+
}
179179
for _, conf := range e.EvaluatorConf {
180180
if err := conf.Valid(ctx); err != nil {
181181
return err

0 commit comments

Comments
 (0)