test: improve ci completion time#132
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
WalkthroughDeletes OpenAI-based moderation code and types from the auto-moderation stack. Updates package scripts and dependencies (removes openai), raises Node engine, adds Dockerignore, enhances GitHub Actions/pnpm caching and BuildKit support, updates the Dockerfile, and tweaks a concurrent test timing. ChangesAuto-moderation Stack Cleanup
Tooling, CI, Docker, and tests
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
toto04
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/throttle.test.ts (1)
22-50:⚠️ Potential issue | 🟠 Major
describe.concurrent+ real timers makes these tests flaky.Tests 1 and 2 assert on exact spy call counts that depend on real
setTimeoutfiring within precise windows (10 ms call cadence vs. 100/50 ms throttle limits). Withdescribe.concurrent, all tests share the same event loop and contend for timer dispatch. On a busy CI runner, a 100 ms throttle timeout can fire 10–30 ms late, shifting "exactly 3 calls" to 2 or 4 and producing intermittent failures.To fix this deterministically, use
vi.useFakeTimers()+vi.advanceTimersByTimeAsync()in each test. The throttle implementation only relies onsetTimeout, which fake timers handle cleanly. However, note thatvi.useFakeTimersis a global switch in the worker thread and does not isolate between concurrent tests in the samedescribe.concurrentblock. Workarounds:
- Run the suite with
--pool=forks(isolates each test file in its own process), or- Move timer-mocked tests into their own non-concurrent
describeblock, or- Drop
describe.concurrententirely for this suiteSketch using fake timers (deterministic, no real waits)
-import { describe, expect, it, vi } from "vitest" +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest" import { throttle } from "@/utils/throttle" -import { wait } from "@/utils/wait" - -async function callNTimes(n: number, ms: number, fn: () => void) { - for (let i = 0; i < n; i++) { - fn() - await wait(ms) - } -} + +async function callNTimes(n: number, ms: number, fn: () => void) { + for (let i = 0; i < n; i++) { + fn() + await vi.advanceTimersByTimeAsync(ms) + } +} @@ describe.concurrent("throttle function", () => { + beforeEach(() => vi.useFakeTimers()) + afterEach(() => vi.useRealTimers()) @@ - await wait(limitms + 20) + await vi.advanceTimersByTimeAsync(limitms + 20)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/throttle.test.ts` around lines 22 - 50, The tests inside describe.concurrent are flaky because they rely on real timers; switch each test that asserts exact call counts to use fake timers by invoking vi.useFakeTimers() at test start and vi.restoreAllMocks()/vi.useRealTimers() at the end (or afterEach), and advance timers deterministically with vi.advanceTimersByTimeAsync(...) instead of awaiting real waits; update the tests that call throttle(...) and wait(...) to replace real wait() delays with vi.advanceTimersByTimeAsync(limitms + 20) or the appropriate advancement so the throttle implementation (which uses setTimeout) fires deterministically; alternatively remove describe.concurrent or move these timer-mocked tests into a non-concurrent describe block if isolating fake timers is undesirable.
🧹 Nitpick comments (3)
tests/throttle.test.ts (1)
55-57: Nit:await wait(1)is unnecessary here.
throttleinvokesfuncsynchronously on the first call (func(...args)aftersetTimeout(handler, limit)insrc/utils/throttle.ts), so the spy is already recorded by the time control reaches theexpect. You can drop theawait wait(1)entirely without weakening the assertion, which also makes the test fully resilient to any event-loop scheduling delays underdescribe.concurrent.♻️ Proposed simplification
throttled() - await wait(1) expect(spy).toHaveBeenCalledTimes(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/throttle.test.ts` around lines 55 - 57, The test calls throttled() and then unnecessarily awaits wait(1) before asserting spy invocation; since throttle calls func synchronously on the first call (see handler scheduling in throttle and the call to func(...args) after setTimeout), remove the `await wait(1)` line so the test simply calls `throttled()` then `expect(spy).toHaveBeenCalledTimes(1)` to make the assertion immediate and resilient under describe.concurrent.package.json (2)
13-62: Nit: PR title doesn't reflect the scope of these changes.The PR is titled "test: halfed test wait time (concurrency)", but this file also bumps the Node engine to
>=24.0.0, swaps the typecheck toolchain (tsc→tsgo), upgrades several runtime deps to new majors (pino10,redis5,croner10), and dropsopenai. Each of those is an independently risky change — Node 24-only dropping for users still on 22 LTS, redis v4→v5 and pino v9→v10 typically have breaking API/config changes, and removingopenaicouples this PR to whichever PR/feature deletes the AI moderation code.Consider splitting into smaller PRs (or at least retitle to reflect the dep/toolchain bump) so reviewers can reason about each change independently and so a regression in, say, redis v5 doesn't get bisected back to "halve test wait time."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 13 - 62, The package.json changes bundle multiple risky modifications (Node engine "node": ">=24.0.0", the typecheck script switching to "typecheck": "tsgo --noEmit", major dependency bumps for "pino", "redis", "croner", and removal of the "openai" dependency) under a PR titled about test wait time; split these into separate PRs or at minimum retitle this PR to reflect the broad scope (e.g., "chore(deps): bump node, switch typecheck to tsgo, upgrade pino/redis/croner, remove openai") and move each logical change into its own branch/PR so reviewers can evaluate Node engine bump, toolchain change, each major dependency upgrade (pino, redis, croner) and the openai removal independently; ensure each PR includes a short migration note calling out possible breaking changes and tests exercising affected codepaths (especially places using pino, redis, croner, and any AI moderation code that referenced openai).
14-14:tsgo(@typescript/native-preview) is now highly stable, but still in beta.As of April 2026 (TypeScript 7.0 Beta release), tsgo is officially described as "highly stable" and "ready to be put to the test in your daily workflows and CI pipelines today" with identical type-checking semantics to TypeScript 6.0. The false-positive/negative risk the original review outlined is substantially lower. However:
- You're pinned to a specific daily dev build (
7.0.0-dev.20260421.2). CI is coupled to a snapshot of a beta compiler. Plan to migrate to stabletypescript@7once it ships (expected late June 2026).- The official recommendation remains to keep stable TypeScript 6.x in production CI until TypeScript 7.0 stable releases. Using
tsgofor experimentation alongside is reasonable for the ~10x speed gain.Optionally, keep a
typecheck:tscfallback using the stabletypescriptpackage as a pre-release sanity check:"typecheck": "tsgo --noEmit", + "typecheck:tsc": "tsc --noEmit",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 14, The package.json currently uses the experimental "tsgo" command in the "typecheck" script; add a stable fallback script and document the intended migration: keep "typecheck" as is (using tsgo) and add "typecheck:tsc": "tsc --noEmit" (or "npm run typecheck:tsc" alias) so CI or devs can run the stable TypeScript 6.x check; also ensure package.json devDependencies include a pinned "typescript" version for the fallback and add a short comment in README/CI notes that the tsgo-based "typecheck" is experimental and will be migrated to "typescript@7" when stable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/throttle.test.ts`:
- Around line 22-50: The tests inside describe.concurrent are flaky because they
rely on real timers; switch each test that asserts exact call counts to use fake
timers by invoking vi.useFakeTimers() at test start and
vi.restoreAllMocks()/vi.useRealTimers() at the end (or afterEach), and advance
timers deterministically with vi.advanceTimersByTimeAsync(...) instead of
awaiting real waits; update the tests that call throttle(...) and wait(...) to
replace real wait() delays with vi.advanceTimersByTimeAsync(limitms + 20) or the
appropriate advancement so the throttle implementation (which uses setTimeout)
fires deterministically; alternatively remove describe.concurrent or move these
timer-mocked tests into a non-concurrent describe block if isolating fake timers
is undesirable.
---
Nitpick comments:
In `@package.json`:
- Around line 13-62: The package.json changes bundle multiple risky
modifications (Node engine "node": ">=24.0.0", the typecheck script switching to
"typecheck": "tsgo --noEmit", major dependency bumps for "pino", "redis",
"croner", and removal of the "openai" dependency) under a PR titled about test
wait time; split these into separate PRs or at minimum retitle this PR to
reflect the broad scope (e.g., "chore(deps): bump node, switch typecheck to
tsgo, upgrade pino/redis/croner, remove openai") and move each logical change
into its own branch/PR so reviewers can evaluate Node engine bump, toolchain
change, each major dependency upgrade (pino, redis, croner) and the openai
removal independently; ensure each PR includes a short migration note calling
out possible breaking changes and tests exercising affected codepaths
(especially places using pino, redis, croner, and any AI moderation code that
referenced openai).
- Line 14: The package.json currently uses the experimental "tsgo" command in
the "typecheck" script; add a stable fallback script and document the intended
migration: keep "typecheck" as is (using tsgo) and add "typecheck:tsc": "tsc
--noEmit" (or "npm run typecheck:tsc" alias) so CI or devs can run the stable
TypeScript 6.x check; also ensure package.json devDependencies include a pinned
"typescript" version for the fallback and add a short comment in README/CI notes
that the tsgo-based "typecheck" is experimental and will be migrated to
"typescript@7" when stable.
In `@tests/throttle.test.ts`:
- Around line 55-57: The test calls throttled() and then unnecessarily awaits
wait(1) before asserting spy invocation; since throttle calls func synchronously
on the first call (see handler scheduling in throttle and the call to
func(...args) after setTimeout), remove the `await wait(1)` line so the test
simply calls `throttled()` then `expect(spy).toHaveBeenCalledTimes(1)` to make
the assertion immediate and resilient under describe.concurrent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91538267-24cb-40fd-8a12-6706520ce609
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
package.jsonsrc/middlewares/auto-moderation-stack/ai-moderation.tssrc/middlewares/auto-moderation-stack/constants.tssrc/middlewares/auto-moderation-stack/index.tssrc/middlewares/auto-moderation-stack/types.tstests/throttle.test.ts
💤 Files with no reviewable changes (4)
- src/middlewares/auto-moderation-stack/constants.ts
- src/middlewares/auto-moderation-stack/index.ts
- src/middlewares/auto-moderation-stack/types.ts
- src/middlewares/auto-moderation-stack/ai-moderation.ts
Co-authored-by: Copilot <copilot@github.com>
2 participants
Volevo eseguire i test un po' più veloci dato che avevo notato che
throttleaspettava tutto sequenzialmente e mi sono fatto prendere la mano 🤭throttlein concorrenzatypecheckè molto più veloce usandotsgodockerfileche del workflow dapolinet.cc)pnpm, così il container smette di scaricarlo con corepack ogni volta che si avvia il pod, che era una cosa che non aveva senso in primo luogo, il time-to-deploy su argocd dovrebbe in teoria essere migliorato di interi secondiniente di lifechanging, ma è stato divertente