test: add logger/aggregator tests for blocked domain detection#1262
test: add logger/aggregator tests for blocked domain detection#1262
Conversation
Add comprehensive tests ensuring blocked domains are properly detected by the log parsers and aggregated correctly: - Parser: denied HTTP requests, TCP_HIT/TCP_REFRESH decision codes, blocked domain extraction, allowed vs denied same-domain distinction - Aggregator: multiple blocked domains, mixed allowed/denied per domain, end-to-end blocked domain detection from real log lines (HTTPS + HTTP) - Streamer: PID enrichment path (withPid=true), PID lookup failure, PID tracking unavailability warning Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (2 files)
Coverage comparison generated by |
There was a problem hiding this comment.
Pull request overview
This PR expands the log-related unit test suite to cover additional “blocked/denied” Squid access log scenarios and to validate PID/process enrichment behavior when streaming logs.
Changes:
- Added
log-parsertest cases for denied HTTP/HTTPS lines and additional decision codes (e.g.,TCP_DENIED,TCP_HIT,NONE_NONE). - Added
log-aggregatortests to ensure denied/blocked domains are counted and grouped correctly, including end-to-endloadAndAggregatecoverage using realistic log lines. - Added
log-streamertests (with module mocks) forwithPidenrichment and for the “PID tracking not available” warning path.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/logs/log-streamer.test.ts |
Adds PID enrichment tests and mocks for pid-tracker to validate withPid behavior. |
src/logs/log-parser.test.ts |
Adds parsing coverage for denied GET/CONNECT lines and additional Squid decision variants. |
src/logs/log-aggregator.test.ts |
Adds aggregation and loadAndAggregate tests focusing on blocked/denied domain statistics. |
Comments suppressed due to low confidence (1)
src/logs/log-streamer.test.ts:345
inodeis modeled as a string in the PID tracking types (PidTrackResult.inode?: string), but this test uses a numeric inode (56789). Using the correct type here will keep the test aligned with runtime expectations and avoid accidental widening of JSON output shape.
(trackPidForPortSync as jest.Mock).mockReturnValue({
pid: 1234,
cmdline: 'curl https://api.github.com',
comm: 'curl',
inode: 56789,
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| jest.mock('execa'); | ||
| jest.mock('fs'); | ||
| jest.mock('../pid-tracker', () => ({ | ||
| trackPidForPortSync: jest.fn().mockReturnValue({ pid: -1, cmdline: '', comm: '', inode: 0 }), |
There was a problem hiding this comment.
The mock trackPidForPortSync return value uses inode: 0 (number), but the real PidTrackResult.inode type is an optional string. Using the wrong type in the module mock can mask type/serialization issues in PID enrichment; return a string inode (e.g. '0'/'56789') or undefined to match production behavior.
This issue also appears on line 340 of the same file.
| trackPidForPortSync: jest.fn().mockReturnValue({ pid: -1, cmdline: '', comm: '', inode: 0 }), | |
| trackPidForPortSync: jest.fn().mockReturnValue({ pid: -1, cmdline: '', comm: '', inode: '0' }), |
| await streamLogs({ | ||
| follow: false, | ||
| source, | ||
| formatter, | ||
| parse: true, | ||
| withPid: true, | ||
| }); |
There was a problem hiding this comment.
These withPid tests exercise PID enrichment while follow: false on a preserved file source. In the CLI, --with-pid is explicitly disabled unless -f/--follow is set (and the option docs say "real-time only"), so this test is asserting behavior that is effectively unreachable and contradicts the intended contract. Consider switching the test to follow: true (tailing) or otherwise aligning it with the documented/CLI behavior.
| it('should mark TCP_HIT as allowed (cached response)', () => { | ||
| const line = | ||
| '1761074374.646 172.30.0.20:39748 example.com:80 93.184.216.34:80 1.1 GET 200 TCP_HIT:HIER_NONE http://example.com/ "-"'; | ||
| const result = parseLogLine(line); | ||
|
|
||
| expect(result).not.toBeNull(); | ||
| // TCP_HIT is neither TCP_TUNNEL nor TCP_MISS, so isAllowed should be false | ||
| // This documents current behavior: only TCP_TUNNEL and TCP_MISS are considered allowed | ||
| expect(result!.isAllowed).toBe(false); | ||
| }); |
There was a problem hiding this comment.
Test name contradicts the asserted behavior: it says "should mark TCP_HIT as allowed" but the test (correctly, per current isAllowed logic) expects isAllowed to be false. Please rename the test to reflect the actual expectation so failures are easier to interpret.
|
🤖 Smoke test results for ✅ GitHub MCP — Last 2 merged PRs: #1261 "ci: skip CI when only release.yml changes", #1260 "fix: make release workflow compatible with branch protection" Overall: PASS
|
|
Smoke test results (run ✅ GitHub MCP: #1230 docs: document flag validation constraints, #1223 docs: sync version references and add missing CLI flags Overall: PASS
|
Chroot Version Comparison Results
Result: ❌ Not all versions matched — Python and Node.js versions differ between host and chroot environments.
|
|
PRs: chore(deps): aggregated dependency updates; docs: document flag validation constraints
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
Summary
Fixes #100
Test plan
🤖 Generated with Claude Code