Copilot commented on code in PR #1150: URL: https://github.com/apache/skywalking-banyandb/pull/1150#discussion_r3341292915
########## test/cases/trace/cmd/generate/seed.go: ########## @@ -0,0 +1,230 @@ +// Licensed to Apache Software Foundation (ASF) under one or more contributor +// license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright +// ownership. Apache Software Foundation (ASF) licenses this file to you under +// the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package main + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "slices" +) + +// TagType describes a trace tag value type. +type TagType int + +const ( + TagTypeString TagType = iota + TagTypeInt + TagTypeTimestamp +) + +// Role describes how generated tests use a tag. +type Role int + +const ( + RoleIdentity Role = iota + RoleIndexed + RolePlain +) + +// TagDef describes a trace tag in positional seed order. +type TagDef struct { + Name string + Type TagType + Pos int + Role Role +} + +// SpanRow mirrors one seeded span row used for generation value selection. +type SpanRow struct { + TagValues map[string]any + Span string +} + +// Trace describes the trace schema and rows used by generated tests. +type Trace struct { + Name string + Group string + Groups []string + Tags []TagDef + OrderRules []string + TraceIDTag string + SpanIDTag string + TimestampTag string + Rows []SpanRow +} + +// SeedData returns trace schemas available to the generator. +func SeedData() []Trace { + return []Trace{swTrace()} +} + +// FindTrace returns a trace schema by name. +func FindTrace(name string) *Trace { + for _, traceDef := range SeedData() { + if traceDef.Name == name { + return &traceDef + } + } + return nil +} Review Comment: FindTrace returns the address of the range variable (a copy). This is a common Go pitfall and can lead to surprising behavior if the returned *Trace is ever mutated or compared by address. Return a pointer to the slice element instead. ########## test/cases/trace/cmd/generate/pairwise.go: ########## @@ -0,0 +1,174 @@ +// Licensed to Apache Software Foundation (ASF) under one or more contributor +// license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright +// ownership. Apache Software Foundation (ASF) licenses this file to you under +// the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package main + +// Copied verbatim from test/cases/measure/cmd/generate/pairwise.go — keep in sync; pure combinatorics, no proto types. + +// Pair holds a parameter-value pair for pairwise testing. +type Pair struct { + Param string + Value string +} + +// TestVector is a set of parameter-value assignments. +type TestVector map[string]string + +// PairwiseGenerate generates a minimal set of test vectors covering all pairs +// of parameter values using a simplified IPO (In-Parameter-Order) algorithm. +func PairwiseGenerate(params map[string][]string, constraints []ConstraintFunc) []TestVector { + paramNames := sortedParamNames(params) + if len(paramNames) == 0 { + return nil + } + + // Collect all required pairs + requiredPairs := collectAllPairs(params, constraints) + + // Build test vectors using greedy pair covering + var vectors []TestVector + coveredPairs := make(map[Pair]bool) + + for len(coveredPairs) < len(requiredPairs) { + tv := buildVector(params, paramNames, coveredPairs, constraints) + if tv == nil { + break + } + // Mark newly covered pairs + newCovers := 0 + for _, pn := range paramNames { + for _, pn2 := range paramNames { + if pn >= pn2 { + continue + } + p := Pair{Param: pn + "=" + tv[pn], Value: pn2 + "=" + tv[pn2]} + if !coveredPairs[p] { + coveredPairs[p] = true + newCovers++ + } + } + } + if newCovers == 0 { + break + } + vectors = append(vectors, tv) + } + return vectors +} + +// ConstraintFunc returns false if the combination is invalid. +type ConstraintFunc func(tv TestVector) bool + +func collectAllPairs(params map[string][]string, constraints []ConstraintFunc) map[Pair]bool { + pairs := make(map[Pair]bool) + paramNames := sortedParamNames(params) + for _, pn := range paramNames { + for _, pn2 := range paramNames { + if pn >= pn2 { + continue + } + for _, v1 := range params[pn] { + for _, v2 := range params[pn2] { + tv := TestVector{pn: v1, pn2: v2} + if validConstraints(tv, constraints) { + pairs[Pair{Param: pn + "=" + v1, Value: pn2 + "=" + v2}] = true + } + } + } + } + } + return pairs +} + +func buildVector(params map[string][]string, paramNames []string, coveredPairs map[Pair]bool, constraints []ConstraintFunc) TestVector { + tv := make(TestVector) + fillVector(tv, params, paramNames, 0, coveredPairs, constraints) + return tv +} + +func fillVector(tv TestVector, params map[string][]string, paramNames []string, paramIdx int, coveredPairs map[Pair]bool, constraints []ConstraintFunc) bool { + if paramIdx >= len(paramNames) { + return validConstraints(tv, constraints) + } + pn := paramNames[paramIdx] + bestVal := "" + bestScore := -1 + for _, val := range params[pn] { + tv[pn] = val + if !validConstraints(tv, constraints) { + continue + } + score := countNewPairs(tv, paramNames, paramIdx, coveredPairs) + if score > bestScore { + bestScore = score + bestVal = val + } + } + if bestVal == "" { + // Fallback: pick first valid value + for _, val := range params[pn] { + tv[pn] = val + if validConstraints(tv, constraints) { + bestVal = val + break + } + } + if bestVal == "" { + tv[pn] = params[pn][0] + } + } + tv[pn] = bestVal + return fillVector(tv, params, paramNames, paramIdx+1, coveredPairs, constraints) Review Comment: In fillVector's fallback path, when no value satisfies constraints, the code sets `tv[pn] = params[pn][0]` but never assigns `bestVal`. The subsequent `tv[pn] = bestVal` overwrites the fallback with an empty string, producing invalid vectors and potentially breaking pair coverage. ########## test/cases/trace/cmd/generate/pairwise.go: ########## @@ -0,0 +1,174 @@ +// Licensed to Apache Software Foundation (ASF) under one or more contributor +// license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright +// ownership. Apache Software Foundation (ASF) licenses this file to you under +// the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package main + +// Copied verbatim from test/cases/measure/cmd/generate/pairwise.go — keep in sync; pure combinatorics, no proto types. + +// Pair holds a parameter-value pair for pairwise testing. +type Pair struct { + Param string + Value string +} + +// TestVector is a set of parameter-value assignments. +type TestVector map[string]string + +// PairwiseGenerate generates a minimal set of test vectors covering all pairs +// of parameter values using a simplified IPO (In-Parameter-Order) algorithm. +func PairwiseGenerate(params map[string][]string, constraints []ConstraintFunc) []TestVector { + paramNames := sortedParamNames(params) + if len(paramNames) == 0 { + return nil + } + + // Collect all required pairs + requiredPairs := collectAllPairs(params, constraints) + + // Build test vectors using greedy pair covering + var vectors []TestVector + coveredPairs := make(map[Pair]bool) + + for len(coveredPairs) < len(requiredPairs) { + tv := buildVector(params, paramNames, coveredPairs, constraints) + if tv == nil { + break + } + // Mark newly covered pairs + newCovers := 0 + for _, pn := range paramNames { + for _, pn2 := range paramNames { + if pn >= pn2 { + continue + } + p := Pair{Param: pn + "=" + tv[pn], Value: pn2 + "=" + tv[pn2]} + if !coveredPairs[p] { + coveredPairs[p] = true + newCovers++ + } + } + } + if newCovers == 0 { + break + } + vectors = append(vectors, tv) + } + return vectors +} + +// ConstraintFunc returns false if the combination is invalid. +type ConstraintFunc func(tv TestVector) bool + +func collectAllPairs(params map[string][]string, constraints []ConstraintFunc) map[Pair]bool { + pairs := make(map[Pair]bool) + paramNames := sortedParamNames(params) + for _, pn := range paramNames { + for _, pn2 := range paramNames { + if pn >= pn2 { + continue + } + for _, v1 := range params[pn] { + for _, v2 := range params[pn2] { + tv := TestVector{pn: v1, pn2: v2} + if validConstraints(tv, constraints) { + pairs[Pair{Param: pn + "=" + v1, Value: pn2 + "=" + v2}] = true + } + } + } + } + } + return pairs +} + +func buildVector(params map[string][]string, paramNames []string, coveredPairs map[Pair]bool, constraints []ConstraintFunc) TestVector { + tv := make(TestVector) + fillVector(tv, params, paramNames, 0, coveredPairs, constraints) + return tv +} Review Comment: buildVector ignores the boolean result of fillVector. As written, buildVector never returns nil, so PairwiseGenerate's `if tv == nil { break }` is dead code and the algorithm can continue with an invalid/partial vector when constraints are unsatisfiable. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
