240 lines
14 KiB
Markdown
240 lines
14 KiB
Markdown
|
# CIIMS Proxy — Test Coverage Fix Plan
|
||
|
|
|
||
|
|
## Objective
|
||
|
|
|
||
|
|
Close all test coverage gaps identified in the codebase analysis. The `internal` package has decent coverage (20 tests) but `cmd/main/main.go` has **zero tests** across 12 functions. This plan adds 49 new tests organized in 4 phases.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Phase 1: `internal` Package — Remaining Gaps (8 tests)
|
||
|
|
|
||
|
|
### Objective
|
||
|
|
Cover the few untested paths in the already well-tested `internal` package.
|
||
|
|
|
||
|
|
### Implementation Plan
|
||
|
|
|
||
|
|
- [x] 1.1 `TestNewClient_ZeroTimeout` — Call `NewClient(0)`, assert error returned and message contains "positive". Also test `NewClient(-1)`.
|
||
|
|
**Rationale:** The error path for invalid timeout values is untested. `loadConfig` depends on this to fail fast.
|
||
|
|
|
||
|
|
- [x] 1.2 `TestNewClient_ValidTimeout` — Call `NewClient(30)`, assert no error and the returned client is non-nil.
|
||
|
|
**Rationale:** Only the success path for `NewClient` is exercised implicitly by other tests; a direct test is cleaner.
|
||
|
|
|
||
|
|
- [x] 1.3 `TestHTTPStatusError_Truncation` — Create `HTTPStatusError` with a body of 600 'x' characters. Assert `Error()` returns a string ending in `"...` and not exceeding ~520 characters (512 + "ciims returned ...: " prefix).
|
||
|
|
**Rationale:** The truncation logic at `http.go:38-40` is untested and could silently break.
|
||
|
|
|
||
|
|
- [x] 1.4 `TestHTTPStatusError_EmptyBody` — Create `HTTPStatusError` with empty body. Assert `Error()` returns `"ciims returned <status>"` without a colon.
|
||
|
|
**Rationale:** The empty-body branch at `http.go:41-43` is untested.
|
||
|
|
|
||
|
|
- [x] 1.5 `TestResponseTooLargeError_Error` — Create `ResponseTooLargeError{Limit: 100}`. Assert `Error()` returns `"ciims response exceeds 100 bytes"`.
|
||
|
|
**Rationale:** Simple error type with no existing direct test.
|
||
|
|
|
||
|
|
- [x] 1.6 `TestCIIMSResponse_NilReceiver` — Call `(*CIIMSResponse)(nil).IsFault()`, `.ErrorMessage()`, `.Messages()`. Assert they return `false`, `""`, and `nil` respectively without panicking.
|
||
|
|
**Rationale:** The nil-guard branches at `http.go:63,67,74` are untested.
|
||
|
|
|
||
|
|
- [x] 1.7 `TestClientSend_SOAPFaultWithHTTP500` — Mock server returns HTTP 500 with a SOAP fault body. Assert `err` is `*HTTPStatusError`, `resp.IsFault()` is true, and `resp.ErrorMessage()` returns the fault text.
|
||
|
|
**Rationale:** `handleSendError` in `main.go` has a code path that extracts SOAP faults from `HTTPStatusError.Body` — this scenario must be tested at the transport layer first.
|
||
|
|
|
||
|
|
- [x] 1.8 `TestXmlElementTexts_MalformedXML` — Call `xmlElementTexts` with truncated XML like `"<soap><Body><string>incomplete"`. Assert it returns gracefully (empty slice) without panicking.
|
||
|
|
**Rationale:** The `decoder.Token()` error path at `codec.go:136-138` is untested.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Phase 2: `cmd/main` — Pure Functions (19 tests)
|
||
|
|
|
||
|
|
### Objective
|
||
|
|
Test all functions in `main.go` that have no Gin dependency: configuration loading, URL parsing, environment parsing, and error handling helpers.
|
||
|
|
|
||
|
|
### Implementation Plan
|
||
|
|
|
||
|
|
- [x] 2.1 `TestNormalizeBaseURL_ValidHTTP` — Input `"http://example.com/path/"`, assert returns `"http://example.com/path"` (trailing slash stripped, scheme lowercased).
|
||
|
|
**Rationale:** Happy path for the URL normalization function.
|
||
|
|
|
||
|
|
- [x] 2.2 `TestNormalizeBaseURL_ValidHTTPS` — Input `"https://EXAMPLE.COM:8443"`, assert host lowercased, scheme lowercased.
|
||
|
|
**Rationale:** Verifies case normalization and port preservation.
|
||
|
|
|
||
|
|
- [x] 2.3 `TestNormalizeBaseURL_Empty` — Input `""`, assert error contains "must not be empty".
|
||
|
|
**Rationale:** Empty input validation.
|
||
|
|
|
||
|
|
- [x] 2.4 `TestNormalizeBaseURL_WhitespaceOnly` — Input `" "`, assert error (after TrimSpace, it's empty).
|
||
|
|
**Rationale:** Edge case for whitespace handling.
|
||
|
|
|
||
|
|
- [x] 2.5 `TestNormalizeBaseURL_InvalidScheme` — Input `"ftp://example.com"`, assert error contains "scheme must be http or https".
|
||
|
|
**Rationale:** Scheme allowlist enforcement.
|
||
|
|
|
||
|
|
- [x] 2.6 `TestNormalizeBaseURL_MissingHost` — Input `"http:///path"`, assert error contains "host is required".
|
||
|
|
**Rationale:** `url.Parse` on `http:///path` sets Host to empty.
|
||
|
|
|
||
|
|
- [x] 2.7 `TestNormalizeBaseURL_QueryNotAllowed` — Input `"http://example.com?a=1"`, assert error contains "query and fragment are not allowed".
|
||
|
|
**Rationale:** Query/fragment rejection prevents URL smuggling.
|
||
|
|
|
||
|
|
- [x] 2.8 `TestNormalizeBaseURL_FragmentNotAllowed` — Input `"http://example.com#section"`, assert error.
|
||
|
|
**Rationale:** Same as above for fragments.
|
||
|
|
|
||
|
|
- [x] 2.9 `TestNormalizeBaseURL_InvalidURL` — Input `"://bad"`, assert error (from `url.Parse`).
|
||
|
|
**Rationale:** Tests the `url.Parse` error path.
|
||
|
|
|
||
|
|
- [x] 2.10 `TestParseAllowedServers_Empty` — Input `""`, assert returns nil slice and nil error.
|
||
|
|
**Rationale:** Empty env var is valid (means "no additional servers").
|
||
|
|
|
||
|
|
- [x] 2.11 `TestParseAllowedServers_WhitespaceOnly` — Input `" , "`, assert error (whitespace-only entries become empty and are rejected by `normalizeBaseURL`).
|
||
|
|
**Rationale:** Edge case for the split logic.
|
||
|
|
|
||
|
|
- [x] 2.12 `TestParseAllowedServers_ValidSingle` — Input `"http://other.example.com"`, assert returns `["http://other.example.com"]`.
|
||
|
|
**Rationale:** Happy path for single additional server.
|
||
|
|
|
||
|
|
- [x] 2.13 `TestParseAllowedServers_ValidMultiple` — Input `"http://a.com,https://b.com:8443"`, assert both are normalized and returned.
|
||
|
|
**Rationale:** Multi-server allowlist parsing.
|
||
|
|
|
||
|
|
- [x] 2.14 `TestParseAllowedServers_InvalidEntry` — Input `"http://good.com,ftp://bad.com"`, assert error references the bad entry and the env var name.
|
||
|
|
**Rationale:** Error propagation with context.
|
||
|
|
|
||
|
|
- [x] 2.15 `TestGetIntEnv_Unset` — Unset the env var, assert returns `(0, nil)`.
|
||
|
|
**Rationale:** Default behavior when env var is absent.
|
||
|
|
|
||
|
|
- [x] 2.16 `TestGetIntEnv_Valid` — Set env var to `"30"`, assert returns `(30, nil)`.
|
||
|
|
**Rationale:** Happy path.
|
||
|
|
|
||
|
|
- [x] 2.17 `TestGetIntEnv_NotAnInteger` — Set env var to `"abc"`, assert error contains "must be an integer".
|
||
|
|
**Rationale:** Parse error path.
|
||
|
|
|
||
|
|
- [x] 2.18 `TestGetIntEnv_Negative` — Set env var to `"-5"`, assert error contains "must be positive".
|
||
|
|
**Rationale:** Negative value rejection (new validation in current code).
|
||
|
|
|
||
|
|
- [x] 2.19 `TestGetIntEnv_Zero` — Set env var to `"0"`, assert error contains "must be positive".
|
||
|
|
**Rationale:** Zero value rejection.]]>
|
||
|
|
---
|
||
|
|
|
||
|
|
## Phase 3: `cmd/main` — Handler-Level Tests (16 tests)
|
||
|
|
|
||
|
|
### Objective
|
||
|
|
Test the Gin HTTP handlers end-to-end using `httptest.NewServer` + a mock CIIMS backend. This is the most critical gap.
|
||
|
|
|
||
|
|
### Implementation Plan
|
||
|
|
|
||
|
|
- [x] 3.1 `TestSendSuccess` — POST to `/send` with valid JSON. Mock CIIMS returns `testSendOK`. Assert 200, `{"error":""}`.
|
||
|
|
**Rationale:** Core happy path for the send endpoint.
|
||
|
|
|
||
|
|
- [x] 3.2 `TestSendSOAPFault` — POST to `/send` with valid JSON. Mock CIIMS returns `testErrMsg`. Assert 500, response contains the fault message.
|
||
|
|
**Rationale:** SOAP fault handling in the handler (was the original nil-pointer bug).
|
||
|
|
|
||
|
|
- [x] 3.3 `TestSendNetworkError` — POST to `/send` with valid JSON. Mock CIIMS is closed before the request. Assert 500, response contains error.
|
||
|
|
**Rationale:** Network error path through `handleSendError`.
|
||
|
|
|
||
|
|
- [x] 3.4 `TestHTTP500WithoutSOAPFaultReturnsStatusError` — POST to `/send` with valid JSON. Mock CIIMS returns HTTP 500 with plain text body. Assert 500, response contains the status error.
|
||
|
|
**Rationale:** HTTP error path through `handleSendError` (non-SOAP-fault branch).
|
||
|
|
|
||
|
|
- [x] 3.5 `TestHTTP500WithSOAPFaultReturnsFaultText` — POST to `/send` with valid JSON. Mock CIIMS returns HTTP 500 with a SOAP fault body. Assert 500, response contains the extracted fault message (not the raw HTTP status).
|
||
|
|
**Rationale:** `handleSendError` has a branch that extracts SOAP faults from `HTTPStatusError.Body` — this is the most complex error path.
|
||
|
|
|
||
|
|
- [x] 3.6 `TestSendMissingRequiredField` — POST to `/send` with `{"user":"x"}` (missing pass, event, msg). Assert 400.
|
||
|
|
**Rationale:** Gin binding validation.
|
||
|
|
|
||
|
|
- [x] 3.7 `TestSendOversizedBody` — POST to `/send` with body > 1MB. Assert 413.
|
||
|
|
**Rationale:** `MaxBytesReader` enforcement.
|
||
|
|
|
||
|
|
- [x] 3.8 `TestSendInvalidJSON` — POST to `/send` with `not json`. Assert 400.
|
||
|
|
**Rationale:** Malformed JSON handling.
|
||
|
|
|
||
|
|
- [x] 3.9 `TestAllowedRequestURLSucceeds` — POST to `/send` with `{"url":"http://allowed.example.com",...}` where the URL is in `AllowedServers`. Assert 200 (request proxied).
|
||
|
|
**Rationale:** Allowlist pass-through.
|
||
|
|
|
||
|
|
- [x] 3.10 `TestDisallowedRequestURLReturns400AndDoesNotCallBackend` — POST to `/send` with `{"url":"http://evil.com",...}`. Assert 400, error contains "not allowed", and backend is never called.
|
||
|
|
**Rationale:** Allowlist rejection.
|
||
|
|
|
||
|
|
- [x] 3.11 `TestReceiveSuccess` — POST to `/receive` with valid JSON. Mock CIIMS returns `testReceiveResp`. Assert 200, response contains messages.
|
||
|
|
**Rationale:** Core happy path for receive.
|
||
|
|
|
||
|
|
- [x] 3.12 `TestReceiveSOAPFault` — POST to `/receive` with valid JSON. Mock CIIMS returns a SOAP fault. Assert 500.
|
||
|
|
**Rationale:** SOAP fault handling in receive (was previously missing — now fixed but untested).
|
||
|
|
|
||
|
|
- [x] 3.13 `TestReceiveInvalidCount` — POST to `/receive` with `"count":0`. Assert 400, error contains "between 1 and". Also test count=1001.
|
||
|
|
**Rationale:** Count lower-bound and upper-bound validation.
|
||
|
|
|
||
|
|
- [x] 3.14 (merged with 3.13) — Already covered by `TestReceiveInvalidCount`.
|
||
|
|
|
||
|
|
- [x] 3.15 `TestReceiveCountValidBoundary` — POST to `/receive` with `"count":1000`. Assert 200 (boundary value).
|
||
|
|
**Rationale:** Maximum allowed count should succeed.
|
||
|
|
|
||
|
|
- [x] 3.16 `TestPing` — GET `/ping`. Assert 200, `{"message":"pong"}`.
|
||
|
|
**Rationale:** Health check endpoint.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Phase 4: `cmd/main` — Config & Router Tests (8 tests)
|
||
|
|
|
||
|
|
### Objective
|
||
|
|
Test `loadConfig`, `newServer`, and `newRouter` with environment variable manipulation.
|
||
|
|
|
||
|
|
### Implementation Plan
|
||
|
|
|
||
|
|
- [x] 4.1 `TestLoadConfig_Minimal` — Set only `CIIMS_SERVER=http://example.com`, unset others. Assert defaults: Listen=`":9090"`, Timeout=240, AllowedServers contains only the server URL.
|
||
|
|
**Rationale:** Default configuration path.
|
||
|
|
|
||
|
|
- [x] 4.2 `TestLoadConfig_Full` — Set all env vars. Assert all values are parsed correctly including `AllowedServers`.
|
||
|
|
**Rationale:** Full configuration path.
|
||
|
|
|
||
|
|
- [x] 4.3 `TestLoadConfigRequiresCIIMSServer` — Set `CIIMS_SERVER=`. Assert error contains "CIIMS_SERVER".
|
||
|
|
**Rationale:** Configuration validation propagates errors.
|
||
|
|
|
||
|
|
- [x] 4.4 `TestLoadConfigRejectsInvalidCIIMSServer` — Set `CIIMS_SERVER=ftp://bad`. Assert error contains "scheme".
|
||
|
|
**Rationale:** Invalid URL causes load failure.
|
||
|
|
|
||
|
|
- [x] 4.5 `TestLoadConfigRejectsNonPositiveTimeout` — Set `CIIMS_TIMEOUT=0`. Assert error contains "positive".
|
||
|
|
**Rationale:** Zero timeout rejection propagates through `NewClient`.
|
||
|
|
|
||
|
|
- [x] 4.6 `TestLoadConfigRejectsInvalidAllowedServer` — Set `CIIMS_ALLOWED_SERVERS=http://bad.example.com?x=1`. Assert error.
|
||
|
|
**Rationale:** Invalid allowed server entry causes load failure.
|
||
|
|
|
||
|
|
- [x] 4.7 `TestNewServerTimeouts` and `TestNewServerConfig` — Call `newServer(config)`. Assert `ReadHeaderTimeout`, `ReadTimeout`, `IdleTimeout` are set, `Addr` matches config.
|
||
|
|
**Rationale:** Server construction verification.
|
||
|
|
|
||
|
|
- [x] 4.8 `TestNewRouter_RoutesExist` — Call `newRouter(config)`. Use `httptest.NewServer` with the Gin engine. Assert GET `/ping`, POST `/send`, POST `/receive` all return non-404.
|
||
|
|
**Rationale:** Route registration verification.
|
||
|
|
|
||
|
|
### Additional Tests (beyond original plan)
|
||
|
|
|
||
|
|
- [x] `TestDefaultURLUsesCIIMSServer` — Verifies the service prefix is appended to the default URL.
|
||
|
|
- [x] `TestRequestURLWithQueryReturns400` — Verifies URLs with query params are rejected.
|
||
|
|
- [x] `TestLoadConfigAllowedServers` — Verifies case normalization and multiple allowed servers.
|
||
|
|
- [x] `TestLoadConfig_InvalidTimeout` — Verifies non-integer timeout produces error.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Verification Criteria
|
||
|
|
|
||
|
|
- All 49 new tests pass with `go test ./... -count=1`
|
||
|
|
- `go vet ./...` reports no issues
|
||
|
|
- Handler tests use `t.Setenv()` to avoid cross-test pollution
|
||
|
|
- Handler tests create a fresh `App` + mock CIIMS `httptest.Server` per sub-test
|
||
|
|
- `internal` tests remain in `package internal`
|
||
|
|
- `cmd/main` tests go in `cmd/main/main_test.go` (package `main`)
|
||
|
|
- No test depends on external network access
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Potential Risks and Mitigations
|
||
|
|
|
||
|
|
1. **Environment variable pollution between tests**
|
||
|
|
Mitigation: Use `t.Setenv()` which automatically restores the original value after the test. For `loadConfig` tests, set all relevant vars in each test case.
|
||
|
|
|
||
|
|
2. **Gin mode pollution (debug vs release logging)**
|
||
|
|
Mitigation: Call `gin.SetMode(gin.TestMode)` in `TestMain` or at the top of each test function.
|
||
|
|
|
||
|
|
3. **Port conflicts in parallel handler tests**
|
||
|
|
Mitigation: Use `httptest.NewServer` (which allocates a random port) for both the proxy and the mock CIIMS backend. Do not use `t.Parallel()` for handler tests that share mock servers.
|
||
|
|
|
||
|
|
4. **`getIntEnv` tests need `os.Setenv`**
|
||
|
|
Mitigation: Use `t.Setenv()` — this is the standard approach since Go 1.17.
|
||
|
|
|
||
|
|
5. **`loadConfig` reads multiple env vars**
|
||
|
|
Mitigation: Set all env vars explicitly in each test case to avoid inheriting values from the test environment.
|
||
|
|
|
||
|
|
---
|
||
|
|
|
||
|
|
## Alternative Approaches
|
||
|
|
|
||
|
|
1. **Table-driven tests for handlers**: Group all send/receive scenarios into `[]struct{name, body, mockResponse, mockStatus, wantStatus, wantBody}` tables. This reduces boilerplate but makes individual test failure messages less clear. **Recommendation**: Use table-driven for `normalizeBaseURL` and `getIntEnv` (pure functions), keep handler tests as individual functions for clarity.
|
||
|
|
|
||
|
|
2. **Refactor handlers to accept `http.Handler` interface**: Extract handler logic from Gin to standard `http.Handler` for easier testing. **Recommendation**: Not necessary — `httptest.NewServer` with Gin works fine and tests the actual routing layer.
|
||
|
|
|
||
|
|
3. **Use `httptest.NewRecorder` directly on Gin**: Call `router.ServeHTTP(w, req)` instead of `httptest.NewServer`. **Recommendation**: Use `httptest.NewServer` for handler tests because `sendMessage` needs a real TCP connection to the mock CIIMS backend (it calls `client.Send` with a URL).
|