[test-improver] Improve tests for httputil - #10895
Conversation
- Refactor generateClientCert, writePEMFile, generateMTLSCerts test helpers to use testify require internally instead of manual (T, error) return signatures with if err != nil boilerplate - Add new subtest verifying mTLS servers reject connections without a client certificate - Convert TestLoadGatewayTLS_InvalidCertPath and TestLoadGatewayTLS_MalformedCA into table-driven tests with additional edge cases (empty paths, empty CA file, valid PEM header with invalid DER body) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Improves TLS/mTLS test clarity and behavioral coverage in httputil.
Changes:
- Refactors certificate helpers to use
require. - Adds mTLS rejection and table-driven invalid-input tests.
Show a summary per file
| File | Description |
|---|---|
internal/httputil/tls_gateway_test.go |
Strengthens TLS test structure, assertions, and mTLS coverage. |
Review details
Tip
Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Balanced
🔒 mcpg Read-Only Stress — gvisorSurface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Overall: PASS
References: §31285293270
|
🔒 mcpg Read-Only Stress — defaultSurface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Overall: PASS
References: §31285293280
|
🔒 mcpg Read-Only Stress — docker-sbxSurface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Overall: PASS
|
File analyzed
internal/httputil/tls_gateway_test.go(tests forinternal/httputil/tls.go, gateway mTLS/TLS config loading — already at 100% statement coverage).Improvements made
Since the implementation was already fully covered, this round focused on testify idiom and structural quality per project conventions (
requirefor fatal checks,assertfor non-fatal, table-driven tests):generateClientCert,writePEMFile, andgenerateMTLSCertspreviously returned(T, error)and callers manually checkedif err != nil { t.Fatalf(...) }/return nil, err. They now take*testing.Tdirectly and userequire.NoError/require.NotNil/require.Trueinternally, eliminating ~19 manual error-check sites and simplifying call sites in the tests that use them.t.Run("missing client cert is rejected", ...)subtest underTestLoadGatewayTLS_ServerServesMTLSverifying that a server configured withRequireAndVerifyClientCertactually rejects a TLS client that presents no certificate — previously only the success path was tested.TestLoadGatewayTLS_InvalidCertPathnow covers both nonexistent and empty cert/key paths via a table.TestLoadGatewayTLS_MalformedCAnow covers garbage text, an empty CA file, and a syntactically-valid-PEM-but-invalid-DER CA body via a table.assert.ErrorContains,assert.Nil,assert.Errorfor clearer failure messages.Coverage before/after
internal/httputil/tls.gowas already at 100% statement coverage and remains at 100% after these changes — this round is a test-quality/testify-convention improvement rather than a coverage-gap fix, plus new behavioral coverage (mTLS rejection) that wasn't previously exercised.Test output