refactor: deduplicate transaction-end-before-headers test fixtures#5659
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the aggregateLogs unit tests to deduplicate repeated fixture setup and assertions around filtering benign Squid error:transaction-end-before-headers log entries, improving maintainability of the filtering invariants.
Changes:
- Introduces shared fixture factories (
validTunnelEntries,transactionEndEntry) for repeated log-entry setup. - Centralizes shared expectations into
expectOnlyValidTunnelStatsand reuses it across filtering tests. - Adds a previously-missing assertion ensuring the filtered
'-'domain does not appear inbyDomain.
Show a summary per file
| File | Description |
|---|---|
| src/logs/log-aggregator.test.ts | Refactors aggregateLogs filtering tests by introducing shared helpers for fixtures and assertions. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Low
| const entries: ParsedLogEntry[] = [ | ||
| createLogEntry({ | ||
| domain: 'github.com', | ||
| url: 'github.com:443', | ||
| isAllowed: true | ||
| }), | ||
| createLogEntry({ | ||
| domain: '-', | ||
| url: 'error:transaction-end-before-headers', | ||
| decision: 'NONE_NONE:HIER_NONE', | ||
| statusCode: 0, | ||
| isAllowed: false | ||
| }), | ||
| createLogEntry({ | ||
| domain: 'npmjs.org', | ||
| url: 'npmjs.org:443', | ||
| isAllowed: true | ||
| }), | ||
| ...validTunnelEntries(), | ||
| transactionEndEntry(), | ||
| ]; |
| const entries: ParsedLogEntry[] = [ | ||
| createLogEntry({ | ||
| domain: 'github.com', | ||
| url: 'github.com:443', | ||
| isAllowed: true | ||
| }), | ||
| createLogEntry({ | ||
| domain: '-', | ||
| url: 'error:transaction-end-before-headers', | ||
| clientIp: '::1', // healthcheck from localhost | ||
| decision: 'NONE_NONE:HIER_NONE', | ||
| statusCode: 0, | ||
| isAllowed: false | ||
| }), | ||
| createLogEntry({ | ||
| domain: '-', | ||
| url: 'error:transaction-end-before-headers', | ||
| clientIp: '172.30.0.20', // shutdown-time connection closure | ||
| decision: 'NONE_NONE:HIER_NONE', | ||
| statusCode: 0, | ||
| isAllowed: false | ||
| }), | ||
| createLogEntry({ | ||
| domain: 'npmjs.org', | ||
| url: 'npmjs.org:443', | ||
| isAllowed: true | ||
| }), | ||
| ...validTunnelEntries(), | ||
| transactionEndEntry({ clientIp: '::1' }), // healthcheck from localhost | ||
| transactionEndEntry({ clientIp: '172.30.0.20' }), // shutdown-time connection closure | ||
| ]; |
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
|
✅ Copilot review passed with no inline comments. @copilot Add the |
Address Copilot review feedback: place transaction-end-before-headers entries between valid tunnel entries (not after them) to preserve coverage of order-dependent filtering logic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
🚀 Security Guard has started processing this pull request |
|
✅ Smoke Gemini completed. All facets verified. 💎 |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
✅ Smoke Claude passed |
|
✅ Build Test Suite completed successfully! |
|
✅ Smoke Copilot BYOK AOAI (Entra) completed. Copilot AOAI BYOK (Entra) mode operational. 🔓 |
|
🔌 Smoke Services — All services reachable! ✅ |
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓 |
|
✅ Smoke Copilot BYOK AOAI (api-key) completed. Copilot AOAI BYOK (api-key) mode operational. 🔓 |
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded. |
|
🔑 Smoke Copilot PAT PAT auth validated. All systems operational. ✅ |
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅ |
|
✅ Contribution Check completed successfully! Contribution guidelines review complete for PR #5659: no important issues found. The change is a scoped test refactor, includes/updates tests in the appropriate test file, has a clear PR description, and does not require documentation updates. |
Smoke Test: Claude Engine Validation
Overall result: PASS
|
Smoke Test: Copilot BYOK (Direct) Mode ✅✅ GitHub MCP — Connected, PR data retrieved Status: PASS — Running in direct BYOK mode (COPILOT_PROVIDER_API_KEY) Assignees:
|
🔬 Smoke Test Results
Overall: PASS PR: refactor: deduplicate transaction-end-before-headers test fixtures
|
🔥 Smoke Test: Copilot PAT Auth — PASS
Overall: PASS · Auth mode: PAT (COPILOT_GITHUB_TOKEN) PR: refactor: deduplicate transaction-end-before-headers test fixtures · author
|
Chroot Version Comparison Results
Overall: ❌ Tests did not pass — Python and Node.js versions differ between host and chroot environments.
|
🔭 Smoke Test: API Proxy OpenTelemetry Tracing
All scenarios pass. ✅
|
|
Running in direct BYOK mode (AWF_AUTH_TYPE=github-oidc + AWF_AUTH_AZURE_* + COPILOT_PROVIDER_BASE_URL) via api-proxy → Azure OpenAI (Foundry, o4-mini-aw) authenticated via Microsoft Entra Status: PASS
|
Smoke Test: GitHub Actions Services Connectivity
Overall: FAIL
|
Gemini Engine Smoke Test Results
Overall Status: FAIL Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
|
|
|
Smoke test results:
|
Two
aggregateLogstests repeated identical fixture arrays and assertion blocks for filtering benign Squid operational entries, making the filtering invariant harder to maintain consistently.Changes
validTunnelEntries()— shared factory for thegithub.com+npmjs.orgvalid tunnel fixture pairtransactionEndEntry(overrides?)— shared factory forerror:transaction-end-before-headersentries; accepts overrides to express the::1vs172.30.0.20client IP variants without repeating all five fieldsexpectOnlyValidTunnelStats(stats)— centralizes the seven shared assertions; the second test now also gains thebyDomain.has('-') === falsecheck it was previously missingAll three helpers are scoped inside
describe('aggregateLogs')to avoid polluting the outer test scope.