Skip to content

chore(tests): remove low-signal tests and consolidate the testing setup - #8295

Merged
waleedlatif1 merged 2 commits into
stagingfrom
chore/test-desloppify
Sep 25, 2026
Merged

waleedlatif1 merged 2 commits into
stagingfrom
chore/test-desloppify

Conversation

@waleedlatif1

@waleedlatif1 waleedlatif1 commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Delete low-signal unit tests across the monorepo: config/registry restatements, mock-call choreography, per-route copies of route-builder behavior, schema accept/reject checks, render/class/query-key tests, tests of mocks and factories, and near-duplicate variants. Kept: security denials, billing math, executor semantics, real parsers, cross-process wire contracts, and demonstrated regressions
  • apps/sim: 4,027 → 3,271 test files, 60,643 → 34,286 tests. Repo-wide: ~438k lines of tests removed
  • Remove production exports whose only callers were deleted tests (validateStringParam, validateRequiredFields, resubscribe, resolveSelectExportValue, getSlackV2ToolAccess, unused provider/model helpers, USAGE_PROVIDER_ICON_IDS)
  • Remove ~130 orphaned @sim/testing exports (unused assertions, builders, factories, mock helpers, setup/)
  • Consolidate the real-database layer: *.postgres.test.ts → *.integration.ts, one TEST_DATABASE_URL/TEST_REDIS_URL contract enforced by packages/db/testing/test-infrastructure.ts, CI discovers suites by glob instead of a hand-maintained file list, and every integration run uploads a JSON report. *.live.test.ts holds opt-in provider/sandbox suites (never in CI); 7 integration suites that already failed on a fresh DB are quarantined explicitly in apps/sim/vitest.config.ts
  • Upgrade Vitest 4.1 → 5.0.1 (5.0.2 is inside the minimumReleaseAge window); publish workflows run tests on Node 22 and keep a Node 20 bundle smoke
  • Drop the 3-way apps/sim test sharding — the suite runs in the Lint and Test job again
  • Drop ~2,960 redundant @vitest-environment node docblocks (node is the default) and ~2,260 vi.clearAllMocks() in beforeEach (Vitest 5 clears mocks before every test)
  • Add testing rules to AGENTS.md/CLAUDE.md: never write unit tests after the code, prefer E2E tests that end with a verifiable artifact, write failure modes down first when testing in isolation
  • Add a test-audit skill (authoring gate, junk patterns, retention bar, audit/campaign workflow), rewrite .claude/rules/sim-testing.md around test layers and naming, and gate new tests in /ship and /cleanup

Type of Change

  • Other: test cleanup, tooling, and docs

Testing

  • bun run test (every workspace + scripts + setup): all green except 2 pre-existing background/knowledge-processing.test.ts cases that fail only when the checkout path contains /private
  • Integration layer run against local Docker Postgres 17/16 + Redis in both push and migrate provisioning: every file that ran in CI before still passes; SCIM HTTP e2e 17/17
  • bunx turbo run type-check, bun run lint, bun run check:audits (49), docs-manifest:check, block registry, check:migrations all pass

Greptile will likely refuse this PR for file count; the diff is almost entirely test deletions.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@waleedlatif1
waleedlatif1 requested a review from a team as a code owner September 25, 2026 11:38
@vercel

vercel Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
docs Ready Ready Preview Sep 25, 2026 11:56am UTC

Request Review

@greptile-apps

greptile-apps Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Medium risk] Consolidates test infrastructure and removes low-value unit tests.

The changes since the previous review appear safe to merge; no new blocking issue was established.

Summary

The PR removes low-signal tests and consolidates unit, integration, and opt-in live-test routing. Since the previous review, it centralizes database URL checks across integration suites and adjusts a few test expectations. No new actionable issue was established.

Reviews (2) · Last reviewed commit: "fix(tests): address review — shared disp..."

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 3000 files

Confidence score: 5/5

  • The Vitest command documented in .claude/rules/sim-testing.md may point to a missing binary and make the simulation test instructions fail; verify the relative path from apps/sim before relying on it.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name=".claude/rules/sim-testing.md">

<violation number="1" location=".claude/rules/sim-testing.md:125">
P3: `../../node_modules/.bin/vitest` run from `apps/sim` resolves one directory above the repository root (`apps/sim` is directly under the root), so it points at a path that does not exist. The hoisted binary is at the repo root: `../node_modules/.bin/vitest run <paths>`.</violation>
</file>

Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
Tip: instead of fixing issues one by one fix them all with cubic

Re-trigger cubic

Comment thread .cursor/rules/sim-testing.mdc
Comment thread apps/sim/background/cleanup-table-row-ttl.integration.ts
Comment thread apps/sim/lib/billing/calculations/usage-reservation.integration.ts Outdated
Comment thread .agents/skills/ship/SKILL.md Outdated
Comment thread apps/sim/lib/knowledge/connectors/sync-persistence.integration.ts Outdated
Comment thread .agents/skills/test-audit/SKILL.md Outdated
Comment thread .agents/skills/cleanup/SKILL.md
Comment thread apps/sim/ee/access-requests/lib/application/flow.integration.ts Outdated
Comment thread .claude/rules/sim-testing.md
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

Restored the formatClientInfo/parseClientInfo wire-contract cases (fixed token order, format→parse round-trip, any-order trailing tokens) in 87cc346 — that header crosses the CLI/desktop → server boundary, so it meets the retention bar.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 3000 files

Confidence score: 4/5

  • .agents/skills/test-audit/SKILL.md may reject valid behavior-focused unit and regression tests solely because they were added after implementation, including tests that expose the intended regression; allow tests before or after code and assess them against the contract.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name=".agents/skills/test-audit/SKILL.md">

<violation number="1" location=".agents/skills/test-audit/SKILL.md:20">
P2: This blanket rule blocks valid behavior-focused unit and regression tests added after implementation, even when they fail for the intended regression. Allow tests before or after code and judge them by the contract they protect and whether they fail when the fix is reverted.</violation>
</file>

Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.

Fix all with cubic | Re-trigger cubic

Comment thread apps/sim/lib/knowledge/__integration__/migration-fixture.ts
Comment thread .agents/skills/test-audit/SKILL.md
@waleedlatif1
waleedlatif1 merged commit 9050e69 into staging Sep 25, 2026
35 checks passed
@waleedlatif1
waleedlatif1 deleted the chore/test-desloppify branch September 25, 2026 16:03

This branch was successfully deployed

1 active deployment
Preview — 87cc346f Deployed Sep 25, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant