Draft
Conversation
This was referenced May 5, 2026
Draft
The previous arrangement detected the test runner at runtime, in two
places:
- inside `run()`, an inner `catch` block re-threw when `jest` was
defined and otherwise called `core.setFailed(e.message)`, so that
tests could observe rejections via `expect(...).rejects.toThrow`,
while production runs would surface the failure as a GitHub
Actions step error;
- at the bottom of the file, the same `typeof jest` check decided
between exporting `run` (for tests) and invoking it immediately
(for the action's runtime entrypoint).
Tying production behaviour to the test framework's name is brittle:
it ties source code to a single, named test runner and conflates
"how do I detect the test environment" with "am I being executed as
the program's entrypoint". The conventional Node.js answer to the
latter, `require.main === module`, has been available for a very long
time and does not need any test-runner-specific knowledge.
Switch to that idiom and move the `setFailed` translation out of
`run()` into the entrypoint call site:
if (require.main === module) {
run().catch(e => core.setFailed(e.message))
} else {
module.exports = run
}
`run()` itself now lets errors propagate naturally, which is exactly
what the existing tests already assume (`errors out if GitHub API
returns 500` uses `await expect(run()).rejects.toThrow(...)`). The
shipped action keeps its current end-user behaviour because uncaught
rejections are caught at the entrypoint and forwarded to
`core.setFailed`.
This commit is preparation for switching the test framework from Jest
to Vitest in a follow-up commit; doing the entrypoint cleanup first
keeps the test-runner switch itself purely about the runner, and not
about source-code adjustments to accommodate it.
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
… them
Previously, `run()` reached out to `@actions/core`, `@actions/github`,
and `rss-parser` directly via top-level `require()` calls, so the
test suite mocked them via `jest.mock(...)` (auto-mock followed by
property mutation on the returned namespace object). That pattern
relies on Jest's CommonJS-aware auto-mock: a `jest.mock(path)` call
without a factory replaces every function export with a `jest.fn()`,
and Jest's CJS module loader makes that interception visible to any
later `require(path)`, including ones reached transitively through
the source under test.
Vitest's `vi.mock` does not have an equivalent CommonJS hook. Its
hoisting rewrites only ESM `import` statements at the test-file
level; `require(path)` calls executed inside an imported source
module are served by Node's regular CJS loader and never see the
mock. I verified this directly with vitest 4.1.5 and a minimal
reproduction (a CJS source module that requires `@actions/core`,
loaded from an ESM test file that calls `vi.mock('@actions/core',
...)`): the mock is visible to the test's own `import` of the
package but not to the source module's `require`, which receives
the real package.
To unblock the upcoming jest-to-vitest switch without prematurely
turning the project into ESM (that is reserved for the dependency
update PR, where `@actions/core` v3 and `@actions/github` v9 force
the migration), thread the previously imported modules through an
optional `deps` parameter:
run({ core, getOctokit, context, parseFeed })
`run()` destructures `deps` (or `defaultDeps()` when called with no
argument, which is what the action's runtime entrypoint does). Each
default is the same module export the source previously closed over
at top level, so production behaviour is unchanged.
The seam for `rss-parser` is `parseFeed(url)` rather than the
module itself: the previous call sequence
`new RSSParser(opts).parseURL(url)` is hidden behind a function
that closes over the same options. This means the test no longer
has to mock `https.get` to feed `RSSParser.parseURL`. The test
controls what `run()` sees by resolving `parseFeed` to a parsed
feed object directly. The tests still build those parsed objects
by invoking the real `RSSParser().parseString(xml)` on the same XML
fixtures the previous tests used, so the rss-parser behaviour
(content unescaping, item title handling, etc.) remains in the
test path; only the network-fetch shim is gone.
The tests now construct their dependency object explicitly per
`beforeEach` and pass it into `run(deps)`. There is no module-level
shared mock state any more; each test gets a fresh
`jest.fn()` for every dependency, and `inputs` is reset between
tests as well.
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Follow the path the sibling Action `setup-git-for-windows-sdk` (also
in the `git-for-windows` GitHub organization) took in commit
348f039f ("Switch from Jest to Vitest as the test framework"): the
upstream `@actions/*` v3+ packages this project will be picking up
in the next PR are ESM-only, which forces an ESM migration for any
project that consumes them, which in turn breaks Jest's CommonJS-
oriented mocking infrastructure. Vitest is the framework where ESM
module mocking is a stable, first-class feature, so the
`setup-git-for-windows-sdk` migration treated the runner switch as a
prerequisite. Doing it here in a stand-alone commit keeps each move
small and reviewable: the runner swap, the dependency upgrade, and
the Node 24 runtime bump each get their own pull request.
Vitest 4.x explicitly refuses CommonJS `require('vitest')` calls
("Vitest cannot be imported in a CommonJS module using require().
Please use \"import\" instead.", verified locally with vitest
4.1.5). Two tactics are used to avoid touching that constraint
without yet flipping the project to ESM:
- the previous commit threaded all of `run()`'s external
dependencies through an injectable `deps` parameter, removing
the need for `vi.mock(...)` to intercept transitive `require()`
calls inside `index.js` (which `vi.mock` cannot do anyway, even
from an ESM test file: the hoisted mock only rewrites ESM
`import` statements at the test-file level);
- the new `vitest.config.js` enables Vitest's `globals: true`
mode, so the existing `__tests__/index.test.js` (still
CommonJS) can continue using `test`, `expect`, `beforeEach` and
`vi` as globals without ever importing them.
Concrete changes:
- `package.json`: drop `jest` and `eslint-plugin-jest`, add
`vitest@^4.1.5` and `@vitest/eslint-plugin@^1.6.16`. Switch the
`test` script from `jest` to `vitest run`.
- Replace `jest.config.js` with `vitest.config.js` (CJS, mirroring
the previous `clearMocks: true`/`testEnvironment: 'node'`
configuration; Vitest's `clearMocks` and `environment: 'node'`
are spelled the same way).
- `.eslintrc.yml`: replace `plugin:jest/recommended` with
`plugin:@vitest/legacy-recommended` (the `@vitest/eslint-plugin`
package exposes a `legacy-recommended` config under the scoped
plugin namespace, which is what eslintrc consumes), and add
`'@vitest/env': true` to `env` so the plugin's globals
(`test`, `expect`, `vi`, ...) are recognised by `no-undef`.
- `__tests__/index.test.js`: drop the `@jest/globals` import (now
served by Vitest's `globals: true`) and rewrite `jest.fn()` as
`vi.fn()`. The test bodies are otherwise unchanged because the
previous commit had already removed module-mocking from them.
`dist/index.js` is unaffected because `vitest`/`jest` are
`devDependencies` and never make it into the bundle. `npm run
prepare` confirms the bundle is byte-identical, so this commit
deliberately does not touch `dist/`.
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Assisted-by: Opus 4.7 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First of three stacked PRs that bring this Action onto Node.js 24
ahead of GitHub's June 2, 2026 deadline (see #PR_NODE24 for the
authoritative reference). This PR is rooted on
main; the nexttwo stack on top of it.
PR ordering:
@actions/*and ESLint, switch to ESMruns.usingto"node24"Why Vitest, why now
The same migration was performed in the sibling Action
setup-git-for-windows-sdk(commit348f039f,"Switch from Jest to Vitest as the test framework") and for the
same reason: the upcoming
@actions/*v3+ packages ship asESM-only modules, and Jest's CommonJS-oriented mocking
infrastructure does not survive an ESM migration. Vitest is the
testing framework where ESM module mocking is a stable, first-class
feature. Doing the runner switch in its own PR keeps each move
small.
What's in this PR (4 commits)
typeof jest !== 'undefined'test-runner sentinel.Replace it with the conventional
require.main === moduleandmove the
core.setFailed(e.message)translation out ofrun()into the entrypoint call site. Pure refactor on Jest; tests
still pass before the runner switch.
@actions/*and the feed parser intorun()insteadof importing them. Verified up front that vitest 4.x's
vi.mockcannot interceptrequire()calls inside atransitively loaded CJS module; the test-file hoisting only
rewrites ESM
importstatements at the test-file level. So thetests stay self-contained by threading dependencies through an
injectable
depsparameter (defaultDeps()covers production)and exposing a
parseFeedseam that hidesnew RSSParser(opts).parseURL(url). Nohttps.getmockingneeded any more.
npm run prepare—dist/rebuild after the source change,per repo convention.
sibling Action's migration commit. Uses
globals: trueinvitest.config.jsso the (still CommonJS) test file can usetest/expect/vias globals without ever importingvitest(which vitest 4.x rejects viarequire()anyway)..eslintrc.ymlswitched toplugin:@vitest/legacy-recommended(the scoped plugin namespace) and gained
'@vitest/env': trueso the plugin's globals are recognised by
no-undef.dist/index.jsis unaffected by this commit because vitest isa
devDependency, so PR1 closes without an extranpm run preparecommit.Verification
npm run lint,npm run test,npm run prepareall clean.dist/index.jswith syntheticINPUT_*env vars: the
::debug::and::error::GitHub Actions loglines confirm
core.getInput()andcore.setFailed()flowthrough the new
require.main === moduleentrypoint correctly.