Refactor log command tests to centralize empty-stats harness setup#3142
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR refactors duplicated empty-stats test setup for the log command test suites into shared command test utilities.
Changes:
- Adds shared
EMPTY_STATSandsetupEmptyStatsHarnesstest helper. - Updates
logs-stats.test.tsandlogs-summary.test.tsto use the helper instead of duplicating mock setup.
Show a summary per file
| File | Description |
|---|---|
src/commands/test-helpers.test-utils.ts |
Adds shared empty aggregation data and harness setup helper. |
src/commands/logs-summary.test.ts |
Replaces inline empty-stats mock wiring with shared helper. |
src/commands/logs-stats.test.ts |
Replaces inline empty-stats mock wiring with shared helper. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
| export function setupEmptyStatsHarness(harness: LogCommandTestHarness, source: LogSource): void { | ||
| harness.mockedDiscovery.discoverLogSources.mockResolvedValue([source]); | ||
| harness.mockedDiscovery.selectMostRecent.mockReturnValue(source); | ||
| harness.mockedAggregator.loadAndAggregate.mockResolvedValue(EMPTY_STATS); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@copilot address review feedback |
Addressed in commit |
Smoke Test Results❌ GitHub API — HTTP 401 authentication failed Result: FAIL (2/3 tests passed)
|
🔬 Smoke Test Results
Overall: FAIL Pre-computed step outputs (
|
🔥 Smoke Test: Copilot BYOK (Offline) Mode
Running in BYOK offline mode ( Overall: PARTIAL PASS — BYOK inference and file I/O pass; GitHub MCP read requires auth not available in this environment.
|
|
fix: skip node --version check under QEMU emulation in agent Dockerfile Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
|
Smoke test results: 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
|
Chroot Smoke Test Results
Overall: ❌ FAILED — Python and Node.js versions differ between host and chroot.
|
Smoke Test Results — FAIL
Overall: FAIL —
|
✨ Enhancement
logs-stats.test.tsandlogs-summary.test.tscontained the same 22-line empty-stats + mock wiring block, flagged by duplicate-code detection. This change consolidates that shared setup into the existing log command test utilities to reduce drift and keep both suites aligned.What does this improve?
Why is this valuable?
Implementation approach:
EMPTY_STATS(AggregatedStats) insrc/commands/test-helpers.test-utils.ts.setupEmptyStatsHarness(harness, source)in the same helper module.src/commands/logs-stats.test.tssrc/commands/logs-summary.test.tsto call the shared helper instead of duplicating inline setup.