Skip to content

Fix test quality issues in TestLLMValueForSpec: loop variable capture, duplicate names, weak assertions #5179

@yrobla

Description

@yrobla

Context

Identified in PR #5142 review by Copilot and @JAORMX on pkg/client/llm_gateway_test.go.

Problems

1. Parallel loop variable capture (Copilot, line 744)

TestLLMValueForSpec runs subtests with t.Parallel() but closes over the loop variable tc. In Go versions before 1.22 this causes all subtests to read the final tc value, producing flaky/incorrect assertions. Fix:

for _, tc := range cases {
    tc := tc // capture range variable
    t.Run(tc.name, func(t *testing.T) {
        t.Parallel()
        ...
    })
}

(Or upgrade to Go 1.22+ loop semantics if the module is already on that version.)

2. Duplicate subtest names (Copilot)

Two subtests are named "NodeTLSRejectUnauthorized" — one for TLSSkipVerify=false and one for true. Duplicate names make output ambiguous. Rename to e.g. "NodeTLSRejectUnauthorized/TLSSkipVerify=false" and "NodeTLSRejectUnauthorized/TLSSkipVerify=true".

3. Weak assertions in TestRealClientConfigs_ConfigureAndRevert (@JAORMX)

The test uses assert.Contains on the raw file body, so it passes even if a value lands at the wrong JSON pointer. Replace with jsonPointerGet-based assertions, e.g.:

selectedType, found := jsonPointerGet(body, "/security/auth/selectedType")
assert.True(t, found)
assert.Equal(t, "gemini-api-key", selectedType)

References

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingclientgoPull requests that update go code

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions