chore: Enhance backend test coverage and add new functional tests for Security page
- Added tests to `proxyhost_service_validation_test.go` to validate fallback parsing and handle invalid hostname characters. - Introduced new tests for DNS challenge validation in `proxyhost_service_validation_test.go`. - Updated `current_spec.md` to reflect changes in testing strategy and coverage goals for PR #729. - Enhanced `Security.functional.test.tsx` to include navigation test for Notifications button. - Mocked `useNavigate` from `react-router-dom` to verify navigation behavior in Security page tests.
This commit is contained in:
+256
-222
@@ -1,308 +1,342 @@
|
||||
---
|
||||
post_title: "Current Spec: Stabilize Flaky SMTP STARTTLS+AUTH Unit Test"
|
||||
post_title: "Current Spec: Raise PR #729 Patch Coverage (Backend Targeted Tests)"
|
||||
categories:
|
||||
- actions
|
||||
- testing
|
||||
- backend
|
||||
- reliability
|
||||
- ci
|
||||
- security
|
||||
tags:
|
||||
- go
|
||||
- smtp
|
||||
- starttls
|
||||
- patch-coverage
|
||||
- codecov
|
||||
- pr-729
|
||||
- unit-tests
|
||||
- ci-stability
|
||||
summary: "Implementation plan to remove CI flakiness in MailService STARTTLS+AUTH tests by hardening mock SMTP server lifecycle and connection handling in test helpers only."
|
||||
summary: "Focused, test-first plan to raise PR #729 patch coverage by adding targeted backend tests for low-coverage changed files, prioritized by impact and implementation risk."
|
||||
post_date: 2026-02-22
|
||||
---
|
||||
|
||||
## Active Plan: Stabilize Flaky SMTP STARTTLS+AUTH Unit Test
|
||||
## Active Plan: Raise PR #729 Patch Coverage (Backend)
|
||||
|
||||
Date: 2026-02-22
|
||||
Status: Active and authoritative
|
||||
Scope Type: Test reliability hardening (backend unit tests only)
|
||||
Scope Type: Backend patch-coverage lift via targeted tests
|
||||
Authority: This is the only active authoritative plan section in this file.
|
||||
|
||||
## 1) Introduction
|
||||
|
||||
This plan addresses flakiness in backend unit test
|
||||
`TestMailService_TestConnection_StartTLSSuccessWithAuth` by improving mock SMTP
|
||||
test helpers used by `backend/internal/services/mail_service_test.go`.
|
||||
|
||||
Root-cause hypothesis to validate and fix:
|
||||
|
||||
- Existing mock SMTP server helpers accept only one connection, then exit.
|
||||
- STARTTLS + AUTH flows in `net/smtp` are negotiation-heavy and can involve
|
||||
additional command/connection behavior under CI timing variance.
|
||||
- Single-accept test server behavior creates race-prone shutdown windows.
|
||||
This plan targets PR #729 patch coverage gaps from
|
||||
[docs/reports/codecove_patch_report.md](docs/reports/codecove_patch_report.md)
|
||||
with a minimal, test-first approach.
|
||||
|
||||
Goal:
|
||||
|
||||
- Make test helper servers robust and deterministic by accepting connections in
|
||||
a loop until cleanup, handling each connection in its own goroutine, and
|
||||
enforcing deterministic shutdown that waits for accept-loop exit and active
|
||||
handler goroutines (explicit per-connection synchronization via waitgroup).
|
||||
- Raise patch coverage by adding focused backend tests for changed files with
|
||||
low patch %, ordered by coverage impact first and implementation risk second.
|
||||
|
||||
Non-goal:
|
||||
Non-goals:
|
||||
|
||||
- No production mail service behavior changes.
|
||||
- No frontend changes.
|
||||
- No broad refactors.
|
||||
- No production code changes unless a branch is untestable without a tiny seam.
|
||||
|
||||
## 2) Research Findings
|
||||
|
||||
### 2.1 Current flaky target and helper topology
|
||||
### 2.1 Patch report baseline
|
||||
|
||||
Primary target test:
|
||||
Current patch coverage snapshot from
|
||||
[docs/reports/codecove_patch_report.md](docs/reports/codecove_patch_report.md):
|
||||
|
||||
- `backend/internal/services/mail_service_test.go`
|
||||
- `TestMailService_TestConnection_StartTLSSuccessWithAuth`
|
||||
1. `backend/internal/services/enhanced_security_notification_service.go`
|
||||
- 13.13% (285 missing + 6 partial)
|
||||
2. `backend/internal/services/notification_service.go`
|
||||
- 75.45% (20 missing + 7 partial)
|
||||
3. `backend/internal/api/handlers/notification_provider_handler.go`
|
||||
- 79.22% (15 missing + 1 partial)
|
||||
4. `backend/internal/api/handlers/security_notifications.go`
|
||||
- 83.11% (10 missing + 3 partial)
|
||||
5. `backend/internal/api/handlers/feature_flags_handler.go`
|
||||
- 71.42% (11 missing + 1 partial)
|
||||
6. `backend/internal/services/proxyhost_service.go`
|
||||
- 33.33% (8 missing + 4 partial)
|
||||
7. `backend/internal/api/routes/routes.go`
|
||||
- 75.00% (2 missing + 2 partial)
|
||||
8. `backend/internal/cerberus/cerberus.go`
|
||||
- 73.33% (2 missing + 2 partial)
|
||||
9. `backend/internal/notifications/router.go`
|
||||
- 88.23% (2 missing)
|
||||
|
||||
Helper functions in same file (current behavior):
|
||||
### 2.2 Existing test surface (reuse-first)
|
||||
|
||||
- `startMockSMTPServer(...)`
|
||||
- Single `Accept()` call, single connection handler, then goroutine exits.
|
||||
- `startMockSSLSMTPServer(...)`
|
||||
- Single `Accept()` call, single connection handler, then goroutine exits.
|
||||
- `handleSMTPConn(...)`
|
||||
- Handles SMTP protocol conversation for each accepted connection.
|
||||
Existing package tests are strong and should be extended instead of introducing
|
||||
new harness patterns:
|
||||
|
||||
### 2.2 Flakiness vector summary
|
||||
- Services:
|
||||
- `backend/internal/services/notification_service_test.go`
|
||||
- `backend/internal/services/proxyhost_service_test.go`
|
||||
- `backend/internal/services/enhanced_security_notification_service_discord_only_test.go`
|
||||
- Handlers:
|
||||
- `backend/internal/api/handlers/notification_provider_handler_test.go`
|
||||
- `backend/internal/api/handlers/notification_provider_patch_coverage_test.go`
|
||||
- `backend/internal/api/handlers/security_notifications_patch_coverage_test.go`
|
||||
- `backend/internal/api/handlers/feature_flags_handler_coverage_test.go`
|
||||
- `backend/internal/api/handlers/feature_flags_blocker3_test.go`
|
||||
- Routes:
|
||||
- `backend/internal/api/routes/routes_coverage_test.go`
|
||||
- Cerberus:
|
||||
- `backend/internal/cerberus/cerberus_test.go`
|
||||
- Notifications router:
|
||||
- `backend/internal/notifications/router_test.go`
|
||||
|
||||
- Current single-accept model is fragile for tests where client behavior may
|
||||
include additional negotiation/timing paths.
|
||||
- Cleanup currently closes listener and waits on a done signal, but done is
|
||||
tied to a single accept goroutine rather than a full server lifecycle.
|
||||
- Under CI contention, this can surface nondeterministic failures in tests
|
||||
expecting reliable STARTTLS + AUTH handshake behavior.
|
||||
### 2.3 Constraints
|
||||
|
||||
### 2.3 Repo config review requested by user
|
||||
|
||||
Reviewed files:
|
||||
|
||||
- `.gitignore`
|
||||
- `codecov.yml`
|
||||
- `.dockerignore`
|
||||
- `Dockerfile`
|
||||
|
||||
Conclusion for this scope:
|
||||
|
||||
- No changes required for this test-only helper stabilization.
|
||||
- Existing ignore/coverage/docker config does not block this plan.
|
||||
- Test-first and minimal: backend tests only by default.
|
||||
- Tiny production seam allowed only for truly unreachable branches.
|
||||
- Keep branch behavior assertions deterministic and table-driven where possible.
|
||||
|
||||
## 3) Requirements (EARS)
|
||||
|
||||
- R1: WHEN mock SMTP test helpers are started, THE SYSTEM SHALL accept
|
||||
connections in a loop until explicit cleanup closes the listener.
|
||||
- R2: WHEN a connection is accepted, THE SYSTEM SHALL handle it in its own
|
||||
goroutine so one slow session cannot block new accepts.
|
||||
- R3: WHEN cleanup is invoked, THE SYSTEM SHALL close the listener and await
|
||||
deterministic server-loop termination plus completion of active connection
|
||||
handlers (done channel + waitgroup synchronization).
|
||||
- R4: IF cleanup wait exceeds timeout, THEN THE SYSTEM SHALL report a failure
|
||||
signal and return without hanging test execution.
|
||||
- R5: WHEN this fix is implemented, THE SYSTEM SHALL keep production code
|
||||
untouched and restrict changes to test helper scope.
|
||||
- R6: WHEN targeted backend tests run, THE SYSTEM SHALL pass reliably in local
|
||||
and CI-like conditions.
|
||||
- R1: WHEN addressing PR #729 coverage gaps, THE SYSTEM SHALL prioritize files
|
||||
by missing patch lines first, then by implementation risk.
|
||||
- R2: WHEN adding coverage, THE SYSTEM SHALL modify or add backend test files
|
||||
only, unless a branch is untestable without a tiny seam.
|
||||
- R3: IF a tiny seam is required, THEN THE SYSTEM SHALL keep it package-local,
|
||||
minimal, and solely to unlock deterministic branch testing.
|
||||
- R4: WHEN tests are implemented, THE SYSTEM SHALL validate with local patch
|
||||
report preflight, targeted tests, backend coverage script, and patch report rerun.
|
||||
- R5: WHEN planning delivery, THE SYSTEM SHALL use a single PR unless scope or
|
||||
rollback risk materially increases.
|
||||
- R6: WHEN plan is finalized, THE SYSTEM SHALL explicitly review
|
||||
`.gitignore`, `.dockerignore`, `codecov.yml`, and `Dockerfile` for required
|
||||
updates.
|
||||
|
||||
## 4) Technical Specification
|
||||
|
||||
### 4.1 Exact target files and functions (minimal diff scope)
|
||||
### 4.1 Priority model
|
||||
|
||||
Primary file to edit:
|
||||
Priority score = Coverage impact (missing+partial lines, weighted high) +
|
||||
implementation risk (weighted low).
|
||||
|
||||
- `backend/internal/services/mail_service_test.go`
|
||||
| Priority | File | Coverage Gap | Risk | Strategy |
|
||||
|---|---|---:|---|---|
|
||||
| P0 | `backend/internal/services/enhanced_security_notification_service.go` | 285 + 6 | Medium | New focused coverage file + extend existing Discord-only tests |
|
||||
| P1 | `backend/internal/services/notification_service.go` | 20 + 7 | Medium | Extend existing comprehensive service tests |
|
||||
| P2 | `backend/internal/api/handlers/notification_provider_handler.go` | 15 + 1 | Low | Extend handler patch tests for admin/error branches |
|
||||
| P2 | `backend/internal/api/handlers/feature_flags_handler.go` | 11 + 1 | Low | Extend coverage tests for env/db parse and transaction branches |
|
||||
| P2 | `backend/internal/api/handlers/security_notifications.go` | 10 + 3 | Low | Extend patch tests for source-validation and dispatch-failure branches |
|
||||
| P3 | `backend/internal/services/proxyhost_service.go` | 8 + 4 | Low | Extend validation + advanced config edge branches |
|
||||
| P4 | `backend/internal/api/routes/routes.go` | 2 + 2 | Medium | Extend route wiring error/non-fatal paths only |
|
||||
| P4 | `backend/internal/cerberus/cerberus.go` | 2 + 2 | Low | Extend IsEnabled/cache and fail-closed dispatch edges |
|
||||
| P5 | `backend/internal/notifications/router.go` | 2 | Low | Add table cases for missing flags/default false |
|
||||
|
||||
Functions in scope:
|
||||
### 4.2 Exact target test files to modify/create
|
||||
|
||||
- `startMockSMTPServer(t *testing.T, tlsConf *tls.Config, supportStartTLS bool, requireAuth bool) (string, func())`
|
||||
- `startMockSSLSMTPServer(t *testing.T, tlsConf *tls.Config, requireAuth bool) (string, func())`
|
||||
Primary target files:
|
||||
|
||||
Related function (read-only unless strictly necessary):
|
||||
1. `backend/internal/services/enhanced_security_notification_service_discord_only_test.go` (modify)
|
||||
2. `backend/internal/services/enhanced_security_notification_service_coverage_test.go` (create)
|
||||
3. `backend/internal/services/notification_service_test.go` (modify)
|
||||
4. `backend/internal/api/handlers/notification_provider_patch_coverage_test.go` (modify)
|
||||
5. `backend/internal/api/handlers/notification_provider_handler_test.go` (modify, only if needed)
|
||||
6. `backend/internal/api/handlers/security_notifications_patch_coverage_test.go` (modify)
|
||||
7. `backend/internal/api/handlers/feature_flags_handler_coverage_test.go` (modify)
|
||||
8. `backend/internal/services/proxyhost_service_test.go` (modify)
|
||||
9. `backend/internal/api/routes/routes_coverage_test.go` (modify)
|
||||
10. `backend/internal/cerberus/cerberus_test.go` (modify)
|
||||
11. `backend/internal/notifications/router_test.go` (modify)
|
||||
|
||||
- `handleSMTPConn(conn net.Conn, tlsConf *tls.Config, supportStartTLS bool, requireAuth bool)`
|
||||
### 4.3 Branch targets per file
|
||||
|
||||
Out of scope:
|
||||
#### P0: enhanced security notification service
|
||||
|
||||
- `backend/internal/services/mail_service.go`
|
||||
- Any non-mail-service test files.
|
||||
- `GetSettings` feature-flag off/on branch behavior.
|
||||
- Managed provider aggregation permutations (0/1/many managed destinations).
|
||||
- Gotify incomplete payload rejection path.
|
||||
- Idempotent `updateManagedProviders` (no save when unchanged).
|
||||
- Migration marker checksum no-op and disabled-flag read-only migration behavior.
|
||||
- Feature defaulting matrix (`CHARON_ENV`, `GIN_MODE`, `_test_mode_marker`).
|
||||
- `sendWebhook` SSRF rejection + non-2xx handling.
|
||||
|
||||
### 4.2 Planned helper behavior changes
|
||||
#### P1: notification service
|
||||
|
||||
For both helper server starters:
|
||||
- Event filtering for security event variants (`security_waf`, `security_acl`,
|
||||
`security_rate_limit`, `security_crowdsec`).
|
||||
- Discord URL validation error branches and non-discord fail-closed paths.
|
||||
- `UpdateProvider` mutation guards (deprecated provider type/enable cases).
|
||||
- Template size/timeouts and payload validation branches where uncovered.
|
||||
|
||||
1. Replace single `Accept()` with accept loop:
|
||||
- Continue accepting until listener close returns an error.
|
||||
- Treat listener-close accept errors as normal shutdown path.
|
||||
2. Spawn per-connection handler goroutine:
|
||||
- Each accepted connection handled independently.
|
||||
- Connection closed within goroutine after handling.
|
||||
3. Deterministic lifecycle signaling:
|
||||
- Keep `done` channel signaling tied to server accept-loop exit.
|
||||
- Ensure exactly one close of `done` from server goroutine.
|
||||
- Track active connection-handler goroutines with explicit waitgroup sync.
|
||||
4. Cleanup contract:
|
||||
- `cleanup()` closes listener first.
|
||||
- Wait for accept-loop exit and active handler completion with bounded timeout (`2s` currently acceptable).
|
||||
- Never block indefinitely.
|
||||
- Timeout path is a test failure signal (non-hanging), not a silent pass.
|
||||
#### P2: handler targets
|
||||
|
||||
### 4.3 Concurrency and race safety notes
|
||||
- Notification provider handler:
|
||||
- `Create` and `Update` blocked-code branches (discord-only, immutable legacy,
|
||||
cannot enable deprecated).
|
||||
- `isProviderValidationError` classification branches.
|
||||
- Security notifications handler:
|
||||
- Invalid source IP parsing, unauthorized source path, malformed payload, and
|
||||
accepted path with dispatch error non-fatal behavior.
|
||||
- Feature flags handler:
|
||||
- legacy fallback hard-false resolution branches, invalid env value warning
|
||||
path coverage, and unknown key ignore behavior in updates.
|
||||
|
||||
- Accept-loop ownership:
|
||||
- One goroutine owns listener accept loop and is the only goroutine that may
|
||||
close `done`.
|
||||
- Connection handler isolation:
|
||||
- One goroutine per accepted connection; no shared mutable state required for
|
||||
protocol behavior in this helper.
|
||||
- Per-connection synchronization:
|
||||
- A waitgroup must increment before each handler goroutine starts and must
|
||||
decrement on handler exit so cleanup can deterministically wait for active
|
||||
handlers to finish.
|
||||
- Listener close semantics:
|
||||
- `listener.Close()` from cleanup is expected to break `Accept()`.
|
||||
- Exit condition should avoid noisy test failures on intentional close.
|
||||
- Cleanup timeout behavior:
|
||||
- Timeout remains defensive to prevent suite hangs under pathological CI
|
||||
resource starvation.
|
||||
- Timeout branch must report failure (e.g., `t.Errorf`/`t.Fatalf` policy) and
|
||||
return; no panic and no silent success.
|
||||
#### P3-P5: lower-line but necessary gaps
|
||||
|
||||
### 4.4 Error handling policy in helpers
|
||||
- Proxy host service: malformed forward host/path stripping and advanced config
|
||||
normalization failure branches.
|
||||
- Routes: migration invocation/error paths and non-fatal startup behavior.
|
||||
- Cerberus: feature-flag absent/false fail-closed dispatch and cache refresh
|
||||
branch coverage.
|
||||
- Notifications router: explicit default false behavior for missing flags.
|
||||
|
||||
- Treat only expected shutdown accept errors (listener close path) as normal.
|
||||
- Surface unexpected `Accept()` failures as test failure signals.
|
||||
- Keep helper logic simple and deterministic; avoid over-engineered retry logic.
|
||||
### 4.4 Tiny seam policy (only if required)
|
||||
|
||||
Allowed only when a specific branch cannot be reached with current test hooks.
|
||||
Potential seam examples (if needed):
|
||||
|
||||
- Package-level function var for ticker/sleep in route boot goroutines.
|
||||
- Package-level clock helper for time-dependent branch determinism.
|
||||
|
||||
If introduced:
|
||||
|
||||
- Must be private to package.
|
||||
- Must not alter runtime behavior.
|
||||
- Must be documented in test file rationale and removed if later unnecessary.
|
||||
|
||||
## 5) Implementation Plan
|
||||
|
||||
### Phase 1: Testing protocol sequencing note
|
||||
### Phase 1: Baseline and preflight
|
||||
|
||||
- Policy remains E2E-first globally.
|
||||
- Explicit exception rationale for this change: scope is backend test-helper-only
|
||||
(`mail_service_test.go`) with no production/frontend/runtime behavior delta,
|
||||
so targeted backend test-first verification is authorized for this plan.
|
||||
- Mandatory preflight before unit/coverage steps:
|
||||
- `bash scripts/local-patch-report.sh`
|
||||
- Artifacts: `test-results/local-patch-report.md` and `test-results/local-patch-report.json`
|
||||
1. Run local patch report preflight:
|
||||
- `bash scripts/local-patch-report.sh`
|
||||
2. Confirm artifacts:
|
||||
- `test-results/local-patch-report.md`
|
||||
- `test-results/local-patch-report.json`
|
||||
3. Capture baseline patch gaps for the 9 target files.
|
||||
|
||||
### Phase 2: Baseline confirmation and failure reproduction context
|
||||
### Phase 2: Highest-impact tests first (P0-P1)
|
||||
|
||||
- Capture current helper behavior and flaky test target:
|
||||
- `go test ./backend/internal/services -run TestMailService_TestConnection_StartTLSSuccessWithAuth -count=1 -v`
|
||||
1. Implement `enhanced_security_notification_service` branch tests.
|
||||
2. Implement `notification_service` gap tests.
|
||||
3. Run targeted service tests only:
|
||||
- `go test ./backend/internal/services -run 'Test(DiscordOnly|EnhancedSecurity|NotificationService|ProxyHostService)' -count=1`
|
||||
|
||||
### Phase 3: Helper lifecycle hardening
|
||||
### Phase 3: Handler and service gap closure (P2-P3)
|
||||
|
||||
- Update `startMockSMTPServer` and `startMockSSLSMTPServer` to loop accept.
|
||||
- Add per-connection goroutine handling.
|
||||
- Preserve/strengthen deterministic cleanup using listener close + done wait +
|
||||
per-connection waitgroup completion.
|
||||
1. Extend handler coverage tests for notification provider, security
|
||||
notifications, and feature flags.
|
||||
2. Extend proxy host service tests for uncovered validation/normalization
|
||||
branches.
|
||||
3. Run targeted handler/service tests:
|
||||
- `go test ./backend/internal/api/handlers -run 'Test(NotificationProvider|SecurityNotification|FeatureFlags)' -count=1`
|
||||
- `go test ./backend/internal/services -run 'TestProxyHostService' -count=1`
|
||||
|
||||
### Phase 4: Targeted validation
|
||||
### Phase 4: Wiring and security/router edges (P4-P5)
|
||||
|
||||
- Re-run target test repeatedly to validate stability:
|
||||
- `go test ./backend/internal/services -run TestMailService_TestConnection_StartTLSSuccessWithAuth -count=20`
|
||||
- Run mail service test subset:
|
||||
- `go test ./backend/internal/services -run TestMailService_ -count=1`
|
||||
- Run race-focused targeted validation:
|
||||
- `go test -race ./backend/internal/services -run 'TestMailService_(TestConnection|Send)' -count=1`
|
||||
1. Extend route, cerberus, and notifications router tests for remaining small
|
||||
branch gaps.
|
||||
2. Run targeted package tests:
|
||||
- `go test ./backend/internal/api/routes -run 'TestRegister' -count=1`
|
||||
- `go test ./backend/internal/cerberus -run 'TestCerberus' -count=1`
|
||||
- `go test ./backend/internal/notifications -run 'TestRouter' -count=1`
|
||||
|
||||
### Phase 5: Backend coverage validation
|
||||
### Phase 5: Coverage validation and rerun
|
||||
|
||||
- Run backend coverage task/script required by repo workflow:
|
||||
- Preferred script: `scripts/go-test-coverage.sh`
|
||||
- VS Code equivalent: backend coverage task if available.
|
||||
1. Run backend coverage script:
|
||||
- `scripts/go-test-coverage.sh`
|
||||
2. Re-run local patch report:
|
||||
- `bash scripts/local-patch-report.sh`
|
||||
3. Verify patch coverage increase and remaining uncovered lines are justified or
|
||||
queued.
|
||||
|
||||
## 6) Validation Matrix
|
||||
## 6) Validation Workflow (Required Order)
|
||||
|
||||
| Validation Item | Command / Task | Scope | Pass Criteria |
|
||||
|---|---|---|---|
|
||||
| Targeted flaky test | `go test ./backend/internal/services -run TestMailService_TestConnection_StartTLSSuccessWithAuth -count=20` | Direct flaky test | No failures across repeated runs |
|
||||
| Mail service subset | `go test ./backend/internal/services -run TestMailService_ -count=1` | Nearby regression safety | All selected tests pass |
|
||||
| Race-focused targeted tests | `go test -race ./backend/internal/services -run 'TestMailService_(TestConnection|Send)' -count=1` | Concurrency/race safety | No race reports; tests pass |
|
||||
| Package sanity | `go test ./backend/internal/services -count=1` | Service package confidence | Package tests pass |
|
||||
| Backend coverage gate | `scripts/go-test-coverage.sh` | Repo-required backend coverage check | Meets configured minimum threshold (85% default) |
|
||||
| Step | Command | Expected Result |
|
||||
|---|---|---|
|
||||
| 1 | `npx playwright test --project=firefox` | E2E-first compliance satisfied before unit/coverage work |
|
||||
| 2 | `bash scripts/local-patch-report.sh` | Local patch preflight artifacts generated (`.md` + `.json`) |
|
||||
| 3 | `go test` targeted packages/files (phased above) | Targeted unit validation passes and intended branches are exercised |
|
||||
| 4 | `scripts/go-test-coverage.sh` | Targeted coverage validation passes backend coverage gate |
|
||||
| 5 | `bash scripts/local-patch-report.sh` | Patch % improves for PR #729 target files |
|
||||
|
||||
Notes:
|
||||
## 7) PR Slicing Strategy
|
||||
|
||||
- E2E-first protocol remains project-wide policy. This plan uses the explicit
|
||||
backend-only targeted-test exception because scope is confined to test helper
|
||||
internals with no production/UI behavior changes.
|
||||
- Local patch report preflight is required before unit/coverage gates.
|
||||
|
||||
## 7) Risk Assessment
|
||||
|
||||
Risk level: Low.
|
||||
|
||||
- Change type: test-only.
|
||||
- Production code: untouched.
|
||||
- Runtime behavior: unchanged for shipped binary.
|
||||
- Primary risk: helper lifecycle bug causing test hangs.
|
||||
- Mitigation: bounded cleanup timeout, accept-loop exit on listener close,
|
||||
focused repeated-run validation.
|
||||
|
||||
## 8) PR Slicing Strategy
|
||||
|
||||
Decision: Single PR.
|
||||
Decision: **Single PR**.
|
||||
|
||||
Rationale:
|
||||
|
||||
- Extremely small scope (one test file, two helper functions).
|
||||
- No cross-domain dependencies.
|
||||
- Easier review and rollback.
|
||||
- All work is backend test-only and tightly coupled to one outcome (patch
|
||||
coverage increase for PR #729).
|
||||
- Review remains tractable because changes are limited to test files.
|
||||
- Rollback is low risk (test-only diff).
|
||||
|
||||
### PR-1 (single slice)
|
||||
Contingency trigger for multi-PR split (only if needed):
|
||||
|
||||
- Scope:
|
||||
- `backend/internal/services/mail_service_test.go` helper lifecycle updates.
|
||||
- Dependencies:
|
||||
- None.
|
||||
- Validation gates:
|
||||
- Validation matrix in Section 6.
|
||||
- Rollback contingency:
|
||||
- Revert single PR if instability increases.
|
||||
- If tiny production seams become necessary in multiple packages, split into:
|
||||
- PR-1: minimal seam(s) + seam tests.
|
||||
- PR-2: coverage tests consuming seam(s).
|
||||
|
||||
## 8) Risk Guardrail (P0 Blast Radius)
|
||||
|
||||
Timebox / fallback trigger:
|
||||
|
||||
- If, after completing P0 + P1 and running `bash scripts/local-patch-report.sh`,
|
||||
either of the following is true:
|
||||
- overall patch coverage is still `< 80%`, or
|
||||
- combined uplift from baseline is `< +8 percentage points`,
|
||||
THEN stop further broad branch expansion and switch to fallback mode.
|
||||
|
||||
Fallback decision path (next-step actions):
|
||||
|
||||
1. Freeze any additional new-branch test expansion in `P2+` files.
|
||||
2. Generate and attach latest artifacts:
|
||||
- `test-results/local-patch-report.md`
|
||||
- `test-results/local-patch-report.json`
|
||||
3. Run targeted gap triage on top remaining uncovered changed lines only.
|
||||
4. Choose one path:
|
||||
- Path A (default): add one minimal seam in highest-impact file and finish in
|
||||
same PR.
|
||||
- Path B: split into follow-up PR for seam + residual coverage if seam scope
|
||||
exceeds one package or introduces rollback risk.
|
||||
5. Re-run `bash scripts/local-patch-report.sh` and proceed only if targets in
|
||||
Section 10 are met.
|
||||
|
||||
## 9) Config File Review Outcome
|
||||
|
||||
Reviewed for this request:
|
||||
Reviewed for this plan:
|
||||
|
||||
- `.gitignore`
|
||||
- `codecov.yml`
|
||||
- `.dockerignore`
|
||||
- `codecov.yml`
|
||||
- `Dockerfile`
|
||||
|
||||
Suggested updates:
|
||||
Planned updates: **None** (test-only backend changes do not require updates).
|
||||
|
||||
- None required for this scope.
|
||||
- Revisit only if implementation introduces new generated artifacts or test
|
||||
output paths not currently handled (not expected here).
|
||||
## 10) Acceptance Criteria (Measurable)
|
||||
|
||||
## 10) Acceptance Criteria
|
||||
Overall patch-coverage target for this PR uplift effort:
|
||||
|
||||
- AC1: `startMockSMTPServer` accepts in loop until cleanup and no longer exits
|
||||
after a single connection.
|
||||
- AC2: `startMockSSLSMTPServer` accepts in loop until cleanup and no longer
|
||||
exits after a single connection.
|
||||
- AC3: Each accepted connection is handled in its own goroutine.
|
||||
- AC4: Cleanup closes listener and uses done-channel plus per-connection
|
||||
waitgroup synchronization with bounded timeout.
|
||||
- AC5: Unexpected `Accept()` failures are surfaced as test failure signals;
|
||||
expected listener-close shutdown errors are treated as normal.
|
||||
- AC6: `TestMailService_TestConnection_StartTLSSuccessWithAuth` passes reliably
|
||||
under repeated runs.
|
||||
- AC7: Targeted race validation for mail-service tests passes with `go test -race`.
|
||||
- AC8: Cleanup timeout path reports failure and returns (non-hanging), never
|
||||
silent pass.
|
||||
- AC9: Backend coverage script/task completes successfully at configured
|
||||
threshold.
|
||||
- AC10: No production file changes are included in the implementation PR.
|
||||
- AC1: Overall PR patch coverage (Codecov patch view) reaches `>= 85%`.
|
||||
|
||||
## 11) Definition of Done
|
||||
Per-file patch coverage targets (from current 9-file backend target list):
|
||||
|
||||
- Planned helper changes are implemented exactly in scoped functions.
|
||||
- Cleanup deterministically waits for accept-loop exit and active handlers via
|
||||
done + waitgroup synchronization.
|
||||
- Only expected listener-close shutdown accept errors are non-fatal; unexpected
|
||||
accept errors fail tests.
|
||||
- Cleanup timeout is reported as failure signal and cannot pass silently.
|
||||
- Validation matrix passes.
|
||||
- Diff is limited to test helper scope in `mail_service_test.go`.
|
||||
- No updates to `.gitignore`, `codecov.yml`, `.dockerignore`, or `Dockerfile`
|
||||
are required or included.
|
||||
- AC2: `backend/internal/services/enhanced_security_notification_service.go` `>= 60%`
|
||||
- AC3: `backend/internal/services/notification_service.go` `>= 90%`
|
||||
- AC4: `backend/internal/api/handlers/notification_provider_handler.go` `>= 92%`
|
||||
- AC5: `backend/internal/api/handlers/security_notifications.go` `>= 95%`
|
||||
- AC6: `backend/internal/api/handlers/feature_flags_handler.go` `>= 90%`
|
||||
- AC7: `backend/internal/services/proxyhost_service.go` `>= 75%`
|
||||
- AC8: `backend/internal/api/routes/routes.go` `>= 90%`
|
||||
- AC9: `backend/internal/cerberus/cerberus.go` `>= 90%`
|
||||
- AC10: `backend/internal/notifications/router.go` `>= 95%`
|
||||
|
||||
Artifact-gated pass condition:
|
||||
|
||||
- AC11: `bash scripts/local-patch-report.sh` must generate both artifacts:
|
||||
- `test-results/local-patch-report.md`
|
||||
- `test-results/local-patch-report.json`
|
||||
- AC12: The generated artifacts must show overall target (AC1) and all per-file
|
||||
targets (AC2-AC10) as met for this uplift effort.
|
||||
|
||||
Process and scope checks:
|
||||
|
||||
- AC13: Plan remains backend test-focused with no unrelated production changes.
|
||||
- AC14: `.gitignore`, `.dockerignore`, `codecov.yml`, and `Dockerfile` review
|
||||
remains recorded with no required updates for this scope.
|
||||
|
||||
Reference in New Issue
Block a user