test(platform-api): convert cross-DB integration tests to godog and broaden coverage#2314
Open
AnuGayan wants to merge 7 commits into
Open
test(platform-api): convert cross-DB integration tests to godog and broaden coverage#2314AnuGayan wants to merge 7 commits into
AnuGayan wants to merge 7 commits into
Conversation
AnuGayan
requested review from
Arshardh,
CrowleyRajapakse,
HeshanSudarshana,
HiranyaKavishani,
Krishanx92,
PasanT9,
RakhithaRR,
Tharsanan1,
VirajSalaka,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
malinthaprasan,
pubudu538,
renuka-fernando,
senthuran16,
tgtshanika,
tharikaGitHub and
tharindu1st
as code owners
Contributor
|
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:
📝 WalkthroughUpdated the platform-api cross-database integration suite from Go table-driven tests to a Godog/Cucumber-based setup while keeping the existing integration build tag and Highlights:
WalkthroughThis PR replaces legacy Sequence Diagram(s)sequenceDiagram
participant TestMain
participant TestFeatures
participant initializeScenario
participant world
participant itDB
TestMain->>TestMain: set APIP_DEMO_MODE=true
TestFeatures->>initializeScenario: register scenario hooks
initializeScenario->>world: create per-scenario world
world->>itDB: openDB (Before hook)
world->>world: seedOrgProject / seedOrgGraph
initializeScenario->>world: register step groups (CRUD, cascade, pagination, LLM, MCP, secret, deployment)
world->>itDB: close (After hook)
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" 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 |
Contributor
Dependency Validation ResultsDependency name: github.com/cucumber/godog |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platform-api/src/internal/repository/secret.go (1)
245-251: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse a locking read on the SQL Server branch. The current
FetchFirstClausechange only makes the probe query dialect-safe; it does not preserve theFOR UPDATEsemantics from the Postgres path. InFindRefsAndSoftDelete, SQL Server can release the read lock before the later update, so the ref check and soft delete can race. UseWITH (UPDLOCK, HOLDLOCK)here, or fold the check and update into one guarded statement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/src/internal/repository/secret.go` around lines 245 - 251, The SQL Server branch in the secret lock query is only dialect-safe right now; it still lacks the locking behavior used by the Postgres path in FindRefsAndSoftDelete. Update the r.db.Driver() non-Postgres branch in secret.go so the probe query uses a locking read on SQL Server, such as WITH (UPDLOCK, HOLDLOCK), or otherwise combine the ref check and soft delete into one guarded statement. Keep the change centered around the lockQuery construction so the existing FindRefsAndSoftDelete flow preserves the intended lock semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform-api/src/internal/integration/steps_crud.go`:
- Around line 335-343: The revoked token assertion in revokedTokenState ignores
the status argument and always compares against GatewayTokenStatusRevoked.
Update revokedTokenState to validate the passed-in status value instead of
hardcoding REVOKED, and keep the existing revoked-by check and GetTokenByUUID
lookup in place so the scenario’s feature value is actually enforced.
- Line 74: The revoked-token step in revokedTokenState is ignoring its captured
status and always asserting GatewayTokenStatusRevoked. Update the assertion
logic to use the status value passed into revokedTokenState (and any helpers it
calls) so the check matches the step input, or remove the status parameter if
only revoked is supported. Locate the fix in revokedTokenState and the related
step definition that captures status.
In `@platform-api/src/internal/integration/steps_llm.go`:
- Around line 97-113: The createTemplateVersion helper is hardcoding the new
template handle suffix to "v2-0" instead of using the requested version
argument. Update the newHandle derivation in createTemplateVersion to build the
suffix from version (or reuse the same handle-formatting helper used by the
template service) so the step remains parameterized and consistent with
CreateNewVersion.
- Around line 179-184: The create-provider step in createLLMProvider currently
assumes repo.Create populated prov.UUID and assigns it directly to
w.providerUUID, so add an explicit UUID presence check immediately after
repo.Create succeeds, matching the validation already done in createLLMTemplate.
If prov.UUID is still empty, return a clear provider-creation error before
storing w.providerUUID and w.providerHandle, so the failure is caught in this
step rather than later at the proxy boundary.
In `@platform-api/src/internal/integration/steps_pagination.go`:
- Around line 396-421: The pageWebSubAPIs helper only fetches two pages with
fixed offsets, so it can miss records or fail when total is not exactly two full
pages. Update world.pageWebSubAPIs to iterate through all pages using
repository.NewWebSubAPIRepo(...).List with an advancing offset until all total
rows are covered, while preserving the existing duplicate/overlap checks. Keep
the page-size validations, but base them on the current chunk and allow the
final page to be smaller than pageSize.
In `@platform-api/src/internal/integration/steps_secret.go`:
- Around line 179-203: The rotation check in firstSecretRotated only verifies
Ciphertext, but rotateFirstSecret also updates Hash, so the test is missing part
of the contract. Update firstSecretRotated to fetch the secret and assert the
rotated Hash matches the value set in rotateFirstSecret, alongside the existing
ciphertext check, using the same repo/getFirstSecret flow and the
world.rotatedCipher state for reference.
In `@platform-api/src/internal/integration/steps_webbroker.go`:
- Around line 129-140: The update helper in updateFirstWebBrokerStatus only
writes through repo.Update and never verifies persistence, so the step can pass
on a no-op. After the Update call, read the WebBroker row back with
repo.GetByHandle using w.webbrokerHandles[0] and w.orgID, then assert the
persisted LifeCycleStatus matches the requested status before returning. Keep
the existing error handling pattern in world.updateFirstWebBrokerStatus and use
the repo/GetByHandle/Update flow to locate the fix.
In `@tests/integration-e2e/steps_test.go`:
- Around line 351-357: In chatResponseIsOpenAIShaped, replace the substring
checks on w.lastChatBody with actual JSON parsing of the completion payload.
Decode the response body into a structured type or map, then assert that object
equals "chat.completion" and that choices is a non-empty array with the expected
type. Keep the same error-returning step function, but make the validation
depend on parsed fields instead of raw byte contains checks.
---
Outside diff comments:
In `@platform-api/src/internal/repository/secret.go`:
- Around line 245-251: The SQL Server branch in the secret lock query is only
dialect-safe right now; it still lacks the locking behavior used by the Postgres
path in FindRefsAndSoftDelete. Update the r.db.Driver() non-Postgres branch in
secret.go so the probe query uses a locking read on SQL Server, such as WITH
(UPDLOCK, HOLDLOCK), or otherwise combine the ref check and soft delete into one
guarded statement. Keep the change centered around the lockQuery construction so
the existing FindRefsAndSoftDelete flow preserves the intended lock semantics.
🪄 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: 1728b64f-b4b6-4edf-aadf-beebe9ab6a72
⛔ Files ignored due to path filters (1)
platform-api/src/go.sumis excluded by!**/*.sum
📒 Files selected for processing (31)
platform-api/it/README.mdplatform-api/src/go.modplatform-api/src/internal/integration/cascade_test.goplatform-api/src/internal/integration/crud_test.goplatform-api/src/internal/integration/features/cascade-delete.featureplatform-api/src/internal/integration/features/llm-repository.featureplatform-api/src/internal/integration/features/mcp-repository.featureplatform-api/src/internal/integration/features/pagination-and-listing.featureplatform-api/src/internal/integration/features/repository-crud.featureplatform-api/src/internal/integration/features/secret-repository.featureplatform-api/src/internal/integration/features/webbroker-repository.featureplatform-api/src/internal/integration/harness.goplatform-api/src/internal/integration/lifecycle_test.goplatform-api/src/internal/integration/steps_cascade.goplatform-api/src/internal/integration/steps_crud.goplatform-api/src/internal/integration/steps_llm.goplatform-api/src/internal/integration/steps_mcp.goplatform-api/src/internal/integration/steps_pagination.goplatform-api/src/internal/integration/steps_secret.goplatform-api/src/internal/integration/steps_webbroker.goplatform-api/src/internal/integration/suite_test.goplatform-api/src/internal/integration/world.goplatform-api/src/internal/repository/secret.gotests/integration-e2e/README.mdtests/integration-e2e/docker-compose.sqlite.yamltests/integration-e2e/docker-compose.sqlserver.yamltests/integration-e2e/docker-compose.yamltests/integration-e2e/features/llm-deployment.featuretests/integration-e2e/mock-openai/openapi.yamltests/integration-e2e/steps_test.gotests/integration-e2e/suite_test.go
💤 Files with no reviewable changes (3)
- platform-api/src/internal/integration/crud_test.go
- platform-api/src/internal/integration/cascade_test.go
- platform-api/src/internal/integration/lifecycle_test.go
Comment on lines
+129
to
+140
| func (w *world) updateFirstWebBrokerStatus(status string) error { | ||
| repo := repository.NewWebBrokerAPIRepo(w.it.db) | ||
| got, err := repo.GetByHandle(w.webbrokerHandles[0], w.orgID) | ||
| if err != nil { | ||
| return fmt.Errorf("[%s] GetByHandle(%s) failed: %w", w.it.driver, w.webbrokerHandles[0], err) | ||
| } | ||
| got.LifeCycleStatus = status | ||
| got.UpdatedBy = "it-user" | ||
| if err := repo.Update(got); err != nil { | ||
| return fmt.Errorf("[%s] Update WebBroker API failed: %w", w.it.driver, err) | ||
| } | ||
| return nil |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Read the WebBroker row back after Update.
This step stops after repo.Update(got). If the update becomes a no-op, the scenario still passes because nothing asserts the persisted status.
Proposed fix
func (w *world) updateFirstWebBrokerStatus(status string) error {
repo := repository.NewWebBrokerAPIRepo(w.it.db)
got, err := repo.GetByHandle(w.webbrokerHandles[0], w.orgID)
if err != nil {
return fmt.Errorf("[%s] GetByHandle(%s) failed: %w", w.it.driver, w.webbrokerHandles[0], err)
}
got.LifeCycleStatus = status
got.UpdatedBy = "it-user"
if err := repo.Update(got); err != nil {
return fmt.Errorf("[%s] Update WebBroker API failed: %w", w.it.driver, err)
}
+ updated, err := repo.GetByHandle(w.webbrokerHandles[0], w.orgID)
+ if err != nil {
+ return fmt.Errorf("[%s] GetByHandle(%s) after update failed: %w", w.it.driver, w.webbrokerHandles[0], err)
+ }
+ if updated == nil || updated.LifeCycleStatus != status {
+ return fmt.Errorf("[%s] Update WebBroker API did not persist status %q", w.it.driver, status)
+ }
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (w *world) updateFirstWebBrokerStatus(status string) error { | |
| repo := repository.NewWebBrokerAPIRepo(w.it.db) | |
| got, err := repo.GetByHandle(w.webbrokerHandles[0], w.orgID) | |
| if err != nil { | |
| return fmt.Errorf("[%s] GetByHandle(%s) failed: %w", w.it.driver, w.webbrokerHandles[0], err) | |
| } | |
| got.LifeCycleStatus = status | |
| got.UpdatedBy = "it-user" | |
| if err := repo.Update(got); err != nil { | |
| return fmt.Errorf("[%s] Update WebBroker API failed: %w", w.it.driver, err) | |
| } | |
| return nil | |
| func (w *world) updateFirstWebBrokerStatus(status string) error { | |
| repo := repository.NewWebBrokerAPIRepo(w.it.db) | |
| got, err := repo.GetByHandle(w.webbrokerHandles[0], w.orgID) | |
| if err != nil { | |
| return fmt.Errorf("[%s] GetByHandle(%s) failed: %w", w.it.driver, w.webbrokerHandles[0], err) | |
| } | |
| got.LifeCycleStatus = status | |
| got.UpdatedBy = "it-user" | |
| if err := repo.Update(got); err != nil { | |
| return fmt.Errorf("[%s] Update WebBroker API failed: %w", w.it.driver, err) | |
| } | |
| updated, err := repo.GetByHandle(w.webbrokerHandles[0], w.orgID) | |
| if err != nil { | |
| return fmt.Errorf("[%s] GetByHandle(%s) after update failed: %w", w.it.driver, w.webbrokerHandles[0], err) | |
| } | |
| if updated == nil || updated.LifeCycleStatus != status { | |
| return fmt.Errorf("[%s] Update WebBroker API did not persist status %q", w.it.driver, status) | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform-api/src/internal/integration/steps_webbroker.go` around lines 129 -
140, The update helper in updateFirstWebBrokerStatus only writes through
repo.Update and never verifies persistence, so the step can pass on a no-op.
After the Update call, read the WebBroker row back with repo.GetByHandle using
w.webbrokerHandles[0] and w.orgID, then assert the persisted LifeCycleStatus
matches the requested status before returning. Keep the existing error handling
pattern in world.updateFirstWebBrokerStatus and use the repo/GetByHandle/Update
flow to locate the fix.
7778d37 to
e3e04af
Compare
Contributor
Author
|
Pushed fixes for the CodeRabbit comments and the failing e2e builds. CI: the CodeRabbit comments:
The WebBroker Re-verified on SQLite: 21 scenarios / 157 steps pass; |
Contributor
Dependency Validation ResultsDependency name: github.com/cucumber/godog |
AnuGayan
force-pushed
the
it/cucumber-cross-db-tests
branch
from
e3e04af to
1539d31
Compare
Contributor
Dependency Validation ResultsDependency name: github.com/cucumber/godog |
AnuGayan
force-pushed
the
it/cucumber-cross-db-tests
branch
from
1539d31 to
501d5ff
Compare
Contributor
Dependency Validation ResultsDependency name: github.com/cucumber/godog |
Contributor
Author
|
Update on the e2e jobs. The
The remaining sqlserver failure is a pre-existing, server-side SQL Server bug, unrelated to this PR: the existing api-deployment scenario's undeploy step returns Suggested follow-up (separate PR): fix the SQL Server incompatibility in the deployment undeploy path and add a Note: a combined platform-api -> gateway LLM-proxy deployment e2e scenario was prototyped but descoped from this PR; the deploy-to-serve path returned 404 at the gateway and needs gateway-internals work better suited to its own change. |
…roaden coverage Convert the platform-api cross-database integration tests from Go table-tests to a godog (Cucumber) suite, keeping the `integration` build tag and the existing `make it-*` targets, and extend coverage to several previously-untested repositories. Cross-DB suite (platform-api/src/internal/integration): - Replace crud/cascade/lifecycle/harness `*_test.go` with Gherkin features + step definitions and an error-returning harness; each scenario runs a fresh DB and surfaces as a `go test` subtest via godog's TestingT. - New scenarios for the LLM control plane (provider template / provider / proxy), MCP proxy, WebBroker API, and the secret store. The LLM and MCP scenarios store a dummy upstream API key, so no real LLM/MCP or credential is needed on any engine. - Carry over the recent upstream additions to the old table-tests: the application_api_key_mappings / application_artifact_mappings renames and the subscription-plan-limit cascade. - Add godog v0.15.0. Fix (internal/repository/secret.go): - The secret scenario surfaced a latent SQL Server bug where the FindRefsAndSoftDelete row-lock used the LIMIT keyword (rejected by SQL Server); it now uses the dialect-aware FetchFirstClause helper. Verified on SQLite (21 scenarios / 157 steps). Postgres / SQL Server require Docker + built images (`make it-*`). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
AnuGayan
force-pushed
the
it/cucumber-cross-db-tests
branch
from
501d5ff to
2836f67
Compare
Contributor
Dependency Validation ResultsDependency name: github.com/cucumber/godog |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Drive the deployment repository through its real methods so the dialect-specific SQL is verified on every engine — not just the SQLite unit-test path. Two scenarios cover: - Create / current-status / current-lookup / list / undeploy lifecycle: CreateWithLimitEnforcement (deployment_status upsert via BuildUpsertQuery), GetStatus, HasActiveDeployment, GetCurrentByGateway (FETCH-first), GetWithContent (content round-trip), GetDeploymentsWithState (the ROW_NUMBER() OVER per-gateway ranking), and the UNDEPLOYED transition via SetCurrentWithDetails. - Hard-limit enforcement: creating beyond the limit prunes the oldest archived deployments (the FETCH-first + delete path) and keeps the stored count bounded. This is the repository whose SQL Server undeploy failure was fixed upstream and was previously only covered by the delete-cascade scenario. Verified on SQLite (23 scenarios / 176 steps). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Dependency Validation ResultsDependency name: github.com/cucumber/godog |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
platform-api/src/internal/integration/steps_deployment.go (1)
192-200: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert deployment statuses in the list step.
This step only checks
len(list), so it would miss a regression where archived deployments come back with a nil or wrongStatus. Add assertions that each returned deployment has a non-nilStatus, and that archived rows are surfaced asARCHIVEDwhen older deployments are present. Based on learnings, deployment retrieval paths likeGetDeploymentsWithStateshould never return a nilStatus, and archived rows should be materialized asARCHIVED.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/src/internal/integration/steps_deployment.go` around lines 192 - 200, The listDeploymentsWithState check only validates count, so update it to also inspect each deployment returned by repository.NewDeploymentRepo(...).GetDeploymentsWithState and assert deployment.Status is never nil. Use the existing listDeploymentsWithState helper to add expectations for archived results too, ensuring older deployments are surfaced with Status set to ARCHIVED rather than nil or another value.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform-api/src/internal/integration/harness.go`:
- Around line 150-154: The bootstrap connection in connectITDB for the master
database is hardcoding the SQL Server SSL setting instead of using the
configured environment value. Update the config.Database setup in harness.go to
use the same env-backed SSL mode as the scenario connection, matching the
existing envOr-driven pattern used for the other database connection fields so
provisioning and test execution stay consistent.
---
Nitpick comments:
In `@platform-api/src/internal/integration/steps_deployment.go`:
- Around line 192-200: The listDeploymentsWithState check only validates count,
so update it to also inspect each deployment returned by
repository.NewDeploymentRepo(...).GetDeploymentsWithState and assert
deployment.Status is never nil. Use the existing listDeploymentsWithState helper
to add expectations for archived results too, ensuring older deployments are
surfaced with Status set to ARCHIVED rather than nil or another value.
🪄 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: 5aa21dfd-1f33-4cd8-87f5-af758e46cdc7
⛔ Files ignored due to path filters (1)
platform-api/src/go.sumis excluded by!**/*.sum
📒 Files selected for processing (25)
platform-api/it/README.mdplatform-api/src/go.modplatform-api/src/internal/integration/cascade_test.goplatform-api/src/internal/integration/crud_test.goplatform-api/src/internal/integration/features/cascade-delete.featureplatform-api/src/internal/integration/features/deployment-repository.featureplatform-api/src/internal/integration/features/llm-repository.featureplatform-api/src/internal/integration/features/mcp-repository.featureplatform-api/src/internal/integration/features/pagination-and-listing.featureplatform-api/src/internal/integration/features/repository-crud.featureplatform-api/src/internal/integration/features/secret-repository.featureplatform-api/src/internal/integration/features/webbroker-repository.featureplatform-api/src/internal/integration/harness.goplatform-api/src/internal/integration/lifecycle_test.goplatform-api/src/internal/integration/steps_cascade.goplatform-api/src/internal/integration/steps_crud.goplatform-api/src/internal/integration/steps_deployment.goplatform-api/src/internal/integration/steps_llm.goplatform-api/src/internal/integration/steps_mcp.goplatform-api/src/internal/integration/steps_pagination.goplatform-api/src/internal/integration/steps_secret.goplatform-api/src/internal/integration/steps_webbroker.goplatform-api/src/internal/integration/suite_test.goplatform-api/src/internal/integration/world.goplatform-api/src/internal/repository/secret.go
💤 Files with no reviewable changes (3)
- platform-api/src/internal/integration/crud_test.go
- platform-api/src/internal/integration/lifecycle_test.go
- platform-api/src/internal/integration/cascade_test.go
✅ Files skipped from review due to trivial changes (1)
- platform-api/it/README.md
🚧 Files skipped from review as they are similar to previous changes (18)
- platform-api/src/internal/integration/features/mcp-repository.feature
- platform-api/src/internal/integration/features/webbroker-repository.feature
- platform-api/src/internal/integration/features/secret-repository.feature
- platform-api/src/internal/integration/features/pagination-and-listing.feature
- platform-api/src/internal/integration/features/repository-crud.feature
- platform-api/src/internal/integration/features/cascade-delete.feature
- platform-api/src/internal/integration/steps_llm.go
- platform-api/src/internal/integration/features/llm-repository.feature
- platform-api/src/internal/integration/steps_mcp.go
- platform-api/src/internal/integration/suite_test.go
- platform-api/src/internal/integration/steps_webbroker.go
- platform-api/src/internal/integration/world.go
- platform-api/src/internal/integration/steps_pagination.go
- platform-api/src/internal/integration/steps_secret.go
- platform-api/src/internal/repository/secret.go
- platform-api/src/internal/integration/steps_crud.go
- platform-api/src/go.mod
- platform-api/src/internal/integration/steps_cascade.go
Comment on lines
+150
to
154
| master, err := connectITDB(&config.Database{ | ||
| Driver: "sqlserver", Host: envOr("IT_DB_HOST", "localhost"), Port: atoiOr("IT_DB_PORT", 1433), | ||
| Name: "master", User: envOr("IT_DB_USER", "sa"), Password: os.Getenv("IT_DB_PASSWORD"), | ||
| SSLMode: "disable", MaxOpenConns: 2, MaxIdleConns: 1, | ||
| }, logger) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use the configured SQL Server SSL mode for the bootstrap connection.
Line 153 hardcodes SSLMode: "disable" while the scenario SQL Server connection honors IT_DB_SSLMODE. Use the same env-backed value here so database provisioning and test execution use consistent connection settings.
Proposed fix
- SSLMode: "disable", MaxOpenConns: 2, MaxIdleConns: 1,
+ SSLMode: envOr("IT_DB_SSLMODE", "disable"), MaxOpenConns: 2, MaxIdleConns: 1,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| master, err := connectITDB(&config.Database{ | |
| Driver: "sqlserver", Host: envOr("IT_DB_HOST", "localhost"), Port: atoiOr("IT_DB_PORT", 1433), | |
| Name: "master", User: envOr("IT_DB_USER", "sa"), Password: os.Getenv("IT_DB_PASSWORD"), | |
| SSLMode: "disable", MaxOpenConns: 2, MaxIdleConns: 1, | |
| }, logger) | |
| master, err := connectITDB(&config.Database{ | |
| Driver: "sqlserver", Host: envOr("IT_DB_HOST", "localhost"), Port: atoiOr("IT_DB_PORT", 1433), | |
| Name: "master", User: envOr("IT_DB_USER", "sa"), Password: os.Getenv("IT_DB_PASSWORD"), | |
| SSLMode: envOr("IT_DB_SSLMODE", "disable"), MaxOpenConns: 2, MaxIdleConns: 1, | |
| }, logger) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform-api/src/internal/integration/harness.go` around lines 150 - 154, The
bootstrap connection in connectITDB for the master database is hardcoding the
SQL Server SSL setting instead of using the configured environment value. Update
the config.Database setup in harness.go to use the same env-backed SSL mode as
the scenario connection, matching the existing envOr-driven pattern used for the
other database connection fields so provisioning and test execution stay
consistent.
…ucumber-cross-db-tests # Conflicts: # platform-api/src/internal/integration/cascade_test.go # platform-api/src/internal/integration/crud_test.go # platform-api/src/internal/integration/lifecycle_test.go
Combined platform-api<->gateway e2e (postgres-only extended suites): gateway restart/recovery, secret resolution, control-plane restart + auto-reconnect, deployment lifecycle (update/delete/restore), API-key auth, and MCP-proxy deployment. LLM-provider and LLM-proxy scenarios quarantined @wip (open provider template-apiVersion bug). Default run: 20/20 scenarios green. Encryption-key unit/integration tests this surfaced: - platform-api config: demo-mode ephemeral secret key breaks decryption across restart (+ stable-key contrast); subscription-token key shares it. - platform-api InHouseVault: round-trip, wrong-key, tampered ciphertext. - gateway aesgcm: key persists across restart; rotation across versions. - gateway controlplane: deprecated secret not purged (characterization). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Dependency Validation ResultsDependency name: github.com/cucumber/godog |
Contributor
Author
|
@copilot resolve the merge conflicts in this pull request |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates platform-api’s cross-database integration tests (under the integration build tag) from Go table-tests to a godog (Cucumber) suite, and expands the gateway e2e godog suite with additional scenarios (secrets, lifecycle, API keys, MCP, LLM). It also includes a targeted SQL Server compatibility fix in SecretRepo surfaced by the new secret scenarios, plus several new tests around encryption-key behavior.
Changes:
- Replaced
platform-api/src/internal/integration/*_test.gotable-tests withfeatures/*.feature+ step definitions run viagodog.TestSuite(per-scenario world/harness). - Expanded
tests/integration-e2egodog suite with new feature files and helpers (secrets, restart/recovery, lifecycle, API-key auth, MCP/LLM flows), plus docker-compose updates. - Fixed SQL Server query incompatibility in
SecretRepo.FindRefsAndSoftDeleteby using the dialect-awareFetchFirstClause(and added/adjusted related tests).
Reviewed changes
Copilot reviewed 53 out of 66 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration-e2e/suite_test.go | Updates tag defaults and adds helper functions for new e2e scenarios (secrets, lifecycle, API keys, MCP/LLM). |
| tests/integration-e2e/README.md | Documents the expanded e2e feature set, tags, and restart/secret semantics. |
| tests/integration-e2e/features/secret.feature | Adds secret propagation scenarios (postgres-only). |
| tests/integration-e2e/features/platform-api-restart.feature | Adds control-plane restart scenarios (postgres-only). |
| tests/integration-e2e/features/mcp-proxy.feature | Adds MCP proxy e2e scenario. |
| tests/integration-e2e/features/llm-proxy.feature | Adds quarantined (@wip) LLM proxy e2e scenario. |
| tests/integration-e2e/features/llm-provider-secret.feature | Adds quarantined (@wip) LLM-provider secret e2e scenarios + bug explanation. |
| tests/integration-e2e/features/lifecycle.feature | Adds REST API update/delete/restore lifecycle e2e scenarios. |
| tests/integration-e2e/features/gateway-restart.feature | Adds gateway restart/recovery scenarios (postgres-only). |
| tests/integration-e2e/features/api-key-auth.feature | Adds API-key auth end-to-end scenario. |
| tests/integration-e2e/docker-compose.yaml | Adds stable secret encryption key + on-demand MCP backend service. |
| tests/integration-e2e/docker-compose.sqlserver.yaml | Adds stable secret encryption key for SQL Server stack. |
| tests/integration-e2e/docker-compose.sqlite.yaml | Adds stable secret encryption key for SQLite stack. |
| portals/ai-workspace/bff/internal/server/composite_handlers_test.go | Formatting adjustments in tests around fake platform API responses. |
| platform-api/src/internal/vault/vault_test.go | Adds unit tests for in-house vault behavior (round-trip, wrong key, tamper). |
| platform-api/src/internal/utils/subscription_token_crypto_test.go | Adds tests for encryption-key derivation and token encrypt/decrypt behaviors. |
| platform-api/src/internal/utils/llm_provider_template_loader.go | Formatting-only changes in template loader struct literal. |
| platform-api/src/internal/service/secret_service_test.go | Minor formatting/spacing changes in mock/test definitions. |
| platform-api/src/internal/service/mcp.go | Formatting-only change. |
| platform-api/src/internal/service/llm.go | Formatting-only change + removes stray blank line. |
| platform-api/src/internal/service/llm_test.go | Formatting-only changes in helper request builders. |
| platform-api/src/internal/service/llm_provider_template_test.go | Formatting-only changes in test request setup. |
| platform-api/src/internal/service/custom_policy_test.go | Formatting/comment whitespace cleanup in tests. |
| platform-api/src/internal/service/artifact_import_lifecycle_test.go | Removes trailing whitespace line. |
| platform-api/src/internal/service/artifact_cross_origin_test.go | Formatting-only changes in test struct literals. |
| platform-api/src/internal/repository/secret.go | Uses dialect-aware pagination/row limiting for SQL Server (FetchFirstClause) in secret soft-delete lock query. |
| platform-api/src/internal/repository/secret_test.go | Minor formatting; adjusts rune math expression formatting. |
| platform-api/src/internal/repository/deployment.go | Whitespace around string concatenation in query. |
| platform-api/src/internal/repository/api_test.go | Removes trailing whitespace line. |
| platform-api/src/internal/model/mcp.go | Formatting/alignment in struct fields. |
| platform-api/src/internal/model/llm.go | Formatting/alignment in struct fields. |
| platform-api/src/internal/model/artifact.go | Formatting/alignment in struct fields. |
| platform-api/src/internal/model/api.go | Formatting/alignment in struct fields. |
| platform-api/src/internal/integration/world.go | New per-scenario world state and DB seeding helpers for godog integration suite. |
| platform-api/src/internal/integration/websub_cascade_test.go | Updates experimental cascade test helpers to new harness helpers. |
| platform-api/src/internal/integration/suite_test.go | Adds godog runner + step registration + per-scenario lifecycle. |
| platform-api/src/internal/integration/steps_secret.go | Adds secret repository step definitions. |
| platform-api/src/internal/integration/steps_pagination.go | Adds pagination/listing step definitions across repos. |
| platform-api/src/internal/integration/steps_mcp.go | Adds MCP proxy repository step definitions. |
| platform-api/src/internal/integration/steps_llm.go | Adds LLM template/provider/proxy repository step definitions. |
| platform-api/src/internal/integration/steps_deployment.go | Adds deployment repository step definitions (current/status/list/undeploy/limits). |
| platform-api/src/internal/integration/steps_cascade.go | Adds cascade-delete step definitions. |
| platform-api/src/internal/integration/lifecycle_test.go | Removes legacy Go lifecycle table-tests (replaced by godog features). |
| platform-api/src/internal/integration/harness.go | Refactors DB harness to return errors + per-scenario cleanup; adds SQL Server DB recreation helper. |
| platform-api/src/internal/integration/harness_experimental_test.go | Adapts experimental integration helpers to new harness signatures. |
| platform-api/src/internal/integration/features/secret-repository.feature | Adds secret repository cross-DB feature. |
| platform-api/src/internal/integration/features/repository-crud.feature | Adds CRUD cross-DB features (API/gateway/api-keys). |
| platform-api/src/internal/integration/features/pagination-and-listing.feature | Adds pagination/filtering cross-DB features. |
| platform-api/src/internal/integration/features/mcp-repository.feature | Adds MCP repository cross-DB feature. |
| platform-api/src/internal/integration/features/llm-repository.feature | Adds LLM repository cross-DB feature. |
| platform-api/src/internal/integration/features/deployment-repository.feature | Adds deployment repository cross-DB feature. |
| platform-api/src/internal/integration/features/cascade-delete.feature | Adds cascade-delete cross-DB feature. |
| platform-api/src/internal/integration/crud_test.go | Removes legacy Go CRUD table-tests (replaced by godog features). |
| platform-api/src/internal/integration/cascade_test.go | Removes legacy Go cascade table-tests (replaced by godog features). |
| platform-api/src/internal/dto/api.go | Removes trailing whitespace line. |
| platform-api/src/internal/constants/error.go | Formatting cleanup for ErrHandleImmutable. |
| platform-api/src/go.sum | Adds dependencies for godog and transitives. |
| platform-api/src/go.mod | Adds github.com/cucumber/godog v0.15.0 and indirect deps. |
| platform-api/src/config/secret_encryption_key_restart_test.go | Adds tests reproducing demo-mode ephemeral key restart behavior and stable-key behavior. |
| platform-api/src/config/config.go | Removes stray blank line. |
| platform-api/it/README.md | Updates documentation for new godog-based cross-DB integration suite layout and scope. |
| gateway/gateway-controller/pkg/encryption/aesgcm/key_rotation_test.go | Adds tests for key rotation behavior in gateway encryption provider. |
| gateway/gateway-controller/pkg/encryption/aesgcm/key_restart_test.go | Adds tests for key persistence across restart in gateway dev mode. |
| gateway/gateway-controller/pkg/controlplane/sync_secrets_deprecate_test.go | Adds test documenting incremental sync behavior for deprecated secrets (known gap). |
Comment on lines
+30
to
+33
| // world holds the state of a single scenario: the database under test plus the | ||
| // identifiers and intermediate results threaded between its Given/When/Then | ||
| // steps. A fresh world (and a fresh database) is created per scenario so the | ||
| // scenarios stay independent. |
Comment on lines
+58
to
+60
| // initializeScenario wires a fresh world (and a fresh database) per scenario and | ||
| // registers every step group, so scenarios stay independent. | ||
| func initializeScenario(ctx *godog.ScenarioContext) { |
Comment on lines
31
to
+35
| The tests run on the host and connect to the database over a published port, so | ||
| the same test binary covers all three engines. | ||
| the same test binary covers all three engines. A fresh database and a fresh | ||
| scenario state are created per scenario, so scenarios are independent. Each | ||
| scenario also surfaces as an individual `go test` subtest via godog's `TestingT` | ||
| integration, so `-run TestFeatures/<scenario>` works. |
Comment on lines
+82
to
+85
| // The second gateway is only wired on the postgres stack; the | ||
| // restart/recovery, secret and lifecycle scenarios are validated | ||
| // postgres-only. Only the base api-deployment feature runs cross-DB. | ||
| tags += " && ~@multigateway && ~@restart && ~@secret && ~@lifecycle && ~@apikey && ~@mcp" |
…ucumber-cross-db-tests # Conflicts: # platform-api/src/config/config.go
… repo Demo-mode LoadConfig now persists the secret encryption key to DATABASE_SECRET_ENCRYPTION_KEY_FILE (defaulting to data/secret-encryption.key next to the DB path), so any test that ran it without overriding that path left an untracked platform-api/src/config/data/secret-encryption.key behind after every 'go test'. Point the four demo-mode tests at per-test temp paths: the ephemeral-key test uses the existing blocking-file pattern so the key really stays ephemeral, the other three use a t.TempDir() key file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ive deps The last Go usage of pb33f/libopenapi was removed in 635f7ae but go.mod was never tidied, leaving it and five transitive deps (generic-list-go, jsonparser, pb33f/jsonpath, pb33f/ordered-map, go.yaml.in/yaml/v4) dangling. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Dependency Validation ResultsDependency name: github.com/cucumber/godog |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
platform-api/src/config/config_test.go (1)
41-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated "blocking directory" setup into a helper.
The same 3-line pattern (write a file, then target
<file>/secret-encryption.keyas the key path to force a directory-creation failure) is duplicated across three tests. Extracting a small helper (e.g.,newBlockedKeyFilePath(t) string) would reduce duplication and centralize the trick if the underlyingos.MkdirAllfailure semantics ever need adjusting.♻️ Suggested helper
+// newBlockedKeyFilePath returns a key-file path whose parent cannot be created, +// forcing LoadConfig's key-file initialization to fail. +func newBlockedKeyFilePath(t *testing.T) string { + t.Helper() + blockingFile := filepath.Join(t.TempDir(), "not-a-dir") + require.NoError(t, os.WriteFile(blockingFile, []byte("block"), 0600)) + return filepath.Join(blockingFile, "secret-encryption.key") +}Also applies to: 62-64, 93-95
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/src/config/config_test.go` around lines 41 - 43, The repeated “blocking directory” setup used in the config tests should be extracted into a shared helper to avoid duplication and keep the directory-failure trick in one place. Add a small helper in config_test.go, such as newBlockedKeyFilePath(t), that creates the blocking file and returns the nested secret-encryption.key path, then replace the repeated inline setup in the relevant tests with calls to that helper. Keep the helper near the tests that use DATABASE_SECRET_ENCRYPTION_KEY_FILE so the intent stays obvious and any future os.MkdirAll-related adjustment only needs to be made once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@platform-api/src/config/config_test.go`:
- Around line 41-43: The repeated “blocking directory” setup used in the config
tests should be extracted into a shared helper to avoid duplication and keep the
directory-failure trick in one place. Add a small helper in config_test.go, such
as newBlockedKeyFilePath(t), that creates the blocking file and returns the
nested secret-encryption.key path, then replace the repeated inline setup in the
relevant tests with calls to that helper. Keep the helper near the tests that
use DATABASE_SECRET_ENCRYPTION_KEY_FILE so the intent stays obvious and any
future os.MkdirAll-related adjustment only needs to be made once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab453e6c-76a7-447c-b65f-b6299a66e803
⛔ Files ignored due to path filters (1)
platform-api/src/go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
platform-api/src/config/config_test.goplatform-api/src/config/secret_encryption_key_restart_test.goplatform-api/src/go.mod
💤 Files with no reviewable changes (1)
- platform-api/src/go.mod
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Converts the platform-api cross-database integration tests from Go table-tests to a godog (Cucumber) suite, keeping the
integrationbuild tag and the existingmake it-*targets, and extends coverage to several previously-untested repositories. Also fixes one latent SQL Server bug surfaced by the new secret tests.Cross-DB suite (
platform-api/src/internal/integration)crud/cascade/lifecycle/harness*_test.gowith Gherkinfeatures/+ step definitions and an error-returning harness. Each scenario runs against a fresh DB and surfaces as ago testsubtest via godog'sTestingT.DeploymentRepothrough its real methods so the dialect-specific SQL runs on every engine:CreateWithLimitEnforcement(thedeployment_statusupsert + FETCH-first hard-limit pruning),GetCurrentByGateway(FETCH-first),GetDeploymentsWithState(theROW_NUMBER() OVERranking),GetStatus,HasActiveDeployment,GetWithContent, and the undeploy transition viaSetCurrentWithDetails— the path whose SQL Server undeploy failure was fixed upstream.application_api_key_mappings/application_artifact_mappingsrenames and the subscription-plan-limit cascade.godog v0.15.0.Fix (
internal/repository/secret.go)The secret scenario surfaced a latent SQL Server bug where the
FindRefsAndSoftDeleterow-lock used theLIMITkeyword (rejected by SQL Server). It now uses the dialect-awareFetchFirstClausehelper. (SecretRepo.Listwas independently fixed upstream with the samePaginationClauseapproach.)Test plan
IT_DB=sqlite make it— 23 scenarios / 176 steps pass; existinginternal/repositoryunit tests pass;go vet -tags integrationand default build clean.pr-check/pr-check-postgres/pr-check-sqlserver(the cross-DB repo tests) green.make it-postgres/make it-sqlserverneed Docker + built images (verify thesecret.goSQL Server fix locally).🤖 Generated with Claude Code