Admin runtime tests #8667
Admin runtime tests #8667
Conversation
| u1 := testcli.New(t, adm, c.Token) | ||
|
|
||
| result := u1.Run(t, "org", "create", "reload-configs-test") | ||
| require.Equal(t, 0, result.ExitCode) | ||
|
|
||
| // deploy the project | ||
| tempDir := initRillProject(t) | ||
| result = u1.Run(t, "project", "deploy", "--interactive=false", "--org=reload-configs-test", "--project=rill-mgd-deploy", "--path="+tempDir) |
There was a problem hiding this comment.
Can this test be moved into the cli package? Since it's somewhat unexpected to see CLI tests in the admin package (the cli package is kind of the "parent" of the admin package – cli already imports admin, but not the other way).
| func triggerDeployment(t *testing.T, adm *testadmin.Fixture, org, project string) *database.Deployment { | ||
| proj, err := adm.Admin.DB.FindProjectByName(t.Context(), org, project) | ||
| require.NoError(t, err) | ||
| depl, err := adm.Admin.DB.FindDeploymentsForProject(t.Context(), proj.ID, "", "") | ||
| require.NoError(t, err) | ||
| require.Len(t, depl, 1) | ||
| err = river.NewReconcileDeploymentWorker(adm.Admin).Work(t.Context(), &riverqueue.Job[river.ReconcileDeploymentArgs]{ | ||
| Args: river.ReconcileDeploymentArgs{ | ||
| DeploymentID: depl[0].ID, | ||
| }, | ||
| }) | ||
| require.NoError(t, err) | ||
| depl, err = adm.Admin.DB.FindDeploymentsForProject(t.Context(), proj.ID, "", "") | ||
| require.NoError(t, err) | ||
| require.Len(t, depl, 1) | ||
| return depl[0] | ||
| } |
There was a problem hiding this comment.
This seems generally useful, would it make sense to move it to testadmin.Fixture? Like adm.ReconcileDeployment(t, org, project)
| return depl[0] | ||
| } | ||
|
|
||
| func initRillProject(t *testing.T) string { |
There was a problem hiding this comment.
This function is only called once – consider inlining it. For test projects of only a few lines, it's anyway nice to have it directly inlined in the tests so changing the files for one test doesn't impact other tests.
There was a problem hiding this comment.
Pull request overview
This PR adds automated integration tests for admin runtime deployment flows. The changes enable testing of deployment lifecycle operations including project deployment, environment variable updates, stopping, and restarting deployments.
Changes:
- Enhanced test runtime to conditionally enable config reloader based on host access settings
- Extended testadmin fixture to optionally include an embedded runtime server for integration testing
- Exposed ReconcileDeploymentWorker constructor for direct invocation in tests
- Added comprehensive integration test covering deployment lifecycle scenarios
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| runtime/testruntime/testruntime.go | Enables config reloader when host access is disabled, ensuring proper configuration updates during tests |
| admin/testadmin/testadmin.go | Adds optional runtime server support to test fixtures with dual-port allocation and proper server initialization |
| admin/jobs/river/reconcile_deployment.go | Exports worker constructor for test usage to trigger deployment reconciliation directly |
| admin/deployments_test.go | New comprehensive integration test validating deployment, environment updates, stop/restart flows |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| result = u1.Run(t, "env", "set", "limit", "20", "--org=reload-configs-test", "--project=rill-mgd-deploy") | ||
| require.Equal(t, 0, result.ExitCode, result.Output) | ||
|
|
||
| // restart the deployment - use the api direclty since the CLI commands wait for deployment to be running which is not possible without river workers |
There was a problem hiding this comment.
Spelling error: "direclty" should be "directly".
| // restart the deployment - use the api direclty since the CLI commands wait for deployment to be running which is not possible without river workers | |
| // restart the deployment - use the api directly since the CLI commands wait for deployment to be running which is not possible without river workers |
| } | ||
|
|
||
| // RuntimeURL returns the URL where the embedded runtime is accessible. | ||
| func (f *Fixture) RuntimeURL() string { |
There was a problem hiding this comment.
The RuntimeURL() method will panic with a nil pointer dereference if called on a Fixture created with startRt=false, because RuntimeServerOpts will be nil. Consider adding a check to return an error or empty string, or document that this method should only be called when the fixture was created with NewWithOptionalRuntime(t, true).
| func (f *Fixture) RuntimeURL() string { | |
| func (f *Fixture) RuntimeURL() string { | |
| if f.RuntimeServerOpts == nil { | |
| return "" | |
| } |
| if err == nil && resp.StatusCode == http.StatusOK { | ||
| resp.Body.Close() | ||
| return true | ||
| } | ||
| return false |
There was a problem hiding this comment.
Potential resource leak: The HTTP response body should be closed even when the status code is not OK or when there's an error. Currently, if err is not nil or if the status code is not OK, the response body may not be closed. Consider closing the response body in all cases where resp is not nil.
| if err == nil && resp.StatusCode == http.StatusOK { | |
| resp.Body.Close() | |
| return true | |
| } | |
| return false | |
| if err != nil { | |
| return false | |
| } | |
| defer resp.Body.Close() | |
| return resp.StatusCode == http.StatusOK |
closes https://linear.app/rilldata/issue/PLAT-198/add-automated-tests-for-deploy-flows
Checklist: