diff --git a/__tests__/cronJobTest/labelSync.test.ts b/__tests__/cronJobTest/labelSync.test.ts index 2cafc6aa..9142b3a9 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 9a78b0b3..978dc85d 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 { http } from 'msw' import { afterEach, beforeEach, expect } from 'vitest' @@ -104,6 +105,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 3f5c5020..46fc1e6c 100644 --- a/__tests__/utils/auth.test.ts +++ b/__tests__/utils/auth.test.ts @@ -513,16 +513,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 +583,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 8c515deb..d0b186e1 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,12 @@ 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') + // all fresh: after a sibling's resetModules the static queueState would warn through another core + // instance, and the fresh module graph's platform slot is empty until this test fills it + const [core, fresh, freshPlatform, adapter] = await Promise.all([import('@actions/core'), import('../../src/utils/mergeQueue'), import('../../src/platform'), import('../../src/adapters/actions')]) + freshPlatform.setPlatform(adapter.actionsPlatform()) - 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/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 7373a0e6..554da00e 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -50,6 +50,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 / 99 % 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 @@ -111,6 +112,38 @@ 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 that outlives the test.** Everything a handler memoizes for an + event — configuration, repository labels, OWNERS trees, warnings already + shown — lives in the platform's `RunState`, and `__tests__/setup.ts` + installs a fresh platform before every test, so a test inherits nothing the + previous one loaded. A test that swaps in its own platform mid-test (or a + `describe` that needs an empty run between its own tests) calls + `utils.newRun()`. 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 c0efa7ee..496b0d7f 100644 --- a/package.json +++ b/package.json @@ -33,6 +33,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",