test: add unit tests with vitest #265
Loading…
Reference in a new issue
No description provided.
Delete branch "test/unit-tests"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
What
First step of the testing strategy (issue #264): Layer 1 — unit tests with Vitest.
vitest+@vitest/coverage-v8devDependencies,test/test:unit/test:coveragescripts,vitest.config.ts(node env),test/setup.tsthat sets the env varssrc/forgejo/fetch.tsreads at import time (FORGEJO_API_URL,CTX_AUTH_TOKEN,CTX_AUTH_USERNAME).src/prompt.test.ts—buildPromptfor issue/PR/review-requested events, branch + tool instructions, empty body, comment ordering.src/schemas.test.ts— event name validation + contract tests validating every payload inexamples/event_payloads/against the schemas.src/context.test.ts—getAuthContextenv parsing,getEventContext(issue vs PR, comments, invalid env) with the Forgejo client mocked.src/tools.test.ts— all 5 tools: default repository/issueId fallback, explicit params, and thecreate-pr-reviewside/line→old_position/new_positionmapping.src/git.test.ts—getLatestCommitId.Notes
tools.test.tsmocks@earendil-works/pi-coding-agent: its index re-exportsmain, which transitively loads an experimental server module importing@earendil-works/pi-server(not installed; the real rolldown build tree-shakes it away). Mocking keeps tests off that module graph.checkoutRepositoryis not unit-tested here — it needs a real git remote, so it belongs to the integration/E2E layers.src/(excludingsrc/forgejo/, which is Layer 2 integration territory): ~88% statements.Checks
pnpm check✅pnpm lint:check✅pnpm format:check✅pnpm test— 30/30 ✅@ci-bot a couple of questions.
I see that in some test groups there's an afterEach hook that calls vi.unmock all or something similar. Is it needed just in those cases? or always and should be added to the others?
I would prefer the payload validating tests to be kept in sync with the forgejo release. Is that possible and would it require the e2e forgejo instance?
Good questions, thanks!
1.
afterEachhooks (vi.unstubAllEnvs/vi.clearAllMocks)They're not "needed everywhere" — they cover two distinct things:
vi.unstubAllEnvs()is only needed in tests that callvi.stubEnv(), which is exclusivelysrc/context.test.ts(it stubsCTX_AUTH_TOKEN,FORGEJO_REPOSITORY, etc.). Without it the stubbed vars would leak into other tests/files. The other test files never stub env vars, so they don't need it.vi.clearAllMocks()is needed wherever mock call history is asserted. Incontext.test.tsit matters because ofexpect(mockedGetPullRequest).not.toHaveBeenCalled()— without clearing, history accumulates and tests become order-dependent.In
tools.test.tsit was technically not required today (all assertions are at-least-oncetoHaveBeenCalledWith), but the lack of it was inconsistent and fragile — e.g., adding anot.toHaveBeenCalled()or enabling shuffle would silently break. I've addedafterEach(() => vi.clearAllMocks())there for the same hygiene (597528b).prompt.test.ts,schemas.test.tsandgit.test.tsdon't mock at all, so they correctly have no hooks.2. Keeping payload contract tests in sync with the Forgejo release
Yes, it's possible, and the e2e instance is only needed for the fully automatic flavor:
examples/event_payloads/README.md→ Forgejo 15.0.3). I've documented the refresh procedure there (capturetoJson(forge.event)from a job running against the target version, update the files + version, runpnpm test). As a bonus, Forgejo publishes its OpenAPI spec per release tag (templates/swagger/v1_json.tmplin the source repo — I verified it's fetchable atv15.0.3), so the underlyingIssue/PullRequest/Comment/Repositorymodels could be diffed against the pinned tag without any instance. The refresh is then a manual/scripted step done when bumping the supported Forgejo version.One nuance worth flagging: the schemas only describe the subset of fields the agent reads and don't set
additionalProperties: false, so the contract tests catch breaking changes (a field the agent uses changed shape or vanished) but not additive ones. That's intentional — a full payload diff against each release is a Layer 2 concern; these tests are the cheap tripwire.@ci-bot mm ok, so I would go with full automatic from e2e and skip using those in the repo
Agreed — payload validation belongs to the e2e layer, where it can be fully automatic. I've removed the fixture-based contract tests from this PR:
src/schemas.test.tsnow only keeps the pure unit tests (eventNameSchemaaccepts the four supported events / rejects unknown ones); the "schemas against the real payloads" block and itsreadFileSyncimport are gone.examples/event_payloads/README.mdis reverted to its pre-PR state — the "Refreshing the payloads" section and the reference tosrc/schemas.test.tsare removed. The fixture files themselves stay, since they pre-date this PR and are used as reference for constructing action workflows, not as test fixtures.Issue/PullRequest/Comment/Repository) themselves remain, sincecontext.tsneeds them for env/event validation — they're just no longer pinned to captured payloads.Updated status:
pnpm test— 24/24 ✅ (30 minus the 6 contract tests)pnpm check✅pnpm lint:check✅pnpm format:check✅Committed as
1fc371b("test(unit): drop payload fixture contract tests, defer to e2e layer"). The full automatic path (spin up the pinned Forgejo, trigger events, capture and validate payloads) stays on the #264 Layer 2 plan.