Conversation
…on functions Cover all 9+ branches of validateOpenTelemetryConfig including: - nil config fast-return - enforceHTTPS=true: endpoint required and must use HTTPS - enforceHTTPS=false: endpoint checks bypassed - W3C traceId: valid 32-char hex, too short/long, uppercase, non-hex, all-zero rejection - W3C spanId: valid 16-char hex, too short/long, uppercase, all-zero rejection - spanId without traceId: warning path (no error) - Fully valid config with all fields Also add direct tests for previously uncovered functions: - validateTrustedBots: nil/empty/valid/whitespace/indexed-error cases - validateCustomSchemas: nil/empty/https-valid/reserved-names/non-https rejection - validateGuardPolicies: nil/empty/nil-policy-skip/valid/invalid-with-guard-name - TestValidateGatewayConfig_OpenTelemetry: OTel integration via validateGatewayConfig - TestValidateGatewayConfig_TrustedBots: trustedBots integration via validateGatewayConfig Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds direct unit and integration-style test coverage for OpenTelemetry config validation and several related config validation helpers in internal/config.
Changes:
- Introduces a new test suite covering
validateOpenTelemetryConfigbranches (endpoint HTTPS enforcement + W3C trace/span ID validation). - Adds focused tests for
validateTrustedBots,validateCustomSchemas, andvalidateGuardPolicies. - Extends
validateGatewayConfigcoverage with OpenTelemetry and trusted-bots scenarios.
Show a summary per file
| File | Description |
|---|---|
| internal/config/validation_otel_test.go | New tests targeting uncovered validation branches for OpenTelemetry config and related helper validators. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
Comment on lines
+240
to
+246
| // traceId validation skipped when enforceHTTPS=false and endpoint missing | ||
| { | ||
| name: "invalid traceId still rejected when enforceHTTPS is false", | ||
| cfg: &TracingConfig{ | ||
| TraceID: "not-a-valid-trace-id", | ||
| }, | ||
| enforceHTTPS: false, |
There was a problem hiding this comment.
The comment says traceId validation is skipped when enforceHTTPS=false and endpoint is missing, but the test case below asserts the opposite (invalid traceId is still rejected) and the implementation in validation.go validates TraceID regardless of enforceHTTPS/endpoint. Please update/remove this comment to avoid misleading future readers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test Coverage Improvement:
validateOpenTelemetryConfigFunction Analyzed
internal/configvalidateOpenTelemetryConfigTestValidateGatewayConfigwhich never setOpenTelemetry)internal/config/validation_otel_test.go(new)Why This Function?
validateOpenTelemetryConfighad zero direct test coverage despite being responsible for spec-critical W3C Trace Context validation. The function contains 9+ distinct branches guarding against:traceIDPattern,allZeroTraceID,spanIDPattern,allZeroSpanIDAdditionally, four closely-related validation helpers (
validateTrustedBots,validateCustomSchemas,validateGuardPolicies) were also uncovered and added in the same pass.Tests Added
TestValidateOpenTelemetryConfig— 23 test casesnilconfig fast-return (bothenforceHTTPSvalues)enforceHTTPS=true: missing endpoint → errorenforceHTTPS=true: HTTP endpoint → HTTPS errorenforceHTTPS=true: bare hostname → HTTPS errorenforceHTTPS=true: valid HTTPS endpoint → passenforceHTTPS=false: missing endpoint allowedenforceHTTPS=false: HTTP endpoint allowedenforceHTTPS=falsewith invalid traceId still rejectedTestValidateTrustedBots— 8 test casesnilbots → validtrusted_bots[0],trusted_bots[1])TestValidateCustomSchemas— 10 test casesnilmap → validnilschema value → valid"stdio"→ error"http"→ errorTestValidateGuardPolicies— 6 test casesnilguards map → passnilGuardConfig → skippednilpolicy → skippedTestValidateGatewayConfig_OpenTelemetry— 6 integration casesTestValidateGatewayConfig_TrustedBots— 4 integration casesCoverage Report
Generated by Test Coverage Improver
Next run will target:
internal/server/response_writer.goorinternal/proxy/handler.goWarning
The following domain was blocked by the firewall during workflow execution:
proxy.golang.orgTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.