XS✔ ◾ Streamline unit test suite#785
Conversation
Strips log-verification assertions that tested internal implementation details (method-entry tracing, JSON response dumps, mid-flow checkpoints, cross-cutting info-log enumerations, redundant log verifies) while preserving all tests of observable behaviour. - Removes 2,640 lines across 11 spec files (~15.7% reduction) - All 783 tests still pass - 100% coverage preserved (statements, branches, functions, lines) Adds TestPlan.md documenting the broader unit test improvement strategy.
Adds tests/testUtilities/stubLocalization.ts that reads the real
resources.resjson file and wires runnerInvoker.loc() to return actual values
with util.format parameter substitution. Replaces hand-written loc() stubs in
seven spec files with a single helper call, removing ~370 lines of duplicated
resource mocks.
Side effects caught by the migration:
- azureReposInvoker.spec.ts had outdated stub text for the insufficient
permissions message ('Code > Read' vs the real 'Code > Read & write'); five
assertions updated to match the real resource.
- tokenManager.spec.ts "invalid scheme" test was passing accidentally because
its manual stub args did not match the actual SUT call, leaving loc() to
return null. The test now receives the real formatted error string and
asserts it, matching the test's own stated intent.
inputs.spec.ts is not migrated in this pass; it embeds the mock return values
in verify(logger.logInfo(...)) assertions across dozens of tests, which
requires a separate per-test refactor.
- 783 tests still pass, 100% coverage preserved.
Adds tests/testUtilities/stubEnv.ts exporting a stubEnv helper that captures the pre-test value of each environment variable and restores it via a global afterEach hook. Replaces hand-written process.env.X = "value" assignments and matching delete process.env.X cleanups across nine spec files. Design notes: - stubEnv takes variadic [name, value] tuples rather than an object literal so that POSIX-style uppercase variable names flow through without needing the camelCase object-key naming convention to be suppressed. - Reflect.deleteProperty is used instead of delete to avoid the dynamic-delete rule trigger; no ESLint suppressions are required anywhere in the helper or its call sites. - Pass undefined as a value to temporarily unset a variable; the original value is restored at afterEach time regardless. - Replaces the existing after() cleanup blocks in tokenManager.spec.ts and azureReposInvoker.spec.ts, and the afterEach cleanup in gitInvoker.spec.ts and gitHubReposInvoker.spec.ts. - 783 tests still pass, 100% coverage preserved, lint clean.
- Introduce a new skill to refresh pinned CI/CD dependencies. - Enforce consistency of Node.js runtime across workflows and pipelines. - Provide guidelines for when to use this skill and the process involved. - Include verification steps and constraints to ensure proper updates.
Splits the four largest spec files into 28 smaller siblings to make refactors cheaper and triage easier: - inputs.spec.ts (1,053 lines) → 8 files per nested describe. - azureReposInvoker.spec.ts (1,848 lines) → 7 files per public method. - gitHubReposInvoker.spec.ts (1,513 lines) → 7 files per public method. - codeMetrics.spec.ts (1,529 lines) → 6 files grouped by topic (defaults, nonDefaults, fileMatching, edgeCases, and the existing getFilesNotRequiringReview/getDeletedFilesNotRequiringReview describes). Shared beforeEach setup is extracted into per-file *TestSetup.ts helpers co-located with the splits. All test cases are preserved verbatim and coverage remains at 100% across statements, branches, functions, and lines. Implements step 4 of TestPlan.md.
Adds a `createSut(...)` helper to each `*TestSetup.ts` file that constructs the SUT from the mocked collaborators. Tests previously inlined the full `new X(instance(a), instance(b), ...)` block at every call site — now each test constructs the SUT on a single line: const sut = createSut(logger, runnerInvoker); Removes ~500 lines of boilerplate across the 22 split spec files while keeping test bodies otherwise unchanged. Unused `instance` imports pruned where no longer referenced. Implements step 6 of TestPlan.md. Step 5 (test data builders) was evaluated and skipped — the targeted constructors all take 3–5 simple positional args, where a builder would add verbosity without improving readability.
Adds `fast-check` property tests for `baseSize`, `growthRate`, `testFactor`, `fileMatchingPatterns`, and `testMatchingPatterns` alongside the existing `codeFileExtensions` coverage in `inputs.property.spec.ts`. The new properties cover the "any value satisfying X → converted result" and "any value failing X → default" invariants that the example-based tests in the per-input spec files can only sample pointwise. Also generalises the local `createInputs` helper to take an `InputOverrides` bag so each property can target a single input field without interfering with the others. Implements step 7 of TestPlan.md. The existing example-based tables stay in place — deletion of interior cases is deferred per the plan's prioritisation.
Extracts two shared fixture arrays into `tests/testUtilities/fixtures/invalidInputs.ts`: - `invalidNumericStrings` — strings that fail to parse as a positive numeric value, used by the base size, growth rate, and test factor specs. Growth rate and test factor spread the fixture and append `"Infinity"` locally. - `invalidPatternStrings` — strings that reduce to empty after whitespace handling, used by the file matching pattern, test matching pattern, and code file extension specs. Prevents the set of "invalid" strings from drifting between the six input specs that shared the same values by copy-paste. Implements step 9 of TestPlan.md.
PR Metrics✔ Thanks for keeping your pull request small.
Metrics computed by PR Metrics. Add it to your Azure DevOps and GitHub PRs! |
Super-linter summary
Super-linter detected linting errors For more information, see the GitHub Actions workflow run Powered by Super-linter BIOME_LINT |
There was a problem hiding this comment.
Pull request overview
Refactors the PR Metrics TypeScript unit tests to reduce brittleness and maintenance cost by centralizing common test scaffolding (localization/env stubs, SUT factories) and expanding property-based testing for input parsing, while keeping coverage intact.
Changes:
- Added shared test helpers for localization (
stubLocalization) and environment variable scoping (stubEnv), plus shared invalid-input fixtures. - Split large specs into smaller, method-focused spec files with shared mock/SUT setup modules.
- Expanded
fast-checkproperty-based tests forInputsparsing invariants.
Reviewed changes
Copilot reviewed 50 out of 50 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/task/tests/utilities/validator.spec.ts | Switches env var setup/cleanup to stubEnv for safer per-test restoration. |
| src/task/tests/testUtilities/stubLocalization.ts | New helper that wires mocked RunnerInvoker.loc() to real resources.resjson. |
| src/task/tests/testUtilities/stubEnv.ts | New helper to set/unset env vars with automatic restoration via afterEach. |
| src/task/tests/testUtilities/fixtures/invalidInputs.ts | New shared fixtures for invalid numeric/pattern input strings. |
| src/task/tests/runners/runnerInvoker.spec.ts | Replaces manual process.env juggling with stubEnv. |
| src/task/tests/repos/tokenManager.spec.ts | Centralizes env setup, removes debug-log coupling, and uses real localization stubbing. |
| src/task/tests/repos/gitHubReposInvokerTestSetup.ts | New shared mock/SUT factory for GitHub repos invoker tests (+ env + localization stubs). |
| src/task/tests/repos/gitHubReposInvoker.updateComment.spec.ts | New focused spec covering updateComment(). |
| src/task/tests/repos/gitHubReposInvoker.setTitleAndDescription.spec.ts | New focused spec covering setTitleAndDescription(). |
| src/task/tests/repos/gitHubReposInvoker.isAccessTokenAvailable.spec.ts | New focused spec covering token availability behavior on GitHub/Azure contexts. |
| src/task/tests/repos/gitHubReposInvoker.getComments.spec.ts | New focused spec for PR + file comment retrieval/mapping. |
| src/task/tests/repos/gitHubReposInvoker.deleteCommentThread.spec.ts | New focused spec covering deletion of review comment threads. |
| src/task/tests/repos/gitHubReposInvoker.createComment.spec.ts | New focused spec covering comment creation paths and error handling. |
| src/task/tests/repos/azureReposInvokerTestSetup.ts | New shared mock/SUT factory for Azure repos invoker tests (+ env + localization stubs). |
| src/task/tests/repos/azureReposInvoker.updateComment.spec.ts | New focused spec covering update thread/comment API behavior and access errors. |
| src/task/tests/repos/azureReposInvoker.setTitleAndDescription.spec.ts | New focused spec covering PR title/description updates and access errors. |
| src/task/tests/repos/azureReposInvoker.isAccessTokenAvailable.spec.ts | New focused spec covering token manager fallback and failure messaging. |
| src/task/tests/repos/azureReposInvoker.getTitleAndDescription.spec.ts | New focused spec for reading PR title/description and env validation branches. |
| src/task/tests/repos/azureReposInvoker.getComments.spec.ts | New focused spec for Azure thread/comment mapping and malformed payload handling. |
| src/task/tests/repos/azureReposInvoker.deleteCommentThread.spec.ts | New focused spec covering delete operations and access errors. |
| src/task/tests/repos/azureReposInvoker.createComment.spec.ts | New focused spec covering thread creation paths and access errors. |
| src/task/tests/pullRequests/pullRequestComments.spec.ts | Uses stubLocalization and removes debug log assertions coupling tests to tracing. |
| src/task/tests/pullRequests/pullRequest.spec.ts | Uses stubEnv/stubLocalization and removes debug log assertions. |
| src/task/tests/metrics/inputsTestSetup.ts | New shared mocks/SUT factory for Inputs initialization specs. |
| src/task/tests/metrics/inputs.testMatchingPatterns.spec.ts | New focused spec for testMatchingPatterns parsing using shared fixtures. |
| src/task/tests/metrics/inputs.testFactor.spec.ts | New focused spec for testFactor parsing using shared fixtures. |
| src/task/tests/metrics/inputs.property.spec.ts | Expands property-based coverage across more Inputs fields and invariants. |
| src/task/tests/metrics/inputs.growthRate.spec.ts | New focused spec for growthRate parsing using shared fixtures. |
| src/task/tests/metrics/inputs.fileMatchingPatterns.spec.ts | New focused spec for fileMatchingPatterns parsing and truncation behavior. |
| src/task/tests/metrics/inputs.codeFileExtensions.spec.ts | New focused spec for codeFileExtensions parsing/normalization. |
| src/task/tests/metrics/inputs.baseSize.spec.ts | New focused spec for baseSize parsing. |
| src/task/tests/metrics/inputs.alwaysCloseComment.spec.ts | New focused spec for alwaysCloseComment parsing. |
| src/task/tests/metrics/inputs.allInputs.spec.ts | New spec covering “all defaults” and “all specified” initialization behavior. |
| src/task/tests/metrics/codeMetricsTestSetup.ts | New shared mocks/SUT factory for CodeMetrics tests (with localization stubbing). |
| src/task/tests/metrics/codeMetricsCalculator.spec.ts | Uses stubEnv/stubLocalization and removes debug log assertions. |
| src/task/tests/metrics/codeMetrics.getFilesNotRequiringReview.spec.ts | New focused spec for parsing/validation of diff summary and file extraction. |
| src/task/tests/metrics/codeMetrics.getDeletedFilesNotRequiringReview.spec.ts | New focused spec for deleted-file extraction error paths. |
| src/task/tests/metrics/codeMetrics.fileMatching.spec.ts | New focused spec for file include/exclude glob behavior and impacts on metrics. |
| src/task/tests/metrics/codeMetrics.edgeCases.spec.ts | New focused spec for edge conditions (e.g., disabled test factor, large multipliers). |
| src/task/tests/metrics/codeMetrics.defaults.spec.ts | New table-driven spec covering many default-input diff scenarios. |
| src/task/tests/git/octokitGitDiffParser.spec.ts | Removes debug log assertions to reduce coupling to internal tracing. |
| .github/skills/update-ci-dependencies/SKILL.md | Adds a repository “skill” doc describing how to refresh CI dependency pins consistently. |
Adds the @typescript-eslint/consistent-type-imports rule and applies its auto-fix across the source and spec files, so imports used only in type positions use the dedicated `import type` syntax.
Super-linter summary
Super-linter detected linting errors For more information, see the GitHub Actions workflow run Powered by Super-linter BIOME_LINT |
Converts the inline `import { type GitPullRequest }` to
`import type { GitPullRequest }` for consistency with surrounding
imports. ESLint's consistent-type-imports rule accepts both forms, so
this was a manual cleanup.
Super-linter summary
All files and directories linted successfully For more information, see the GitHub Actions workflow run Powered by Super-linter |
Super-linter summary
All files and directories linted successfully For more information, see the GitHub Actions workflow run Powered by Super-linter |
The `getIssueCommentsResponse` constant is a shared object. Assigning `body` on `response.data[0]` previously mutated the constant itself, leaving hidden coupling between tests in the same file (and any future suite that touches the constant). `structuredClone` makes each test work on an independent copy so mutations never escape.
Super-linter summary
All files and directories linted successfully For more information, see the GitHub Actions workflow run Powered by Super-linter |
- Update test files to use localized strings for logging assertions. - Refactor localization stubbing to utilize a new localize function. - Enhance clarity and maintainability of test code by reducing duplication.
Super-linter summary
All files and directories linted successfully For more information, see the GitHub Actions workflow run Powered by Super-linter |
- Add cleanup step for debug and release directories before copying source files. - Ensure test:fast command also cleans up debug directory before execution.
Super-linter summary
All files and directories linted successfully For more information, see the GitHub Actions workflow run Powered by Super-linter |
Replace hard-coded English strings in test assertions with localize() lookups against resources.resjson. Covers pullRequestMetrics.succeeded, repos.azureReposInvoker.noAzureReposAccessToken, repos.gitHubReposInvoker.noGitHubAccessToken, and repos.tokenManager.incorrectAuthorizationScheme.
Super-linter summary
All files and directories linted successfully For more information, see the GitHub Actions workflow run Powered by Super-linter |
Super-linter summary
All files and directories linted successfully For more information, see the GitHub Actions workflow run Powered by Super-linter |
The test stubs thenResolve(null), so rename the case from "returns undefined" to "returns null" to match the actual behaviour.
Super-linter summary
All files and directories linted successfully For more information, see the GitHub Actions workflow run Powered by Super-linter |
Reconcile main's userAgent relocation to constants.ts with our local test-split refactor: drop the local export from gitHubReposInvoker.ts, re-source userAgent in gitHubReposInvokerTestSetup.ts, and update Update-Version.ps1 to track the new path. Port the dotfile and non-parseable URL coverage from main's monolithic specs into the matching split files. Regenerate dist/index.mjs.
Super-linter summary
Super-linter detected linting errors For more information, see the GitHub Actions workflow run Powered by Super-linter GIT_COMMITLINT |
Super-linter summary
All files and directories linted successfully For more information, see the GitHub Actions workflow run Powered by Super-linter |
Summary
The unit test suite is refactored to reduce maintenance cost without reducing coverage. The changes target long-standing pain points where refactors cascade into large test-edit diffs, and replace exhaustive sample-based checks with property-based coverage.
Changes
beforeEachblocks that re-implemented the resource file as mock stubs.process.envjuggling.new X(instance(...), ...)boilerplate to a single line per test.fast-checkproperties back up the example-based tables for input parsing so invariants are verified over ranges rather than hand-picked values.Impact