Conversation
BenchstatBase: |
WalkthroughThis pull request refactors test infrastructure to enable concurrent test execution with coordinated database setup. It introduces synchronized lifecycle hooks across test suites and a new setup module that manages template creation and per-node database cloning for parallel test nodes. Changes
Sequence Diagram(s)sequenceDiagram
participant Ginkgo as Ginkgo Test Runner
participant SyncSetup as SynchronizedBeforeSuite
participant Template as SetupTemplate
participant DB as PostgreSQL
participant Node as SetupNode
participant TestContext as Test Context
Ginkgo->>SyncSetup: Trigger synchronized setup
SyncSetup->>Template: SetupTemplate(setupOpts)
Template->>DB: Connect (DUTY_DB_URL or embedded)
Template->>DB: Create template database
Template->>DB: Populate dummy data (if enabled)
Template-->>SyncSetup: Return serialized templateInfo
Note over Ginkgo: Per test node execution
SyncSetup->>Node: SetupNode(templateInfo, setupOpts)
Node->>DB: Clone template to node database
Node->>Node: Setup node context with DB name/URL
Node->>Node: Inject fake Kubernetes client
Node-->>TestContext: Return node-specific context
TestContext-->>Ginkgo: Context ready for test execution
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 114: The go.mod contains improperly organized require blocks (the
indirect entry "github.com/clipperhouse/stringish v0.1.1 // indirect" is out of
order and split across blocks); run `go mod tidy` to merge/clean the require
blocks and reorder entries alphabetically, also update/go mod tidy to refresh
go.sum, then commit the tidied go.mod and go.sum so the module files are
consistent.
In `@Makefile`:
- Around line 15-18: The ginkgo commands in the Makefile (the default ginkgo
target and the test-concurrent target) are missing the repository-standard label
filter for local-ignored tests; update both ginkgo invocations to use
--label-filter "!e2e && !ignore_local" (i.e., add && !ignore_local to the
existing --label-filter) so the lines that currently call ginkgo -r -v
--skip-package=... --label-filter "!e2e" include the combined filter.
In `@tests/setup/template.go`:
- Around line 144-151: The direct-mode branch that checks info.TemplateDB == ""
currently starts duty in duty.ClientOnly via duty.Start("direct",
duty.ClientOnly, ...), which skips migrations and opts.DummyData and allows all
nodes to share one DB; change this branch to follow the same initialization flow
as SetupDB (i.e., invoke duty.Start with the same mode/opts used in SetupDB so
per-node clones, migrations and DummyData are applied, then call
setupNodeContext), or if you intentionally cannot create per-node clones, make
this branch explicitly reject parallel execution (e.g., panic or return an
error) with a clear message; update the code paths around TemplateDB,
duty.Start, and setupNodeContext to reflect the chosen behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61ba1b3e-5561-4cc8-84e3-ba129d6c2bdf
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
Makefilego.modtests/context_test.gotests/e2e/loki_test.gotests/e2e/suite_test.gotests/setup/common.gotests/setup/template.gotests/suite_test.go
| ) | ||
|
|
||
| require ( | ||
| github.com/clipperhouse/stringish v0.1.1 // indirect |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the current state of go.mod and see line 114 in context
head -n 120 go.mod | tail -n 20Repository: flanksource/duty
Length of output: 667
🏁 Script executed:
# Also check the full size of go.mod to understand the file better
wc -l go.modRepository: flanksource/duty
Length of output: 70
🏁 Script executed:
# Let's search for the stringish dependency to understand the context
rg "stringish" go.mod go.sumRepository: flanksource/duty
Length of output: 319
🏁 Script executed:
# Show larger context around line 114 - the indirect require block
sed -n '110,130p' go.modRepository: flanksource/duty
Length of output: 879
🏁 Script executed:
# Check the entire indirect require block to see ordering
sed -n '/^require (/,/^)/p' go.mod | grep "indirect"Repository: flanksource/duty
Length of output: 16297
🏁 Script executed:
# Look for CI configuration files that might show the lint failure
find . -maxdepth 2 -name ".github" -o -name "*.yml" -o -name "*.yaml" | head -20Repository: flanksource/duty
Length of output: 132
🏁 Script executed:
# Check if there's a Makefile or script that runs go mod tidy
find . -maxdepth 2 -name "Makefile" -o -name "*.sh" | xargs grep -l "go mod tidy" 2>/dev/null || echo "No tidy scripts found"Repository: flanksource/duty
Length of output: 70
🏁 Script executed:
# Check the Makefile for tidy/lint targets
grep -A 3 "tidy\|lint\|mod" Makefile | head -20Repository: flanksource/duty
Length of output: 518
🏁 Script executed:
# Look for GitHub Actions workflows
ls -la .github/workflows/ 2>/dev/null || echo "No workflows directory"Repository: flanksource/duty
Length of output: 535
🏁 Script executed:
# Check golangci.yaml for relevant lint rules
cat .golangci.yamlRepository: flanksource/duty
Length of output: 671
🏁 Script executed:
# Check the lint.yml workflow for go mod tidy check
cat .github/workflows/lint.ymlRepository: flanksource/duty
Length of output: 961
Commit the tidied module files.
The go.mod file violates Go module hygiene. Line 114's indirect dependency is split across two require blocks and lacks proper alphabetical ordering (entries starting with "c" should precede those starting with "g"). Run go mod tidy to reorganize and merge the require blocks, then commit the result.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 114, The go.mod contains improperly organized require blocks
(the indirect entry "github.com/clipperhouse/stringish v0.1.1 // indirect" is
out of order and split across blocks); run `go mod tidy` to merge/clean the
require blocks and reorder entries alphabetically, also update/go mod tidy to
refresh go.sum, then commit the tidied go.mod and go.sum so the module files are
consistent.
| ginkgo -r -v --skip-package=tests/e2e --skip-package=bench --label-filter "!e2e" | ||
|
|
||
| test-concurrent: ginkgo | ||
| ginkgo -r -v --nodes=4 --skip-package=bench --label-filter "!e2e" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the repository currently has specs labeled ignore_local.
rg -n --glob '**/*_test.go' 'Label\("ignore_local"\)'Repository: flanksource/duty
Length of output: 42
Add !ignore_local filter to match repository standards.
The repository standard (per learnings) is to use --label-filter='!ignore_local' when running tests with ginkgo. Update both targets to include this filter, e.g. --label-filter "!e2e && !ignore_local".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 15 - 18, The ginkgo commands in the Makefile (the
default ginkgo target and the test-concurrent target) are missing the
repository-standard label filter for local-ignored tests; update both ginkgo
invocations to use --label-filter "!e2e && !ignore_local" (i.e., add &&
!ignore_local to the existing --label-filter) so the lines that currently call
ginkgo -r -v --skip-package=... --label-filter "!e2e" include the combined
filter.
| if info.TemplateDB == "" { | ||
| // Direct connection mode (DUTY_DB_CREATE=false) | ||
| PgUrl = info.AdminURL | ||
| ctx, _, err := duty.Start("direct", duty.ClientOnly, duty.WithUrl(PgUrl)) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("failed to connect to db: %v", err)) | ||
| } | ||
| return setupNodeContext(ctx, "direct") |
There was a problem hiding this comment.
Direct mode regresses the old setup contract.
Unlike tests/setup/common.go:SetupDB, the TemplateDB == "" branch on Line 147 goes straight into duty.ClientOnly, so an existing DUTY_DB_URL no longer gets migrations or opts.DummyData. That matches the current BeforeSuite panic on missing migration_logs/properties, and test-concurrent would also make every node share the same database on this path. Keep this branch on the same initialization flow as SetupDB, or explicitly reject parallel execution when no per-node clone can be created.
🧰 Tools
🪛 GitHub Actions: Test
[error] 149-149: PANICKED! BeforeSuite failed: failed to connect to db: ERROR: relation "migration_logs" does not exist (SQLSTATE 42P01); earlier errors include relation "properties" does not exist. This caused the OpenAPI/Duty test suite to abort with a failing BeforeSuite.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/setup/template.go` around lines 144 - 151, The direct-mode branch that
checks info.TemplateDB == "" currently starts duty in duty.ClientOnly via
duty.Start("direct", duty.ClientOnly, ...), which skips migrations and
opts.DummyData and allows all nodes to share one DB; change this branch to
follow the same initialization flow as SetupDB (i.e., invoke duty.Start with the
same mode/opts used in SetupDB so per-node clones, migrations and DummyData are
applied, then call setupNodeContext), or if you intentionally cannot create
per-node clones, make this branch explicitly reject parallel execution (e.g.,
panic or return an error) with a clear message; update the code paths around
TemplateDB, duty.Start, and setupNodeContext to reflect the chosen behavior.
1 participant
Summary by CodeRabbit
Tests
test-concurrenttargetChores