chores: Add GH Workflow Integration Tests#53
Conversation
Closes: PQE-401
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds integration testing: a GitHub Actions workflow runs ops package integration tests against Postgres and Neo4j service containers; a new docker-compose file defines those services; and a new integration test suite ( Changes
Sequence Diagram(s)sequenceDiagram
participant GH as "GitHub Actions Runner"
participant Repo as "Repository"
participant Postgres as "Postgres Container"
participant Neo4j as "Neo4j Container"
participant Test as "Integration Tests (ops)"
GH->>Repo: checkout code
GH->>Postgres: start postgres service (healthcheck)
GH->>Neo4j: start neo4j service (healthcheck)
GH->>GH: setup Go, cache modules
GH->>Test: run `go test -tags integration ./ops/...` with connection string
Test->>Postgres: open DB connection / run write/read transactions
Test->>Neo4j: open DB connection / run write/read transactions
Postgres-->>Test: return query results
Neo4j-->>Test: return query results
Test-->>GH: exit status (pass/fail)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
.github/workflows/go-test.yml (3)
78-106: Both database services start for each matrix job.Currently, both Postgres and Neo4j containers start for every matrix entry, even though each test only uses one. This doubles resource usage and startup time.
GitHub Actions doesn't natively support conditional services, but you could work around this by:
- Moving to separate jobs instead of a matrix
- Using
docker runin a step instead of services (conditionally based on matrix.driver)This is a minor optimization and fine to defer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/go-test.yml around lines 78 - 106, The workflow currently starts both services "postgres" and "neo4j" for every matrix entry which wastes resources; change the workflow so only the needed DB runs per matrix.driver by either (A) splitting into separate jobs (e.g., a job for postgres tests and another for neo4j tests) and assigning the appropriate service in each job, or (B) remove the services section and conditionally start the required container with docker run inside a step that checks matrix.driver (use matrix.driver to decide whether to docker run postgres:16 or neo4j:4.4.42); update job dependencies and health/wait logic accordingly so tests run only after the selected DB is ready.
92-93: Named volumes may not work as expected in GitHub Actions services.GitHub Actions service containers are ephemeral and volumes declared this way don't persist between workflow runs. The
pg_dataandneo4j_datavolumes are unnecessary here since tests start withcleanDBanyway and don't rely on persistence.Consider removing the volume declarations to avoid potential confusion or unexpected behavior.
♻️ Proposed simplification
options: >- --health-cmd "pg_isready -U dawgs -d dawgs" --health-interval 10s --health-timeout 5s --health-retries 5 - volumes: - - pg_data:/var/lib/postgresql/data neo4j: image: neo4j:4.4.42 env: NEO4J_AUTH: neo4j/weneedbetterpasswords ports: - 7687:7687 options: >- --health-cmd "curl -f http://localhost:7474" --health-interval 10s --health-timeout 5s --health-retries 5 - volumes: - - neo4j_data:/dataAlso applies to: 105-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/go-test.yml around lines 92 - 93, The workflow defines named service volumes (volumes: - pg_data:/var/lib/postgresql/data and neo4j_data) which are unnecessary for ephemeral GitHub Actions service containers and may mislead; remove the volume declarations referencing pg_data and neo4j_data from the service definitions in the workflow file so the Postgres/Neo4j services run without persisted volumes (tests rely on cleanDB), leaving any other service config intact.
24-24: Remove temporary branch trigger before merging.The
PQE-401branch was added to test this workflow. Remember to remove it before merging tomain, or the workflow will continue triggering on that branch pattern after the PR is merged.♻️ Proposed removal
pull_request: branches: - "main" - "stage/**" - - "PQE-401" types:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/go-test.yml at line 24, The workflow is still configured to trigger on a temporary branch pattern "PQE-401"; remove that branch entry from the triggers in the workflow file so the workflow no longer listens for that temporary branch after merge. Locate the literal "PQE-401" in the workflow’s trigger list (the value currently in the array under on: push/branches or similar) and delete that string entry, leaving only the intended branch patterns (e.g., main) in the branches array.docker-compose.yml (1)
1-25: Consider adding health checks for local development consistency.The GitHub Actions workflow defines health checks for both services, but this local docker-compose.yml does not. Adding health checks here would ensure consistent behavior between local and CI environments, especially when scripts wait for services to be ready.
♻️ Proposed health checks for local parity
services: postgres: image: postgres:16 environment: POSTGRES_USER: dawg POSTGRES_PASSWORD: weneedbetterpasswords POSTGRES_DB: dawg ports: - "65432:5432" volumes: - pg_data:/var/lib/postgresql/data + healthcheck: + test: ["CMD", "pg_isready", "-U", "dawg", "-d", "dawg"] + interval: 10s + timeout: 5s + retries: 5 neo4j: image: neo4j:4.4.42 environment: NEO4J_AUTH: neo4j/weneedbetterpasswords ports: - "7687:7687" - "7474:7474" volumes: - neo4j_data:/data + healthcheck: + test: ["CMD", "curl", "-f", "http://localhost:7474"] + interval: 10s + timeout: 5s + retries: 5🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 1 - 25, Add Docker healthchecks to the postgres and neo4j services so local compose matches CI: for the postgres service (where POSTGRES_USER/POSTGRES_PASSWORD/POSTGRES_DB are set) add a healthcheck that runs pg_isready against port 5432 with a sensible interval/timeout/retries; for the neo4j service (where NEO4J_AUTH is set) add a healthcheck that probes the HTTP port 7474 (or Bolt 7687) using a simple curl or bolt/cypher check with basic auth, and set interval/timeout/retries so scripts that wait for healthy status behave the same locally as in GitHub Actions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ops/ops_test.go`:
- Around line 140-157: The WriteTransaction that calls DeleteRelationships on
testSuite.db ignores its returned error; add a check after the call to
testSuite.db.WriteTransaction (the block invoking DeleteRelationships) and fail
the test if err != nil (use require.NoError or require.Nil with t) so deletion
failures don't go unnoticed; keep the subsequent ReadTransaction and assertions
unchanged but only run them when the write error is nil.
- Around line 63-77: The Neo4j branch opens the database with dawgs.Open
(neo4j.DriverName) but never calls db.AssertSchema like the Postgres branch
does; after successfully creating db in the case "neo4j","bolt" block (after the
dawgs.Open and require.NoError(t, err)), add a call to
db.AssertSchema(context.Background(), testSchema) and handle the error similarly
to the Postgres branch (require.NoError) so the test schema is ensured for
Neo4j; reference symbols: case "neo4j","bolt", u.Scheme, connStr, dawgs.Open,
neo4j.DriverName, db, db.AssertSchema, testSchema.
---
Nitpick comments:
In @.github/workflows/go-test.yml:
- Around line 78-106: The workflow currently starts both services "postgres" and
"neo4j" for every matrix entry which wastes resources; change the workflow so
only the needed DB runs per matrix.driver by either (A) splitting into separate
jobs (e.g., a job for postgres tests and another for neo4j tests) and assigning
the appropriate service in each job, or (B) remove the services section and
conditionally start the required container with docker run inside a step that
checks matrix.driver (use matrix.driver to decide whether to docker run
postgres:16 or neo4j:4.4.42); update job dependencies and health/wait logic
accordingly so tests run only after the selected DB is ready.
- Around line 92-93: The workflow defines named service volumes (volumes: -
pg_data:/var/lib/postgresql/data and neo4j_data) which are unnecessary for
ephemeral GitHub Actions service containers and may mislead; remove the volume
declarations referencing pg_data and neo4j_data from the service definitions in
the workflow file so the Postgres/Neo4j services run without persisted volumes
(tests rely on cleanDB), leaving any other service config intact.
- Line 24: The workflow is still configured to trigger on a temporary branch
pattern "PQE-401"; remove that branch entry from the triggers in the workflow
file so the workflow no longer listens for that temporary branch after merge.
Locate the literal "PQE-401" in the workflow’s trigger list (the value currently
in the array under on: push/branches or similar) and delete that string entry,
leaving only the intended branch patterns (e.g., main) in the branches array.
In `@docker-compose.yml`:
- Around line 1-25: Add Docker healthchecks to the postgres and neo4j services
so local compose matches CI: for the postgres service (where
POSTGRES_USER/POSTGRES_PASSWORD/POSTGRES_DB are set) add a healthcheck that runs
pg_isready against port 5432 with a sensible interval/timeout/retries; for the
neo4j service (where NEO4J_AUTH is set) add a healthcheck that probes the HTTP
port 7474 (or Bolt 7687) using a simple curl or bolt/cypher check with basic
auth, and set interval/timeout/retries so scripts that wait for healthy status
behave the same locally as in GitHub Actions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3140a6c9-ee5c-49d7-9a38-712503c2653d
📒 Files selected for processing (3)
.github/workflows/go-test.ymldocker-compose.ymlops/ops_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/go-test.yml (2)
74-77: Consider using GitHub secrets for credentials.The connection strings contain hardcoded passwords. While these are ephemeral CI containers, using GitHub secrets is a cleaner approach and avoids static analysis warnings (Checkov CKV_SECRET_4).
Additionally, the environment variable
Driver_CONNECTION_STRING(line 116) uses mixed case. Convention is typicallySCREAMING_SNAKE_CASEfor environment variables (e.g.,DRIVER_CONNECTION_STRING).Example using secrets
# In repository secrets, define: # - TEST_PG_PASSWORD # - TEST_NEO4J_PASSWORD matrix: include: - driver: pg connection: postgres://dawgs:${{ secrets.TEST_PG_PASSWORD }}@localhost:5432/dawgs?sslmode=disable - driver: neo4j connection: bolt://neo4j:${{ secrets.TEST_NEO4J_PASSWORD }}@localhost:7687🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/go-test.yml around lines 74 - 77, Replace the hardcoded passwords in the matrix "connection" values for the job entries that set driver/connection (the pg and neo4j rows) to use GitHub Actions secrets (e.g., secrets.TEST_PG_PASSWORD and secrets.TEST_NEO4J_PASSWORD) instead of plain-text passwords, and rename the environment variable key currently referenced as Driver_CONNECTION_STRING to use SCREAMING_SNAKE_CASE (DRIVER_CONNECTION_STRING) throughout the workflow to follow env var conventions and avoid casing mismatches.
78-102: Both service containers start for every matrix entry.With the current setup, both Postgres and Neo4j containers are started for every matrix entry, even though only one is used per run. This doubles the resource consumption.
Consider either:
- Splitting into two separate jobs with explicit service definitions
- Using conditional service definitions (though GitHub Actions has limited support for this)
Option 1: Separate jobs
integration-test-pg: name: Run integration tests (pg) runs-on: ubuntu-latest services: postgres: image: postgres:16 # ... postgres config only steps: # ... integration-test-neo4j: name: Run integration tests (neo4j) runs-on: ubuntu-latest services: neo4j: image: neo4j:4.4.42 # ... neo4j config only steps: # ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/go-test.yml around lines 78 - 102, The workflow currently starts both service containers (services: postgres and neo4j) for every matrix entry, doubling resource use; split this into two jobs each defining only the required service (e.g., create jobs like integration-test-pg that defines only the postgres service and integration-test-neo4j that defines only the neo4j service) or gate service startup with job-level conditionals tied to the matrix value (use job "if" checks such as if: matrix.database == 'postgres' to only declare the postgres service, and similarly for neo4j); update any step references to the service host/port accordingly so tests run against the correct service.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/go-test.yml:
- Line 24: Remove the temporary branch trigger string "PQE-401" from the
workflow's branches trigger (the branches array under on.push/on.pull_request)
so the feature-branch-specific trigger is not included in the final workflow;
update the branches array to only include the intended long-term branch names
(e.g., main) and ensure no other temporary branch names remain.
- Around line 98-102: The health check uses curl which is not present in the
neo4j:4.4.42 image; update the Docker Compose/GitHub Actions service options
(the options key / --health-cmd) to use a tool present in the image—either
replace the curl command with a wget-based check (e.g., use wget -q --spider
http://localhost:7474/ || exit 1) or use cypher-shell to run a simple query
(e.g., cypher-shell -a bolt://localhost:7687 -u neo4j -p "$NEO4J_PASSWORD"
'RETURN 1' >/dev/null 2>&1 || exit 1) and keep the same --health-interval,
--health-timeout and --health-retries settings.
---
Nitpick comments:
In @.github/workflows/go-test.yml:
- Around line 74-77: Replace the hardcoded passwords in the matrix "connection"
values for the job entries that set driver/connection (the pg and neo4j rows) to
use GitHub Actions secrets (e.g., secrets.TEST_PG_PASSWORD and
secrets.TEST_NEO4J_PASSWORD) instead of plain-text passwords, and rename the
environment variable key currently referenced as Driver_CONNECTION_STRING to use
SCREAMING_SNAKE_CASE (DRIVER_CONNECTION_STRING) throughout the workflow to
follow env var conventions and avoid casing mismatches.
- Around line 78-102: The workflow currently starts both service containers
(services: postgres and neo4j) for every matrix entry, doubling resource use;
split this into two jobs each defining only the required service (e.g., create
jobs like integration-test-pg that defines only the postgres service and
integration-test-neo4j that defines only the neo4j service) or gate service
startup with job-level conditionals tied to the matrix value (use job "if"
checks such as if: matrix.database == 'postgres' to only declare the postgres
service, and similarly for neo4j); update any step references to the service
host/port accordingly so tests run against the correct service.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0da95224-bcda-4dd1-b430-d7a3cd55b6f0
📒 Files selected for processing (1)
.github/workflows/go-test.yml
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/go-test.yml (1)
24-24:⚠️ Potential issue | 🟡 MinorRemove PR-specific branch trigger before merge.
Line 24 still includes
PQE-401, which keeps this workflow tied to a feature branch after merge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/go-test.yml at line 24, Remove the PR-specific branch trigger string "PQE-401" from the workflow's branch trigger list so the GitHub Actions workflow is not tied to that feature branch after merge; locate the branch array where "PQE-401" is present (the branch trigger entries) and delete that entry (or replace it with the intended persistent branch names like main) to ensure the workflow triggers only on the correct branches.
🧹 Nitpick comments (1)
.github/workflows/go-test.yml (1)
75-77: Avoid embedding credentials directly in workflow YAML.Even for test containers, hardcoded credentials here trigger secret-scanning noise and can be copied into less safe contexts. Prefer job-level env vars (or repo variables) and reference them in both service env and matrix connection strings.
Also applies to: 82-85, 95-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/go-test.yml around lines 75 - 77, Replace hardcoded DB credentials in the GitHub Actions workflow by moving secrets into job-level environment variables and referencing those vars in service env and matrix connection strings; e.g., define env entries like POSTGRES_USER, POSTGRES_PASSWORD, NEO4J_USER, NEO4J_PASSWORD at the job level and then update the connection values (the postgres "connection: postgres://..." and neo4j "connection: bolt://...") and any other matrix entries to use the env variables (e.g., postgres://$POSTGRES_USER:$POSTGRES_PASSWORD@localhost:5432/dawgs?sslmode=disable) or use ${{ secrets.X }} if you prefer repo secrets, and do the same for the other occurrences mentioned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/go-test.yml:
- Around line 78-103: The workflow currently starts both services (services:
postgres and neo4j) for every matrix entry, causing matrix legs to interfere;
fix by splitting the work into separate jobs (e.g., test-postgres and
test-neo4j) or by making separate matrix jobs per backend so each job's services
include only the needed service. Update the job that currently defines both
postgres and neo4j (the go-test job / strategy.matrix) to instead create two
jobs—one with only the postgres service and one with only the neo4j service—and
reuse the same steps/steps template so each matrix leg runs isolated against its
intended backend (reference symbols: services, postgres, neo4j, strategy.matrix,
go-test job name). Ensure tests/steps still read the matrix variables (driver)
or adjust matrix entries per job accordingly.
- Around line 76-77: The Neo4j test matrix is using the wrong URL scheme
("bolt://...") which fails the scheme validation in drivers/neo4j/neo4j.go that
expects "neo4j"; update the connection URL in the matrix to use the "neo4j://"
scheme (e.g., change bolt://neo4j:password@localhost:7687 to
neo4j://neo4j:password@localhost:7687) so the scheme check in the code passes
and the Neo4j test leg can initialize correctly.
---
Duplicate comments:
In @.github/workflows/go-test.yml:
- Line 24: Remove the PR-specific branch trigger string "PQE-401" from the
workflow's branch trigger list so the GitHub Actions workflow is not tied to
that feature branch after merge; locate the branch array where "PQE-401" is
present (the branch trigger entries) and delete that entry (or replace it with
the intended persistent branch names like main) to ensure the workflow triggers
only on the correct branches.
---
Nitpick comments:
In @.github/workflows/go-test.yml:
- Around line 75-77: Replace hardcoded DB credentials in the GitHub Actions
workflow by moving secrets into job-level environment variables and referencing
those vars in service env and matrix connection strings; e.g., define env
entries like POSTGRES_USER, POSTGRES_PASSWORD, NEO4J_USER, NEO4J_PASSWORD at the
job level and then update the connection values (the postgres "connection:
postgres://..." and neo4j "connection: bolt://...") and any other matrix entries
to use the env variables (e.g.,
postgres://$POSTGRES_USER:$POSTGRES_PASSWORD@localhost:5432/dawgs?sslmode=disable)
or use ${{ secrets.X }} if you prefer repo secrets, and do the same for the
other occurrences mentioned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 50e616ad-2b15-411d-8bb8-956607f5ac6c
📒 Files selected for processing (1)
.github/workflows/go-test.yml
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/go-test.yml (1)
77-102:⚠️ Potential issue | 🟠 MajorMatrix legs are still coupled by always starting both databases.
At Line [77], both
postgresandneo4jservices are started for every matrix run. This means thepgleg can fail on Neo4j startup/health (and vice versa), which hurts reliability and adds avoidable runtime.Suggested direction (split into per-backend jobs)
- integration-test: - strategy: - matrix: - include: - - driver: pg - connection: postgres://... - - driver: neo4j - connection: bolt://... - services: - postgres: ... - neo4j: ... + integration-test-pg: + services: + postgres: ... + env: + Driver_CONNECTION_STRING: postgres://... + steps: + - run: go test -tags integration -v ./ops/... + + integration-test-neo4j: + services: + neo4j: ... + env: + Driver_CONNECTION_STRING: bolt://... + steps: + - run: go test -tags integration -v ./ops/...In GitHub Actions, when a job uses a matrix and defines multiple job-level services, are all services started for every matrix entry, or can services be skipped per entry without splitting jobs?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/go-test.yml around lines 77 - 102, The workflow currently starts both services ("postgres" and "neo4j") for every matrix entry, coupling matrix legs and letting one DB's startup/health affect the other; split the runs so each matrix/job only starts the service it needs by removing the global services block that lists both and instead create separate jobs (or matrix include entries) per backend (e.g., a job named test-postgres that defines services: postgres and a job named test-neo4j that defines services: neo4j) and drive them with strategy.matrix (or separate job entries) so "postgres" and "neo4j" are started only for their respective jobs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ops/ops_test.go`:
- Around line 65-67: The scheme rewrite for bolt is currently case-sensitive
(u.Scheme == "bolt") so mixed-case inputs like "BOLT://..." won't be rewritten;
update the check in ops_test.go to compare case-insensitively (e.g., use
strings.EqualFold(u.Scheme, "bolt") or strings.ToLower(u.Scheme) == "bolt")
before setting u.Scheme = "neo4j" and recomputing connStr; also add the strings
import if not present.
---
Duplicate comments:
In @.github/workflows/go-test.yml:
- Around line 77-102: The workflow currently starts both services ("postgres"
and "neo4j") for every matrix entry, coupling matrix legs and letting one DB's
startup/health affect the other; split the runs so each matrix/job only starts
the service it needs by removing the global services block that lists both and
instead create separate jobs (or matrix include entries) per backend (e.g., a
job named test-postgres that defines services: postgres and a job named
test-neo4j that defines services: neo4j) and drive them with strategy.matrix (or
separate job entries) so "postgres" and "neo4j" are started only for their
respective jobs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: abee189d-336b-4dd1-996a-a28f4741228f
📒 Files selected for processing (2)
.github/workflows/go-test.ymlops/ops_test.go
…y its own service container
Description
integration-testjob forpgandneo4jdrivers.Resolves: Closes: PQE-401
Type of Change
Testing
go test -tags manual_integration ./integration/...)Screenshots (if appropriate):
Driver Impact
drivers/pg)drivers/neo4j)Checklist
go.mod/go.sumare up to date if dependencies changedSummary by CodeRabbit
Tests
Chores