Skip to content

Strengthen test assertions in llm_gateway_test.go using jsonPointerGet#5187

Merged
yrobla merged 1 commit intomainfrom
fix/5179-test-quality-llm-gateway
May 5, 2026
Merged

Strengthen test assertions in llm_gateway_test.go using jsonPointerGet#5187
yrobla merged 1 commit intomainfrom
fix/5179-test-quality-llm-gateway

Conversation

@yrobla
Copy link
Copy Markdown
Contributor

@yrobla yrobla commented May 5, 2026

Summary

Tests in pkg/client/llm_gateway_test.go were using assert.Contains on raw file bytes, which passes even when a value lands at the wrong JSON pointer or appears only as a key name. This extends the jsonPointerGet-based assertion pattern (introduced in #5142) to all remaining tests in the file.

  • Replace raw-string assert.Contains with jsonPointerGet exact-path assertions in TestConfigureLLMGateway_CreatesFile, _DeepNestedKey, _PreservesExistingKeys, _ProxyMode, and the three TLS skip-verify tests
  • Replace assert.NotContains with _, ok := jsonPointerGet(...); assert.False(t, ok) in TestRevertLLMGateway_RemovesKey and the TLS / Gemini TLS tests
  • assert.Contains is kept where jsonPointerGet cannot help: JSONC comment preservation checks and non-string JSON values (e.g. the permissions object)

Fixes #5179

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests (task test)
  • E2E tests (task test-e2e)
  • Linting (task lint-fix)
  • Manual testing (describe below)

API Compatibility

  • This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

Changes

File Change

Does this introduce a user-facing change?

Implementation plan

Approved implementation plan

Special notes for reviewers

Tests in `pkg/client/llm_gateway_test.go` were using `assert.Contains` on raw
file bytes, which passes even when a value lands at the wrong JSON pointer or
appears only as a key name. This extends the jsonPointerGet-based assertion
pattern (introduced in #5142) to all remaining tests in the file.

- Replace raw-string `assert.Contains` with `jsonPointerGet` exact-path assertions
  in `TestConfigureLLMGateway_CreatesFile`, `_DeepNestedKey`, `_PreservesExistingKeys`,
  `_ProxyMode`, and the three TLS skip-verify tests
- Replace `assert.NotContains` with `_, ok := jsonPointerGet(...); assert.False(t, ok)`
  in `TestRevertLLMGateway_RemovesKey` and the TLS / Gemini TLS tests
- `assert.Contains` is kept where `jsonPointerGet` cannot help: JSONC comment
  preservation checks and non-string JSON values (e.g. the `permissions` object)

Closes #5179
@github-actions github-actions Bot added the size/XS Extra small PR: < 100 lines changed label May 5, 2026
@yrobla yrobla requested a review from Copilot May 5, 2026 07:31
@codecov
Copy link
Copy Markdown

codecov Bot commented May 5, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.57%. Comparing base (655de22) to head (287d0e0).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5187      +/-   ##
==========================================
+ Coverage   67.53%   67.57%   +0.03%     
==========================================
  Files         602      602              
  Lines       61167    61167              
==========================================
+ Hits        41310    41334      +24     
+ Misses      16729    16704      -25     
- Partials     3128     3129       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR strengthens the correctness of pkg/client/llm_gateway_test.go by replacing brittle raw-substring assertions against the serialized settings file with exact JSON-pointer lookups via jsonPointerGet, ensuring tests fail when values land at the wrong path (or only appear as key names).

Changes:

  • Replaced multiple assert.Contains(string(data), ...) checks with jsonPointerGet(data, "/path") + assert.Equal(...) for exact JSON-pointer assertions.
  • Replaced several assert.NotContains(...) checks with jsonPointerGet(...); assert.False(t, ok) in revert and TLS-related tests.
  • Kept raw-substring assertions where the asserted value is non-string JSON (e.g., permissions) or where JSONC comment preservation is being tested.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/client/llm_gateway_test.go
Comment thread pkg/client/llm_gateway_test.go
Comment thread pkg/client/llm_gateway_test.go
Comment thread pkg/client/llm_gateway_test.go
Comment thread pkg/client/llm_gateway_test.go
Comment thread pkg/client/llm_gateway_test.go
@yrobla yrobla merged commit 22f4769 into main May 5, 2026
57 of 58 checks passed
@yrobla yrobla deleted the fix/5179-test-quality-llm-gateway branch May 5, 2026 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Extra small PR: < 100 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

4 participants