Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added automatically generated acceptance tests for CLI commands #2501

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Added automatically generated acceptance tests for CLI commands #2501

wants to merge 6 commits into from

Conversation

andrewnester
Copy link
Contributor

@andrewnester andrewnester commented Mar 14, 2025 •
edited
Loading

Changes

Added automatically generated acceptance tests for CLI commands.
Only List and Get commands are supported at the moment

dashboards and queries-legacy are skipped for now as their list implementation stops only when API returns no results which acceptance test stubs do not support yet

Why

It helps to make sure that our CLI commands are generated and working correctly when CLI is regenerated. It is especially important for commands with template overrides where template might get out of date with SDK and API changes

It also highlights easier when there are breaking changes to CLI commands as they will be visible in the change of test script for corresponding command

andrewnester
andrewnester commented Mar 17, 2025
View reviewed changes
acceptance/cmd/workspace/query-history/output.txt
@@ -0,0 +1,5 @@

>>> errcode [CLI] query-history list
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found an actual issue, will fix

@andrewnester andrewnester mentioned this pull request Mar 17, 2025
Merged
@andrewnester andrewnester changed the title [WIP, Hackathon] Added automatically generated acceptance tests for CLI commands Added automatically generated acceptance tests for CLI commands Mar 17, 2025
@andrewnester andrewnester marked this pull request as ready for review March 17, 2025 12:57
@andrewnester andrewnester requested a review from denik March 17, 2025 14:19
shreyas-goenka
shreyas-goenka reviewed Mar 17, 2025
View reviewed changes
cmd/workspace/serving-endpoints/serving-endpoints.go
@@ -24,7 +24,7 @@ func New() *cobra.Command {
Short: `The Serving Endpoints API allows you to create, update, and delete model serving endpoints.`,
Long: `The Serving Endpoints API allows you to create, update, and delete model
serving endpoints.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What caused this diff? Seems like something we should revert.

.gitattributes
@@ -134,6 +260,3 @@ cmd/workspace/warehouses/warehouses.go linguist-generated=true
cmd/workspace/workspace-bindings/workspace-bindings.go linguist-generated=true
cmd/workspace/workspace-conf/workspace-conf.go linguist-generated=true
cmd/workspace/workspace/workspace.go linguist-generated=true
bundle/internal/tf/schema/\*.go linguist-generated=true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not go away. Seems unintended.

.codegen.json
@@ -1,9 +1,11 @@
{
"mode": "cli_legacy",
"api_changelog": true,
"formatter": "go run golang.org/x/tools/cmd/goimports@latest -w $FILENAMES && go fmt ./...",
"formatter": "go run golang.org/x/tools/cmd/goimports@latest -w cmd && go fmt ./...",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated, but we can also remove go fmt ./...

ref: https://arc.net/l/quote/widxburh

.codegen.json
"services": {
".codegen/service.go.tmpl": "cmd/{{if .IsAccounts}}account{{else}}workspace{{end}}/{{(.TrimPrefix \"account\").KebabName}}/{{(.TrimPrefix \"account\").KebabName}}.go"
".codegen/service.go.tmpl": "cmd/{{if .IsAccounts}}account{{else}}workspace{{end}}/{{(.TrimPrefix \"account\").KebabName}}/{{(.TrimPrefix \"account\").KebabName}}.go",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated, but seems like .KebabName no longer includes an account or a workspace prefix. Can we simply this?

.codegen/tests/cmd/workspace/script.tmpl
# Code generated from OpenAPI specs by Databricks SDK Generator. DO NOT EDIT.
{{- $excludes :=
list
"command-execution"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent indentation, spaces vs tabs.

.codegen/tests/cmd/workspace/script.tmpl
{{- $generated := false -}}
{{- $supportedMethods := list "get" "list" "list-pipelines" }}
{{- range .Methods}}
{{- if in $supportedMethods .KebabName}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can inline skipThisFile and remove $generated by using a negative predicate:

Suggested change
{{- if in $supportedMethods .KebabName}}
{{- if not (in $supportedMethods .KebabName) }}

.codegen/tests/cmd/workspace/test.toml.tmpl
{{- $generated = true }}

[[Server]]
Pattern = "{{.Verb}} {{replaceAll "2.2" "2.1" .Path}}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this replacement?

.codegen/tests/cmd/workspace/test.toml.tmpl
{{- if in $supportedMethods .KebabName}}
{{- $generated = true }}

[[Server]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of defining a server definition for every command, should we generate a FakeWorkspace, extending the idea in libs/testserver/fake_workspace.go to support stubs for all API calls?

The advantage of fake workspace is eventually it'll be possible to do more CRUD-like autogenerated tests as well. It's also a more general interface, which can be useful in other contexts since it's basically a local simulation of a Databricks workspace.

Writing the server in Go also allows us handle all sorts of API paths and support CREATE, DELETE etc since generating the server stubs in Go is a lot more powerful that generating them in toml.

github-merge-queue bot pushed a commit that referenced this pull request Mar 18, 2025
@andrewnester
Fixed query-history list command (#2506)
976391c 
## Changes
Fixed `Error: template: command:1:8: executing "command" at <.>: range
can't iterate over` for query-history list command

## Why
`query-history list` command has an override to use a different output
template for list command. This template got outdated comparing to
underlying API response structure and hence led to the error above.

it was found by automatically generated tests here:
#2501

## Tests
Added acceptance test
@andrewnester andrewnester marked this pull request as draft March 26, 2025 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shreyas-goenka shreyas-goenka shreyas-goenka left review comments

@pietern pietern Awaiting requested review from pietern pietern is a code owner

@denik denik Awaiting requested review from denik denik is a code owner

@anton-107 anton-107 Awaiting requested review from anton-107 anton-107 will be requested when the pull request is marked ready for review anton-107 is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andrewnester @denik @shreyas-goenka