refactor: deduplicate logs-stats/logs-summary pipeline and test scaffolding#2433
refactor: deduplicate logs-stats/logs-summary pipeline and test scaffolding#2433
Conversation
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (2 files)
✨ New Files (1 files)
Coverage comparison generated by |
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.
There was a problem hiding this comment.
Pull request overview
This PR refactors the log-reporting commands by extracting the shared stats/summary pipeline into runLogsCommand(...) and consolidating duplicated Jest setup into a reusable test harness. In the broader codebase, it simplifies the awf logs stats / awf logs summary command handlers while trying to preserve their only intentional behavioral difference: when source-selection info logs are emitted.
Changes:
- Added
runLogsCommand(options, shouldLog)inlogs-command-helpers.tsand routed bothlogs-statsandlogs-summarythrough it. - Tightened shared logging option typing from generic strings to
LogStatsFormat. - Added
createLogCommandTestHarness()and rewired the stats/summary test files to use the shared setup.
Show a summary per file
| File | Description |
|---|---|
src/commands/test-helpers.ts |
Adds a shared Jest harness for logs command tests. |
src/commands/logs-summary.ts |
Replaces inline summary pipeline with shared helper call and summary-specific logging predicate. |
src/commands/logs-summary.test.ts |
Refactors summary tests to use the shared harness. |
src/commands/logs-stats.ts |
Replaces inline stats pipeline with shared helper call and stats-specific logging predicate. |
src/commands/logs-stats.test.ts |
Refactors stats tests to use the shared harness. |
src/commands/logs-command-helpers.ts |
Introduces the shared stats/summary execution pipeline and narrows logging-related types. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 3
| /** | ||
| * Shared test helpers for log command tests (stats, summary) | ||
| */ | ||
|
|
||
| import * as logDiscovery from '../logs/log-discovery'; | ||
| import * as logAggregator from '../logs/log-aggregator'; | ||
| import * as statsFormatter from '../logs/stats-formatter'; | ||
|
|
||
| /** | ||
| * Creates typed mock references and registers shared beforeEach/afterEach | ||
| * hooks for log command tests. Call once at the top of a describe block. | ||
| * | ||
| * Note: jest.mock() calls for log-discovery, log-aggregator, stats-formatter, | ||
| * and logger must remain in each test file — Jest hoists them file-locally. | ||
| * | ||
| * @returns Harness with typed mock references and spy instances (mockExit and | ||
| * mockConsoleLog are populated before each test runs). | ||
| */ | ||
| export function createLogCommandTestHarness() { |
| // For summary command: only show info logs in pretty format. | ||
| // This differs intentionally from `logs-stats` which logs for all non-JSON formats. | ||
| // The stricter approach here keeps markdown output (the default, intended for | ||
| // GitHub Actions step summaries) free of extra lines that would pollute $GITHUB_STEP_SUMMARY. | ||
| const source = await discoverAndSelectSource(options.source, { | ||
| format: options.format, | ||
| shouldLog: (format) => format === 'pretty', | ||
| }); | ||
|
|
||
| // Load and aggregate logs | ||
| const stats = await loadLogsWithErrorHandling(source); | ||
|
|
||
| // Format and output | ||
| const colorize = !!(process.stdout.isTTY && options.format === 'pretty'); | ||
| const output = formatStats(stats, options.format, colorize); | ||
| console.log(output); | ||
| await runLogsCommand(options, (format) => format === 'pretty'); |
| // For stats command: show info logs for all non-JSON formats | ||
| const source = await discoverAndSelectSource(options.source, { | ||
| format: options.format, | ||
| shouldLog: (format) => format !== 'json', | ||
| }); | ||
|
|
||
| // Load and aggregate logs | ||
| const stats = await loadLogsWithErrorHandling(source); | ||
|
|
||
| // Format and output | ||
| const colorize = !!(process.stdout.isTTY && options.format === 'pretty'); | ||
| const output = formatStats(stats, options.format, colorize); | ||
| console.log(output); | ||
| await runLogsCommand(options, (format) => format !== 'json'); |
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 the review feedback |
All three review points addressed in ceac013:
|
Smoke Test Results✅ GitHub API: Last 2 merged PRs retrieved (#2436, #2434) Overall: PASS
|
🔥 Smoke Test Results
PR: "refactor: deduplicate logs-stats/logs-summary pipeline and test scaffolding" Overall: PASS ✅
|
Smoke Test: Copilot BYOK (Offline) Mode
Running in BYOK offline mode ( PR author: Overall: PASS (core BYOK path verified; pre-step template vars unresolved in this run)
|
Chroot Version Comparison Results
Result: FAILED — Python and Node.js versions differ between host and chroot environments.
|
|
fix: exclude test-utils from build, add shouldLog behavior tests 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: GitHub Actions Services Connectivity
Overall: FAIL —
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
logs-statsandlogs-summaryhad character-for-character identical test boilerplate (~40 lines each) and near-identical production pipelines differing only in a singleshouldLogpredicate.Production
logs-command-helpers.ts: AddedrunLogsCommand(options, shouldLog)— the shared discover→load→format→print pipeline. Also tightenedLoggingOptions.formatandshouldLogfromstringtoLogStatsFormat.logs-stats.ts/logs-summary.ts: Each command now reduces to a singlerunLogsCommandcall passing only its distinguishing predicate:Tests
src/commands/test-helpers.test-utils.ts: NewcreateLogCommandTestHarness()— registers sharedbeforeEach/afterEachhooks and returns typed mock references forlogDiscovery,logAggregator, andstatsFormatter. Named with the.test-utils.tssuffix so it is excluded from the production build (seetsconfig.jsonexclude).logs-stats.test.ts: Uses the shared harness and includes a new test asserting thatlogger.infois emitted for non-JSON formats (pretty,markdown) and suppressed forjson.logs-summary.test.ts: Uses the shared harness and includes a new test asserting thatlogger.infois emitted only forprettyand suppressed formarkdownandjson— directly covering the regression risk of polluting$GITHUB_STEP_SUMMARY.tsconfig.json: Added"**/*.test-utils.ts"to theexcludearray so test helper files are never emitted intodist/or packaged with the CLI.