refactor(cli): cover gen types tests with effect lint (CLI-2410) - #6769
Conversation
There was a problem hiding this comment.
🤖 AI Review
Three findings are confirmed: the integration tests use scoped temporary directories without supplying a Scope, Docker failures lose diagnostic detail, and an e2e comment refers to removed checks. The lint finding is refuted: the new pattern adds the migrated tests to Effect lint coverage.
Findings
| Severity | Location | Category | Sources | Claim |
|---|---|---|---|---|
| 🟠 MAJOR | apps/cli/src/commands/gen/types/types.integration.test.ts:65 |
test-runtime |
codex | The integration tests create scoped temporary directories without supplying a Scope, so tests that call makeWorkdir cannot typecheck or run as written. |
| 🟡 MINOR | apps/cli/src/commands/gen/types/types.e2e.test.ts:128 |
diagnostics |
claude | The new runDocker wrapper loses the actual Docker exit code and can hide the error message when stderr is present. A timeout also loses output captured before the timeout. |
| ⚪ NIT | apps/cli/src/commands/gen/types/types.e2e.test.ts:355 |
comments |
codex | The comment refers to previous undefined and empty-string checks as if they appear below, but the current code uses Option checks. |
Refuted findings (kept for transparency, not posted as review comments)
.oxlintrc.effect.json:19(lint-coverage): The new blanket exclusion removes Effect lint coverage from the migrated gen/types test files.
Refuted: In ignorePatterns, the leading ! reverses the initial ignore-all pattern. The new entry includes the gen/types tests in Effect lint coverage; it does not exclude them.
Stats
Claude findings: 1 · Codex findings: 3 · Confirmed: 3 · Refuted: 1 · Uncertain: 0
Models: claude-opus-5-5 + gpt-6-sol · Trigger: auto · Workflow run
This review runs once per PR. A maintainer can request another with a /ai-review comment.
Coly010
left a comment
There was a problem hiding this comment.
Great way to finish the family off. I checked every expect chain against base as a multiset across all six files and they line up (268/268 in integration), titles match, and --debug=files shows exactly the 36 gen files linted with zero findings. Moving container cleanup onto Effect.ensuring is a nice bonus too - it now runs on test timeouts, where the old finally didn't
Also fwiw the failing "Post preview command comment" check is just the cancelled duplicate run, the sibling Test run on the same SHA is fully green
Approving. Only thing I'd ask is a quick tidy of the description before it becomes the squash commit, a few bits don't match the diff:
- it's twelve allow-list entries collapsing rather than eighteen
- there's no
Cause.prettyin this PR, the suite still usesString(exit.cause) Exit.isFailureand the stateful mocks (api.requests,child.calls) were already there on base
None of that's a problem with the code, just so nobody goes hunting for it later
|
:D another flake |
8877d85 to
6c69166
Compare
TL;DR
brings the
supabase gen typestests under the effect lint, finishing thegenfamilywhats introduced?
effect lint applied to the six
gen typestest files, which leaves the whole family covered:genallow list entries in.oxlintrc.effect.jsoncollapse into one!apps/cli/src/commands/gen/**, which also pulls intypes.errors.unit.test.ts, the onegen typestest file this branch leaves untouchedFileSystemwith scoped temp directories, all eighty five cases kept, and its injected network failure becomes a taggedApiCallFailureprocess.envmutation for a hermetic call, and the generator test awaits throughEffectrather than a bare promisewithTempHome, and the live test forwards the vitest signalref: