fix: Make docker and stuff run for CI. #3792
Conversation
perkinsjr
requested review from
chronark,
mcstepp,
MichaelUnkey,
ogzhanolguncu and
Flo4604
as code owners
|
📝 WalkthroughWalkthroughUpdates CI unit test workflow to set up Docker/Testcontainers and install Goose CLI. Adjusts a ClickHouse test to pass a new tags parameter (null) to verifications.timeseries.perDay and performs minor SQL formatting changes. Changes
Sequence Diagram(s)sequenceDiagram
actor Dev as Developer
participant GH as GitHub Actions (Unit Tests)
participant Docker as Docker Engine
participant Goose as Goose CLI
participant Tests as Test Runner
Dev->>GH: Push/PR triggers unit tests
GH->>GH: Setup Buildx
GH->>Docker: Configure socket permissions
GH->>GH: Install Goose CLI (v3.20.0)
GH->>Tests: Run tests with Testcontainers env\n(RYUK_DISABLED, DOCKER_HOST, HOST_OVERRIDE)
Tests->>Docker: Start containers as needed
Tests-->>GH: Report results
sequenceDiagram
participant Test as ClickHouse Test
participant TS as verifications.timeseries
participant CH as ClickHouse
Test->>TS: perDay({ ..., tags: null })
TS->>CH: Execute timeseries query (tags param considered)
CH-->>TS: Rows
TS-->>Test: Aggregated per-day results
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
internal/clickhouse/src/verification_outcomes_propagate_correctly.test.ts (1)
147-161: Exercise the new tags parameter in the perDay timeseries test and assert DB ↔ API parityShort: perDay already accepts null (verificationTimeseriesParams uses z.array(...).nullable()). Update the test to (1) call perDay with an explicit tag filter using the correct shape and assert identical results, and (2) assert the DB aggregates (directAggregateData) match expectedOutcomes.
Files / locations:
- internal/clickhouse/src/verification_outcomes_propagate_correctly.test.ts — around the perDay call (lines ~147–161) and the block where dbAggregates is computed but not asserted.
- internal/clickhouse/src/verifications.ts — verificationTimeseriesParams shows tags is nullable (no change required, just confirming the API accepts null).
Suggested changes (insert after the existing perDay call and add the DB assertion where dbAggregates is built):
- Exercise tag filter (correct shape: { operator, value }):
const dailyWithTagFilter = await ch.verifications.timeseries.perDay({ workspaceId, keyspaceId: keySpaceId, keyId, startTime: start - interval, endTime: end + interval, identities: null, keyIds: null, names: null, outcomes: null, tags: [{ operator: "is", value: "tag" }], }); if (daily && dailyWithTagFilter) { expect(dailyWithTagFilter.length).toBe(daily.length); const sum = (buckets: typeof daily) => buckets.reduce( (acc, b) => acc + (b.y.valid_count ?? 0) + (b.y.rate_limited_count ?? 0) + (b.y.disabled_count ?? 0), 0, ); expect(sum(dailyWithTagFilter)).toBe(sum(daily)); }
- Assert DB vs API parity where dbAggregates is constructed:
if (directAggregateData && directAggregateData.length > 0) { const dbAggregates = { VALID: 0, RATE_LIMITED: 0, DISABLED: 0, }; directAggregateData.forEach((row) => { if (row.outcome in dbAggregates) { dbAggregates[row.outcome as keyof typeof dbAggregates] = row.total; } }); expect(dbAggregates).toStrictEqual(expectedOutcomes); }Notes:
- The correct tags parameter shape is an array of { operator: "is" | "contains" | "startsWith" | "endsWith", value: string } — do not pass plain string arrays.
- I verified verificationTimeseriesParams declares tags as .nullable(), so passing null is explicit and valid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
.github/workflows/job_test_unit.yaml(2 hunks)internal/clickhouse/src/verification_outcomes_propagate_correctly.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{js,jsx,ts,tsx}: Use Biome for formatting and linting in TypeScript/JavaScript projects
Prefer named exports over default exports in TypeScript/JavaScript, except for Next.js pages
Files:
internal/clickhouse/src/verification_outcomes_propagate_correctly.test.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.{ts,tsx}: Follow strict TypeScript configuration
Use Zod for runtime validation in TypeScript projects
Files:
internal/clickhouse/src/verification_outcomes_propagate_correctly.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Use Vitest for unit and integration tests in TypeScript projects
Files:
internal/clickhouse/src/verification_outcomes_propagate_correctly.test.ts
**/*.{env,js,ts,go}
📄 CodeRabbit Inference Engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
internal/clickhouse/src/verification_outcomes_propagate_correctly.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
🔇 Additional comments (2)
internal/clickhouse/src/verification_outcomes_propagate_correctly.test.ts (2)
90-97: SQL formatting cleanup looks good.Readability improved; no semantic changes. The filter conditions remain identical.
119-126: LGTM on aggregate query formatting.Same query semantics with cleaner SELECT/WHERE layout.
|
I don't disagree with rabbit, but at the same time.. if it makes the test go green... |
|
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (08/15/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
What does this PR do?
I dunno if this is the right thing, or if it works, or if we should have tsts.
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Tests
Chores