From b7a7f8d17e273c8481854b4117e0c78f3ed604b8 Mon Sep 17 00:00:00 2001 From: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com> Date: Tue, 27 Oct 2026 08:02:03 -0400 Subject: [PATCH 1/3] test: make the unit suite order-independent under vitest --sequence.shuffle `npx vitest run --sequence.shuffle` fails 11 tests in 8 files on main under 4 of 12 seeds tried; the default order passes only because each leaking test happens to run before a neighbour that would not notice. Three mechanisms, all within a file: - a shared imported JSON fixture mutated and never restored: assign, unassign and handleIssueComment rewrite `comment.body`; the cron lgtm test pushed a `hold` label onto the list every other test is served. Clone the fixture or reset the field in beforeEach. - loadProwConfig probes the organization and repository tiers under Promise.all, so when the tier a test exercises rejects, the other tier's sequential probe chain keeps issuing requests after the test returned, and the next test's unhandled-request guard or request:start listener records them. Waiting for the in-flight count to reach zero is satisfied in the gap between two probes. `utils.configFoundAt` serves an empty configuration at the other tier's first probe, so the only sibling request is already in flight when the rejection lands. - module memos not reset: auth.test.ts's outer describes never reset the configuration cache and inherited a `trusted` policy; owners.test.ts's expandOwnersSet describe never reset the group memo; mergeQueueGuards imported @actions/core fresh after a sibling's vi.resetModules while still calling the statically imported queueState, so the assertion spied on a different core instance than the one the module warns through. Adds `npm run test:shuffle` and a "Test order" section to docs/contributing.md describing the three patterns. 18 seeds pass after the change; the bundle suite was already order-independent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com> --- __tests__/cronJobTest/labelSync.test.ts | 3 +- __tests__/cronJobTest/lgtm.test.ts | 7 ++-- __tests__/issueCommentTest/assign.test.ts | 2 ++ .../handleIssueComment.test.ts | 2 ++ __tests__/issueCommentTest/unassign.test.ts | 2 ++ __tests__/testUtils.ts | 21 ++++++++++++ __tests__/utils/auth.test.ts | 18 ++++++----- __tests__/utils/mergeQueueGuards.test.ts | 7 ++-- __tests__/utils/owners.test.ts | 7 ++-- __tests__/utils/utilsErrorPaths.test.ts | 10 +++--- docs/contributing.md | 32 +++++++++++++++++++ package.json | 1 + 12 files changed, 90 insertions(+), 22 deletions(-) diff --git a/__tests__/cronJobTest/labelSync.test.ts b/__tests__/cronJobTest/labelSync.test.ts index 5508e64c..e4b3ef74 100644 --- a/__tests__/cronJobTest/labelSync.test.ts +++ b/__tests__/cronJobTest/labelSync.test.ts @@ -289,7 +289,8 @@ describe('label-sync job', () => { it('fails the run when the configuration cannot be loaded', async () => { server.use( http.get(`${repo}/contents/.github%2Fprow.yaml`, utils.mockResponse(500, { message: 'boom' })), - ...utils.noOrgOrRepoConfigExcept('.github/prow.yaml'), + // end the organization tier on its first probe: its chain would otherwise outlive this test (see configFoundAt) + ...utils.configFoundAt('Codertocat/.project:prow.yaml', '.github/prow.yaml'), ) const writes = recordWrites([]) const setFailed = vi.spyOn(core, 'setFailed').mockImplementation(() => {}) diff --git a/__tests__/cronJobTest/lgtm.test.ts b/__tests__/cronJobTest/lgtm.test.ts index aafa346d..e0d41178 100644 --- a/__tests__/cronJobTest/lgtm.test.ts +++ b/__tests__/cronJobTest/lgtm.test.ts @@ -179,8 +179,10 @@ describe('cronLgtm', () => { // Instead, we use it to gain the repo owner and url const context = new utils.MockContext(pullReqOpenedEvent) - listPullReqs[0].labels[0].name = 'lgtm' - listPullReqs[0].labels.push({ + // a copy: the shared fixture is what the default handlers above serve to every other test + const heldPrs = structuredClone(listPullReqs) + heldPrs[0].labels[0].name = 'lgtm' + heldPrs[0].labels.push({ id: 1, node_id: '123', url: 'https://api.github.com/repos/octocat/Hello-World/labels/hold', @@ -189,6 +191,7 @@ describe('cronLgtm', () => { color: 'f29513', default: true, }) + routePulls(heldPrs) const observeReq = new utils.ObserveRequest() server.use( diff --git a/__tests__/issueCommentTest/assign.test.ts b/__tests__/issueCommentTest/assign.test.ts index 9d1b7e2a..57148d0e 100644 --- a/__tests__/issueCommentTest/assign.test.ts +++ b/__tests__/issueCommentTest/assign.test.ts @@ -21,6 +21,8 @@ afterAll(() => server.close()) describe('/assign', () => { beforeEach(() => { utils.setupActionsEnv('/assign') + // the tests below rewrite the shared fixture's comment; start each from the file's own body + issueCommentEventAssign.comment.body = '/assign' }) it('handles self assigning with comment /assign', async () => { diff --git a/__tests__/issueCommentTest/handleIssueComment.test.ts b/__tests__/issueCommentTest/handleIssueComment.test.ts index 371c39f4..f0d4592d 100644 --- a/__tests__/issueCommentTest/handleIssueComment.test.ts +++ b/__tests__/issueCommentTest/handleIssueComment.test.ts @@ -28,6 +28,8 @@ beforeAll(() => server.listen(utils.failOnUnhandledRequest)) // the dispatch tests below mock every command; the sweep that follows a command is stubbed the same way let sweepStubs: ReturnType[] beforeEach(() => { + // the dispatch tests rewrite the shared fixture's comment; start each from the file's own body + issueCommentEvent.comment.body = 'Meow meow!' sweepStubs = [ vi.spyOn(requireMatchingLabel, 'checkRequiredLabels').mockResolvedValue(), vi.spyOn(tide, 'tideOnComment').mockResolvedValue(), diff --git a/__tests__/issueCommentTest/unassign.test.ts b/__tests__/issueCommentTest/unassign.test.ts index 545ff1bd..fc91530d 100644 --- a/__tests__/issueCommentTest/unassign.test.ts +++ b/__tests__/issueCommentTest/unassign.test.ts @@ -21,6 +21,8 @@ afterAll(() => server.close()) describe('/unassign', () => { beforeEach(() => { utils.setupActionsEnv('/unassign') + // the tests below rewrite the shared fixture's comment; start each from the file's own body + issueCommentEventUnassign.comment.body = '/unassign' }) it('handles self unassignment with comment /unassign', async () => { diff --git a/__tests__/testUtils.ts b/__tests__/testUtils.ts index 8ad2ad96..7fa5e266 100644 --- a/__tests__/testUtils.ts +++ b/__tests__/testUtils.ts @@ -1,5 +1,6 @@ import type { SetupServerApi } from 'msw/node' import type { Context } from '../src/utils/context' +import { Buffer } from 'node:buffer' import * as github from '@actions/github' import { http } from 'msw' import { afterEach, beforeEach, expect } from 'vitest' @@ -109,6 +110,26 @@ export function noOrgOrRepoConfigExcept(...except: string[]) { .map(source => http.get(contentsUrl(source), mockResponse(404, { message: 'Not Found' }))) } +/** + * configFoundAt serves an empty configuration file at the given probe and 404 + * at every other one, so the tier that owns it ends with that single request. + * + * loadProwConfig probes the organization and repository tiers at once; when the + * tier a test is exercising rejects, the other tier's chain of probes keeps + * going after the test's await returns, and a probe started between tests is + * reported against whichever test runs next. Ending the other tier on its first + * probe leaves only a request that is already in flight when the rejection lands. + * + * @param source - `owner/repo:path` or a repo-relative path of the probe that exists + * @param others - further probes the caller serves itself + */ +export function configFoundAt(source: string, ...others: string[]) { + return [ + http.get(contentsUrl(source), mockResponse(200, { type: 'file', encoding: 'base64', content: Buffer.from('{}\n').toString('base64') })), + ...noOrgOrRepoConfigExcept(source, ...others), + ] +} + /** * repoHasLabels serves `GET /repos/Codertocat/Hello-World/labels` with the * given labels on a single page (any `per_page`/`page` query is ignored), so diff --git a/__tests__/utils/auth.test.ts b/__tests__/utils/auth.test.ts index ebdc7ce0..97deb0c7 100644 --- a/__tests__/utils/auth.test.ts +++ b/__tests__/utils/auth.test.ts @@ -29,6 +29,8 @@ beforeAll(() => { utils.setupActionsEnv() server.listen(utils.failOnUnhandledRequest) }) +// the configuration is memoized per repository: a test that loaded one must not hand it to the next +beforeEach(() => resetProwConfigCache()) afterEach(() => server.resetHandlers()) afterAll(() => server.close()) @@ -376,7 +378,6 @@ describe('authorization policies', () => { } beforeEach(() => { - resetProwConfigCache() calls = [] server.events.on('request:start', recordCall) server.events.on('request:end', endCall) @@ -513,16 +514,16 @@ describe('authorization policies', () => { }) it.each([ - ['a failed read', [http.get(utils.contentsUrl('.github/prow.yaml'), utils.mockResponse(500, { message: 'boom' })), ...utils.noOrgOrRepoConfigExcept('.github/prow.yaml')]], - ['malformed yaml', prowYaml('authorization:\n close: everyone\n')], - ])('refuses with a warning, and no further call, when the configuration cannot be loaded: %s', async (_, handlers) => { + ['a failed read', http.get(utils.contentsUrl('.github/prow.yaml'), utils.mockResponse(500, { message: 'boom' }))], + ['malformed yaml', prowYaml('authorization:\n close: everyone\n')[0]], + ])('refuses with a warning, and no further call, when the configuration cannot be loaded: %s', async (_, repoFile) => { const warning = vi.spyOn(core, 'warning').mockImplementation(() => {}) - server.use(...handlers) + // the organization tier is probed alongside the failing repository tier; end it on its first probe so + // the only sibling request is in flight, and drained by afterEach, when the rejection lands + server.use(repoFile, ...utils.configFoundAt('Codertocat/.project:prow.yaml', '.github/prow.yaml')) await expect(closePolicyAllows(octokit, context, 'Alice')).resolves.toBe(false) expect(warning).toHaveBeenCalledWith(expect.stringContaining('authorization: could not load prow config: ')) - // the organization tier keeps probing after the repository tier failed; let it finish inside this test - await vi.waitFor(() => expect(calls).toContain('GET /repos/Codertocat/.github/contents/prow.yaml')) expect(calls.filter(call => !call.includes('/contents/'))).toEqual([]) }) @@ -583,7 +584,8 @@ describe('authorization policies', () => { it('falls back to the members message, with a warning, when the configuration cannot be loaded', async () => { const warning = vi.spyOn(core, 'warning').mockImplementation(() => {}) - server.use(rootOwners(), ...membership(), ...prowYaml('authorization:\n review: anyone\n users: [alice]\n')) + // end the organization tier on its first probe: its chain would otherwise outlive this test (see configFoundAt) + server.use(rootOwners(), ...membership(), prowYaml('authorization:\n review: anyone\n users: [alice]\n')[0], ...utils.configFoundAt('Codertocat/.project:prow.yaml', '.github/prow.yaml')) await expect(assertAuthorizedByOwnersOrMembership(octokit, context, 'reviewers', 'Alice')) .rejects diff --git a/__tests__/utils/mergeQueueGuards.test.ts b/__tests__/utils/mergeQueueGuards.test.ts index 2b99609c..1b93b273 100644 --- a/__tests__/utils/mergeQueueGuards.test.ts +++ b/__tests__/utils/mergeQueueGuards.test.ts @@ -2,7 +2,7 @@ import type { Octokit } from '@octokit/rest' import { afterEach, describe, expect, it, vi } from 'vitest' -import { enqueue, enqueuedByBot, queueState } from '../../src/utils/mergeQueue' +import { enqueue, enqueuedByBot } from '../../src/utils/mergeQueue' import pullReqOpenedEvent from '../fixtures/pullReq/pullReqOpenedEvent.json' import * as utils from '../testUtils' @@ -23,9 +23,10 @@ afterEach(() => vi.resetModules()) describe('errorMessage', () => { it('reports a null rejection of the state query as "null" instead of throwing', async () => { - const core = await import('@actions/core') + // both fresh: after a sibling's resetModules the static queueState would warn through another core instance + const [core, fresh] = await Promise.all([import('@actions/core'), import('../../src/utils/mergeQueue')]) - await expect(queueState(rejectingWith(null), context, 7)).resolves.toBeUndefined() + await expect(fresh.queueState(rejectingWith(null), context, 7)).resolves.toBeUndefined() expect(core.warning).toHaveBeenCalledWith('could not read the merge queue state of pr #7; falling back to a direct merge: null') }) diff --git a/__tests__/utils/owners.test.ts b/__tests__/utils/owners.test.ts index 35018d06..ea525c7d 100644 --- a/__tests__/utils/owners.test.ts +++ b/__tests__/utils/owners.test.ts @@ -12,6 +12,9 @@ import { resetOwnersCaches, } from '../../src/utils/owners' +// the group, tree and OWNERS-presence memos live on the module: no describe may inherit another's +beforeEach(() => resetOwnersCaches()) + describe('parseOwners', () => { it('reads approvers and reviewers', () => { const owners = parseOwners('OWNERS', 'approvers:\n- alice\nreviewers:\n- bob\n- carol\n') @@ -231,8 +234,6 @@ describe('expandGroups', () => { expect([...result].sort()).toEqual(['alice', 'bob', 'carol']) }) - beforeEach(() => resetOwnersCaches()) - it('ignores groups of another org and groups it cannot read', async () => { const result = await expandGroups(client({}), context, ['alice', '@other/team', '@myorg/missing']) @@ -311,8 +312,6 @@ describe('ownersInclude', () => { } } - beforeEach(() => resetOwnersCaches()) - it('answers a direct listing without reading any team', async () => { const octokit = client(['bob']) diff --git a/__tests__/utils/utilsErrorPaths.test.ts b/__tests__/utils/utilsErrorPaths.test.ts index d6128303..5faf40da 100644 --- a/__tests__/utils/utilsErrorPaths.test.ts +++ b/__tests__/utils/utilsErrorPaths.test.ts @@ -115,12 +115,14 @@ describe('utils error paths', () => { describe('loadProwConfig explicit sources', () => { const explicitUrl = `${utils.api}/repos/cncf/prow-config/contents/prow.yaml` + // the repository tier is probed alongside the explicit source; every rejecting case below ends it on its + // first probe so that no probe of its chain starts after the test has returned (see configFoundAt) it('wraps a non-404 failure reading the explicit repo file', async () => { process.env.INPUT_CONFIG = 'cncf/prow-config:prow.yaml' server.use( http.get(explicitUrl, utils.mockResponse(500, { message: 'boom' })), - ...utils.noOrgOrRepoConfigExcept(), + ...utils.configFoundAt('.github/prow.yaml'), ) await expect(loadProwConfig(octokit, context)).rejects.toThrow( @@ -132,7 +134,7 @@ describe('utils error paths', () => { process.env.INPUT_CONFIG = 'cncf/prow-config:prow.yaml' server.use( http.get(explicitUrl, utils.mockResponse(200, [{ ...labelFileContents, name: 'a.yaml' }])), - ...utils.noOrgOrRepoConfigExcept(), + ...utils.configFoundAt('.github/prow.yaml'), ) await expect(loadProwConfig(octokit, context)).rejects.toThrow( @@ -144,7 +146,7 @@ describe('utils error paths', () => { process.env.INPUT_CONFIG = 'cncf/prow-config:prow.yaml' server.use( http.get(explicitUrl, utils.mockResponse(200, { ...labelFileContents, content: undefined, encoding: 'base64' })), - ...utils.noOrgOrRepoConfigExcept(), + ...utils.configFoundAt('.github/prow.yaml'), ) await expect(loadProwConfig(octokit, context)).rejects.toThrow('prow.yaml is not a file') @@ -154,7 +156,7 @@ describe('utils error paths', () => { process.env.INPUT_CONFIG = 'https://config.example.com/prow.yaml' server.use( http.get('https://config.example.com/prow.yaml', () => HttpResponse.error()), - ...utils.noOrgOrRepoConfigExcept(), + ...utils.configFoundAt('.github/prow.yaml'), ) await expect(loadProwConfig(octokit, context)).rejects.toThrow( diff --git a/docs/contributing.md b/docs/contributing.md index 5f1ccc83..9fe98ed9 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -47,6 +47,7 @@ stay in step with `action.yml` and the label catalogue. | `npm test` | The whole Vitest suite | | `npm run test:coverage` | The suite with v8 coverage; CI enforces the thresholds in `vitest.config.mjs` (lines 99, branches 99, functions 99, statements 99, a floor set just under what `main` reaches) | | `npx vitest run __tests__/bundle` | Only the bundle acceptance harness | +| `npm run test:shuffle` | The whole suite with the tests of every file in a random order (`--sequence.shuffle`; pass `--sequence.seed=` to repeat an order); see "Test order" below | | `npm run test:coverage:e2e` | Only the bundle acceptance harness, run against a source-mapped bundle built from `__tests__/bundle/coverageEntry/` into `.coverage-bundle/` so its hits are reported against `src/`, written to `coverage-e2e/` so it never overwrites the unit run's `coverage/` (ncc's map is line-only, so the build also emits tsc's per-file maps into `.coverage-bundle/tsc/` and `coverageEntry/remap.mjs` composes their columns into it; without that, inner lines of a multi-line expression that tsc flattens onto one JS line report 0 hits). This is end-to-end-only coverage and is deliberately reported separately from `test:coverage`: the unit run already reaches every line, so a union would say nothing about what the bundle exercises, and the two runs' statement and branch maps come from different transforms (vite vs. tsc+webpack) and do not combine. Fails below 99 % lines / 98 % branches / 99 % functions / 99 % statements (`vitest.e2e-coverage.config.mjs`), a floor set just under what `main` reaches so a scenario that stops driving a `src/` path is caught | Unit tests under `__tests__/` import `src/` directly and mock the GitHub API @@ -98,6 +99,37 @@ export async function gate(a: boolean, b: boolean): Promise { In the same reproduction an explicit `else { … }` block, which owns a V8 range of its own, is counted correctly. +### Test order + +Vitest runs the tests of a file in source order, so a test that leaves state +behind is only noticed by the tests that happen to follow it. `npm run +test:shuffle` runs every file's tests in a random order (the seed is printed +with the failure; `--sequence.seed=` repeats it). The three ways a test has +leaked into its neighbour here, and how each file now avoids them: + +- **A mutated fixture.** The JSON fixtures are imported once per file, so + `event.comment.body = '/assign @x'` is seen by every later test of the file; + the test that relied on the fixture's own body then dispatched a different + command. Either `structuredClone` the fixture before changing it (the cron + tests' `lgtmPr`) or reset the field in a `beforeEach`. +- **A probe chain outliving its test.** `loadProwConfig` probes the + organization and repository tiers under `Promise.all`; when the tier a test + exercises rejects, the other tier's probes keep going after the test's + `await` has returned, and a probe that starts between two tests is reported — + by `failTestOnUnhandledRequests`, or by a `request:start` listener — against + the next one. Waiting for the in-flight count to reach zero does not help, + because the chain is idle between two of its probes. `utils.configFoundAt` + serves an empty configuration at the other tier's first probe, so the only + sibling request is already in flight when the rejection lands. +- **A memo on a module instance.** `loadProwConfig` is memoized per + repository and `queueState` warns once per process; a test that does not + reset them inherits what the previous test loaded. `setupActionsEnv` / + `setupJobsEnv` reset every cache; a file that calls neither (or only in + `beforeAll`) needs `resetProwConfigCache()` in a `beforeEach`, and a file + that uses `vi.resetModules()` must import `@actions/core` and the module under + test fresh together, or the assertion spies on a different `core` instance + than the one the module warns through. + ### Mutation checks Line coverage says a test *reached* a statement, not that it would notice the diff --git a/package.json b/package.json index fe26c5e1..aee0c1cd 100644 --- a/package.json +++ b/package.json @@ -26,6 +26,7 @@ "test:coverage": "vitest run --coverage", "pack:coverage": "ncc build __tests__/bundle/coverageEntry/main.ts -o .coverage-bundle --source-map --no-source-map-register && tsc -p __tests__/bundle/coverageEntry/tsconfig.json --outDir .coverage-bundle/tsc && node __tests__/bundle/coverageEntry/remap.mjs", "test:coverage:e2e": "npm run pack:coverage && vitest run --coverage --config vitest.e2e-coverage.config.mjs __tests__/bundle", + "test:shuffle": "vitest run --sequence.shuffle", "test:mutation": "stryker run", "test:watch": "vitest", "all": "npm run build && npm run lint:fix && npm run lint && npm run pack && npm test" From e2f71dfe5dbe7a7d8a8718cdd8596ae8216fbf74 Mon Sep 17 00:00:00 2001 From: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com> Date: Tue, 27 Oct 2026 15:05:15 -0400 Subject: [PATCH 2/3] test(typecheck): type-check __tests__ with tsc -p tsconfig.test.json and fix the 29 errors it finds tsconfig.json excludes __tests__/, and Vitest's esbuild transform strips types without checking them, so nothing in the repository type-checks the test suite. Running tsc over src/ and __tests__/ together reports 29 errors in 17 files, several of them fixtures that drifted from the production types they claim to be: - PullRequestOwners gained headSha; the pullOwners() fixture in approve.test.ts, approveErrorPaths.test.ts and approveGuards.test.ts still builds the old shape - ProwConfig gained approve, lgtm, sweep and authorization; the hand-built configs in blunderbuss.test.ts and requireMatchingLabel.test.ts still carry only the original six keys (now built from mergeProwConfig) - msw 3 renamed SetupServerApi to SetupServer (testUtils.ts) - missingNumberMessage.test.ts imports @actions/github/lib/context, which docs/contributing.md forbids; it now imports src/utils/context - lgtmBinding.test.ts builds a sender without the type field the payload type requires - the remainder are untyped spy declarations (tide.test.ts, tideGuards.test.ts), an untyped it.each table (ownersGuards.test.ts), an unresolved this (fakeGithub.ts), a delete on a required field (sweepArms.test.ts), an over-narrow createLabel parameter (labelSync.test.ts), a milestone type intersection that can never be null (milestone.test.ts), a spyOn over a module/name union (handleIssueComment.test.ts) and index-signature mismatches (configurationDocs.test.ts) Adds tsconfig.test.json (extends tsconfig.json, noEmit, includes src/ and __tests__/, excludes the coverageEntry build which has its own tsconfig) and an npm run typecheck script that runs it. npm run build is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: quality <280983584+hivecommons-hive[bot]@users.noreply.github.com> --- __tests__/bundle/fakeGithub.ts | 5 +++-- __tests__/bundle/sweepArms.test.ts | 2 +- __tests__/configurationDocs.test.ts | 10 +++++----- __tests__/cronJobTest/labelSync.test.ts | 2 +- __tests__/issueCommentTest/handleIssueComment.test.ts | 3 ++- __tests__/issueCommentTest/milestone.test.ts | 2 +- __tests__/missingNumberMessage.test.ts | 2 +- __tests__/plugins/approve.test.ts | 1 + __tests__/plugins/approveErrorPaths.test.ts | 1 + __tests__/plugins/approveGuards.test.ts | 1 + __tests__/plugins/blunderbuss.test.ts | 7 ++++--- __tests__/plugins/lgtmBinding.test.ts | 4 ++-- __tests__/plugins/requireMatchingLabel.test.ts | 3 ++- __tests__/plugins/tide.test.ts | 3 ++- __tests__/plugins/tideGuards.test.ts | 3 ++- __tests__/testUtils.ts | 4 ++-- __tests__/utils/ownersGuards.test.ts | 2 +- docs/contributing.md | 6 ++++++ package.json | 1 + tsconfig.test.json | 9 +++++++++ 20 files changed, 48 insertions(+), 23 deletions(-) create mode 100644 tsconfig.test.json diff --git a/__tests__/bundle/fakeGithub.ts b/__tests__/bundle/fakeGithub.ts index 93dd6406..7ff48b7e 100644 --- a/__tests__/bundle/fakeGithub.ts +++ b/__tests__/bundle/fakeGithub.ts @@ -143,7 +143,7 @@ export async function start(): Promise { await new Promise(resolve => server.listen(0, '127.0.0.1', resolve)) const { port } = server.address() as AddressInfo - return { + const github: FakeGithub = { url: `http://127.0.0.1:${port}`, requests, unmatched, @@ -154,7 +154,7 @@ export async function start(): Promise { return unmatched.filter(r => !allowances.some(a => matches(a, r))) }, assertNoUnexpectedRequests() { - const unexpected = this.unexpectedRequests() + const unexpected = github.unexpectedRequests() if (unexpected.length > 0) { const listing = unexpected.map(r => ` ${r.method} ${r.path}`).join('\n') throw new Error(`the fake GitHub API answered ${unexpected.length} request(s) no route or allowance covers:\n${listing}`) @@ -197,6 +197,7 @@ export async function start(): Promise { }) }, } + return github } function mergeQueueRoute(queue: MergeQueueFixture): Route { diff --git a/__tests__/bundle/sweepArms.test.ts b/__tests__/bundle/sweepArms.test.ts index 7bdf0698..24f94b36 100644 --- a/__tests__/bundle/sweepArms.test.ts +++ b/__tests__/bundle/sweepArms.test.ts @@ -157,7 +157,7 @@ describe('dist/index.js schedule sweep job arms', () => { // requested_reviewers is optional on the pulls listing item (sweep.ts:153 `pr.requested_reviewers ?? []`): a fresh // pull request listed without it has no requested reviewers, so blunderbuss reads the one review and asks for two it('a fresh pull request listed without requested_reviewers: the one review is read and two reviewers are requested', async () => { - const pr = forkPr() + const pr: Partial> = forkPr() delete pr.requested_reviewers routeSweep(pr) diff --git a/__tests__/configurationDocs.test.ts b/__tests__/configurationDocs.test.ts index a7dbdb83..ba5aefd1 100644 --- a/__tests__/configurationDocs.test.ts +++ b/__tests__/configurationDocs.test.ts @@ -72,11 +72,11 @@ const example = parseProwConfig(docPath, yamlBlock('`prow.yaml`')) /** what the code resolves for a section nobody configured, keyed like the document headings */ const resolvedDefaults: Record> = { - tide: resolveTide({}, ''), + tide: { ...resolveTide({}, '') }, hold: { label: resolveHoldLabel({}) }, - blunderbuss: blunderbussSettings(empty), - approve: approveSettings(empty), - lgtm: lgtmSettings(empty), + blunderbuss: { ...blunderbussSettings(empty) }, + approve: { ...approveSettings(empty) }, + lgtm: { ...lgtmSettings(empty) }, sweep: { lookback: defaultSweepLookback }, authorization: { ...resolveAuthorization({}) }, } @@ -89,7 +89,7 @@ describe(`${docPath} prow.yaml example`, () => { }) it.each(['tide', 'hold', 'approve', 'lgtm', 'sweep', 'authorization'])('spells out the code defaults of %s, as its comment claims', (key) => { - expect(example[key as keyof typeof resolvedDefaults]).toEqual(resolvedDefaults[key]) + expect(example[key as keyof ProwConfig]).toEqual(resolvedDefaults[key]) }) it('names the tide.labels default of a repository with OWNERS files', () => { diff --git a/__tests__/cronJobTest/labelSync.test.ts b/__tests__/cronJobTest/labelSync.test.ts index 5508e64c..f716488d 100644 --- a/__tests__/cronJobTest/labelSync.test.ts +++ b/__tests__/cronJobTest/labelSync.test.ts @@ -318,7 +318,7 @@ describe('label-sync job', () => { const real = new Octokit({ auth: token, baseUrl: utils.api }) const refused = vi.fn().mockRejectedValue('offline') const issues = Object.create(real.issues, { - createLabel: { value: (params: { name: string }) => (params.name === 'kind/bug' ? refused() : real.issues.createLabel(params)) }, + createLabel: { value: (params: NonNullable[0]>) => (params.name === 'kind/bug' ? refused() : real.issues.createLabel(params)) }, }) return Object.create(real, { issues: { value: issues } }) }) diff --git a/__tests__/issueCommentTest/handleIssueComment.test.ts b/__tests__/issueCommentTest/handleIssueComment.test.ts index 371c39f4..a107f2a7 100644 --- a/__tests__/issueCommentTest/handleIssueComment.test.ts +++ b/__tests__/issueCommentTest/handleIssueComment.test.ts @@ -405,7 +405,8 @@ it.each([ ] as const)('listing only %s also enables %s', async (alias, base, mod, fn) => { utils.setupActionsEnv(alias) - const spy = vi.spyOn(mod, fn).mockImplementation(() => Promise.resolve()) + // the three modules export differently named handlers of one shape; the table pairs each with its own name + const spy = vi.spyOn(mod as Record Promise>, fn).mockImplementation(() => Promise.resolve()) const setFailed = vi.spyOn(core, 'setFailed').mockImplementation(() => {}) issueCommentEvent.comment.body = base diff --git a/__tests__/issueCommentTest/milestone.test.ts b/__tests__/issueCommentTest/milestone.test.ts index 3c610f15..1d230ef1 100644 --- a/__tests__/issueCommentTest/milestone.test.ts +++ b/__tests__/issueCommentTest/milestone.test.ts @@ -198,7 +198,7 @@ describe('/milestone', () => { }) it('still sends the clear when the issue has no milestone', async () => { - const payload = structuredClone(issueCommentEvent) as typeof issueCommentEvent & { issue: { milestone: unknown } } + const payload = structuredClone(issueCommentEvent) as Omit & { issue: Omit & { milestone: unknown } } payload.comment.body = '/milestone clear' payload.issue.milestone = null diff --git a/__tests__/missingNumberMessage.test.ts b/__tests__/missingNumberMessage.test.ts index 7ac8dbcd..1ec2cf56 100644 --- a/__tests__/missingNumberMessage.test.ts +++ b/__tests__/missingNumberMessage.test.ts @@ -1,4 +1,4 @@ -import type { Context } from '@actions/github/lib/context' +import type { Context } from '../src/utils/context' import { setupServer } from 'msw/node' import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it } from 'vitest' diff --git a/__tests__/plugins/approve.test.ts b/__tests__/plugins/approve.test.ts index a891fdb1..bdbf5554 100644 --- a/__tests__/plugins/approve.test.ts +++ b/__tests__/plugins/approve.test.ts @@ -16,6 +16,7 @@ function pullOwners(ownersFiles: Record, files: string[], author return { number: 1, baseSha: 'basesha', + headSha: 'headsha', author, draft: false, open: true, diff --git a/__tests__/plugins/approveErrorPaths.test.ts b/__tests__/plugins/approveErrorPaths.test.ts index a6caf75e..d9743e6f 100644 --- a/__tests__/plugins/approveErrorPaths.test.ts +++ b/__tests__/plugins/approveErrorPaths.test.ts @@ -28,6 +28,7 @@ function pullOwners(ownersFiles: Record, files: string[], author return { number: 1, baseSha: 'basesha', + headSha: 'headsha', author, draft: false, open: true, diff --git a/__tests__/plugins/approveGuards.test.ts b/__tests__/plugins/approveGuards.test.ts index 59050605..39fcd8cc 100644 --- a/__tests__/plugins/approveGuards.test.ts +++ b/__tests__/plugins/approveGuards.test.ts @@ -46,6 +46,7 @@ function pullOwners(ownersFiles: Record, files: string[], author return { number: 1, baseSha: 'basesha', + headSha: 'headsha', author, draft: false, open: true, diff --git a/__tests__/plugins/blunderbuss.test.ts b/__tests__/plugins/blunderbuss.test.ts index 5f9b4dce..4db52de3 100644 --- a/__tests__/plugins/blunderbuss.test.ts +++ b/__tests__/plugins/blunderbuss.test.ts @@ -8,6 +8,7 @@ import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } import { handleIssueComment } from '../../src/issueComment/handleIssueComment' import { autoCc, blunderbuss, blunderbussSettings, pickReviewers } from '../../src/plugins/blunderbuss' import { handlePullReq, pullRequestHandlers } from '../../src/pullReq/handlePullReq' +import { mergeProwConfig } from '../../src/utils/config' import issueCommentEvent from '../fixtures/issues/issueCommentEvent.json' import labelFileContents from '../fixtures/labels/labelFileContentsResp.json' import pullReqOpenedEvent from '../fixtures/pullReq/pullReqOpenedEvent.json' @@ -117,7 +118,7 @@ describe('pickReviewers', () => { describe('blunderbussSettings', () => { it('applies the defaults', () => { - expect(blunderbussSettings({ labels: {}, require_matching_label: [], tide: {}, hold: {}, blunderbuss: {}, sources: [] })).toEqual({ + expect(blunderbussSettings({ ...mergeProwConfig({}, {}), sources: [] })).toEqual({ request_count: 2, max_request_count: undefined, exclude_approvers: false, @@ -128,11 +129,11 @@ describe('blunderbussSettings', () => { }) it('reads delay as milliseconds', () => { - expect(blunderbussSettings({ labels: {}, require_matching_label: [], tide: {}, hold: {}, blunderbuss: { delay: '1h30m' }, sources: [] }).delay).toBe(90 * 60_000) + expect(blunderbussSettings({ ...mergeProwConfig({}, {}), blunderbuss: { delay: '1h30m' }, sources: [] }).delay).toBe(90 * 60_000) }) it('lowercases ignore_authors', () => { - expect(blunderbussSettings({ labels: {}, require_matching_label: [], tide: {}, hold: {}, blunderbuss: { ignore_authors: ['Dependabot[bot]'] }, sources: [] }).ignore_authors).toEqual(['dependabot[bot]']) + expect(blunderbussSettings({ ...mergeProwConfig({}, {}), blunderbuss: { ignore_authors: ['Dependabot[bot]'] }, sources: [] }).ignore_authors).toEqual(['dependabot[bot]']) }) }) diff --git a/__tests__/plugins/lgtmBinding.test.ts b/__tests__/plugins/lgtmBinding.test.ts index 19549b9f..8203688d 100644 --- a/__tests__/plugins/lgtmBinding.test.ts +++ b/__tests__/plugins/lgtmBinding.test.ts @@ -20,7 +20,7 @@ afterAll(() => server.close()) const repo = `${utils.api}/repos/Codertocat/Hello-World` const headSha = pullReqOpenedEvent.pull_request.head.sha -function labeled(label: string, sender: Record = { login: 'alice', type: 'User' }, action = 'labeled') { +function labeled(label: string, sender: { login: string, type: string } = { login: 'alice', type: 'User' }, action = 'labeled') { return new utils.MockContext({ ...pullReqOpenedEvent, action, label: { name: label }, sender }) } @@ -114,7 +114,7 @@ describe('lgtmOnPullRequest', () => { }) it('throws when the payload has no head', async () => { - await expect(lgtmOnPullRequest(new utils.MockContext({ action: 'labeled', label: { name: 'lgtm' }, sender: { login: 'alice' } }))).rejects.toThrow('missing pull request head') + await expect(lgtmOnPullRequest(new utils.MockContext({ action: 'labeled', label: { name: 'lgtm' }, sender: { login: 'alice', type: 'User' } }))).rejects.toThrow('missing pull request head') }) }) diff --git a/__tests__/plugins/requireMatchingLabel.test.ts b/__tests__/plugins/requireMatchingLabel.test.ts index 8616b35f..7e2d6c44 100644 --- a/__tests__/plugins/requireMatchingLabel.test.ts +++ b/__tests__/plugins/requireMatchingLabel.test.ts @@ -9,6 +9,7 @@ import { handleIssueComment } from '../../src/issueComment/handleIssueComment' import { handleIssues } from '../../src/issues/handleIssues' import { applicableRules, checkRequiredLabels, enforceRequiredLabels, evaluate, maxGracePeriodMs, parseDuration, requireMatchingLabel } from '../../src/plugins/requireMatchingLabel' import { handlePullReq } from '../../src/pullReq/handlePullReq' +import { mergeProwConfig } from '../../src/utils/config' import * as labeling from '../../src/utils/labeling' import { newOctokit } from '../../src/utils/octokit' import * as sleepModule from '../../src/utils/sleep' @@ -105,7 +106,7 @@ function commentEvent(body: string, overrides: Record = {}) { } function configWith(...rules: RequireMatchingLabel[]): ProwConfig { - return { labels: {}, require_matching_label: rules, tide: {}, hold: {}, blunderbuss: {}, sources: [] } + return { ...mergeProwConfig({}, {}), require_matching_label: rules, sources: [] } } const botComment = { id: 11, body: `Please add a kind label.\n\n${marker}`, user: { login: 'github-actions[bot]', type: 'Bot' } } diff --git a/__tests__/plugins/tide.test.ts b/__tests__/plugins/tide.test.ts index e35ae322..437cde49 100644 --- a/__tests__/plugins/tide.test.ts +++ b/__tests__/plugins/tide.test.ts @@ -1,3 +1,4 @@ +import type { MockInstance } from 'vitest' import { Buffer } from 'node:buffer' import * as core from '@actions/core' import { http } from 'msw' @@ -62,7 +63,7 @@ function observeMerge(status = 200, body: unknown = { merged: true }) { return observe } -let sleep: ReturnType +let sleep: MockInstance let octokit: ReturnType let context: utils.MockContext diff --git a/__tests__/plugins/tideGuards.test.ts b/__tests__/plugins/tideGuards.test.ts index 378c90bb..13f11a73 100644 --- a/__tests__/plugins/tideGuards.test.ts +++ b/__tests__/plugins/tideGuards.test.ts @@ -1,3 +1,4 @@ +import type { MockInstance } from 'vitest' import * as core from '@actions/core' import { http } from 'msw' import { setupServer } from 'msw/node' @@ -81,7 +82,7 @@ function serveGraphql(queue: utils.MergeQueueFixture = {}) { return calls } -let sleep: ReturnType +let sleep: MockInstance let octokit: ReturnType let context: utils.MockContext diff --git a/__tests__/testUtils.ts b/__tests__/testUtils.ts index 8ad2ad96..2f0a6155 100644 --- a/__tests__/testUtils.ts +++ b/__tests__/testUtils.ts @@ -1,4 +1,4 @@ -import type { SetupServerApi } from 'msw/node' +import type { SetupServer } from 'msw/node' import type { Context } from '../src/utils/context' import * as github from '@actions/github' import { http } from 'msw' @@ -38,7 +38,7 @@ export const failOnUnhandledRequest = { onUnhandledRequest: 'error', onUnhandled * * @param server - the file's msw server */ -export function failTestOnUnhandledRequests(server: SetupServerApi) { +export function failTestOnUnhandledRequests(server: SetupServer) { const unhandled: string[] = [] let allowed = false const record = ({ request }: { request: Request }) => { diff --git a/__tests__/utils/ownersGuards.test.ts b/__tests__/utils/ownersGuards.test.ts index 60d185df..561e9fd4 100644 --- a/__tests__/utils/ownersGuards.test.ts +++ b/__tests__/utils/ownersGuards.test.ts @@ -144,7 +144,7 @@ describe('branchHasOwners', () => { resetOwnersCaches() }) - it.each([ + it.each<[string | null, string]>([ ['boom', 'a string'], [null, 'null'], ])('reports a tree rejection of %j (%s) as a listing error, not as a missing tree', async (reason) => { diff --git a/docs/contributing.md b/docs/contributing.md index 5f1ccc83..fcc09cae 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -15,6 +15,12 @@ npm run all # build, lint, pack the dist/ bundle, and test Note that `npm run all` runs `lint:fix` first, which rewrites files in place. +`npm run build` compiles `src/` only (`tsconfig.json` excludes `__tests__/`), +and Vitest strips types without checking them, so a test whose fixture has +drifted from the type it claims to be still runs. `npm run typecheck` +(`tsc -p tsconfig.test.json`) type-checks `src/` and `__tests__/` together +without emitting; run it after changing an exported type or a test fixture. + The action runs from the committed `dist/index.js` (an `ncc` bundle of `src/`). Any change to `src/` must be followed by `npm run pack`, and the resulting `dist/index.js` committed alongside it; CI fails if `dist/` is out of date. diff --git a/package.json b/package.json index fe26c5e1..8fcf9420 100644 --- a/package.json +++ b/package.json @@ -18,6 +18,7 @@ }, "scripts": { "build": "tsc", + "typecheck": "tsc -p tsconfig.test.json", "lint": "eslint", "lint:fix": "eslint --fix", "pack": "ncc build", diff --git a/tsconfig.test.json b/tsconfig.test.json new file mode 100644 index 00000000..cdd8003b --- /dev/null +++ b/tsconfig.test.json @@ -0,0 +1,9 @@ +{ + "extends": "./tsconfig.json", + "compilerOptions": { + "rootDir": ".", + "noEmit": true + }, + "include": ["src/**/*.ts", "__tests__/**/*.ts"], + "exclude": ["node_modules", "__tests__/bundle/coverageEntry"] +} From bc5324c79efda95c61609eebd8c07727a0e4c0cb Mon Sep 17 00:00:00 2001 From: Bob Killen Date: Thu, 29 Oct 2026 11:25:19 -0500 Subject: [PATCH 3/3] test(typecheck): fix the three type errors #593 and #595 added to __tests__/ meowImageEdges.test.ts types the spy as MockInstance, setup.ts types overwriteMethod's _super parameter, and configTypeGuards.test.ts reads labels?.kind? off parseProwConfig's Partial. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Bob Killen --- __tests__/issueCommentTest/meowImageEdges.test.ts | 2 +- __tests__/setup.ts | 2 +- __tests__/utils/configTypeGuards.test.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/__tests__/issueCommentTest/meowImageEdges.test.ts b/__tests__/issueCommentTest/meowImageEdges.test.ts index b2e811e4..db23f8d7 100644 --- a/__tests__/issueCommentTest/meowImageEdges.test.ts +++ b/__tests__/issueCommentTest/meowImageEdges.test.ts @@ -42,7 +42,7 @@ function contextFor(body: string) { // prefix, a url with only a username or only a password is refused, and the // length bound is inclusive at 4096. describe('/meow image validation edges', () => { - let createComment: ReturnType> + let createComment: MockInstance beforeEach(() => { utils.setupActionsEnv('/meow') diff --git a/__tests__/setup.ts b/__tests__/setup.ts index 1b22ebe2..143b7340 100644 --- a/__tests__/setup.ts +++ b/__tests__/setup.ts @@ -16,7 +16,7 @@ vi.mock('@actions/core', { spy: true }) // through the same chai method. chai.use((_chai, utils) => { for (const method of ['throw', 'throws', 'Throw']) { - utils.overwriteMethod(_chai.Assertion.prototype, method, _super => function (this: object, ...args: unknown[]) { + utils.overwriteMethod(_chai.Assertion.prototype, method, (_super: (...args: unknown[]) => unknown) => function (this: object, ...args: unknown[]) { const object: unknown = utils.flag(this, 'object') const negate = Boolean(utils.flag(this, 'negate')) const promise = utils.flag(this, 'promise') diff --git a/__tests__/utils/configTypeGuards.test.ts b/__tests__/utils/configTypeGuards.test.ts index b92c165c..f5057572 100644 --- a/__tests__/utils/configTypeGuards.test.ts +++ b/__tests__/utils/configTypeGuards.test.ts @@ -19,7 +19,7 @@ describe('parseProwConfig type guards', () => { expect(() => parseProwConfig('x', 'kind:\n - { name: bug, color: 123456 }\n')).toThrow(malformed) expect(() => parseProwConfig('x', 'kind:\n - { name: bug, color: 12 }\n')).toThrow(malformed) - expect(parseProwConfig('x', 'kind:\n - { name: bug, color: "123456" }\n').labels.kind.definitions).toEqual([ + expect(parseProwConfig('x', 'kind:\n - { name: bug, color: "123456" }\n').labels?.kind?.definitions).toEqual([ { name: 'bug', color: '123456' }, ]) })