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] 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"