From 97f67162511cf3479d91fc1b07045874748040fa Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 12 Sep 2026 16:03:21 +0000 Subject: [PATCH 1/3] Test DDL execution and EXPLAIN/DESCRIBE statement paths Cover execute_ddl buffering, empty DDL, progress TTY, async/sync fake-admin LROs, and EXPLAIN/DESCRIBE/ANALYZE query execution plus plan-export edge cases without changing production code. Co-authored-by: apstndb --- internal/mycli/execute_ddl_rpc_test.go | 351 ++++++++++++++++++ internal/mycli/execute_ddl_test.go | 120 ++++++ .../statements_explain_describe_rpc_test.go | 104 ++++++ .../mycli/statements_explain_describe_test.go | 208 +++++++++++ 4 files changed, 783 insertions(+) create mode 100644 internal/mycli/execute_ddl_rpc_test.go create mode 100644 internal/mycli/statements_explain_describe_rpc_test.go diff --git a/internal/mycli/execute_ddl_rpc_test.go b/internal/mycli/execute_ddl_rpc_test.go new file mode 100644 index 00000000..aea2cbdd --- /dev/null +++ b/internal/mycli/execute_ddl_rpc_test.go @@ -0,0 +1,351 @@ +// Copyright 2026 apstndb +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package mycli + +import ( + "context" + "errors" + "io" + "net" + "os" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "cloud.google.com/go/longrunning/autogen/longrunningpb" + adminapi "cloud.google.com/go/spanner/admin/database/apiv1" + "cloud.google.com/go/spanner/admin/database/apiv1/databasepb" + "github.com/apstndb/spanner-mycli/internal/mycli/streamio" + "google.golang.org/api/option" + "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/credentials/insecure" + "google.golang.org/grpc/status" + "google.golang.org/grpc/test/bufconn" + "google.golang.org/protobuf/proto" + "google.golang.org/protobuf/types/known/anypb" + "google.golang.org/protobuf/types/known/emptypb" + "google.golang.org/protobuf/types/known/timestamppb" +) + +const ddlRPCOpName = "projects/test/instances/test/databases/test/operations/op-ddl" + +func TestExecuteDdlStatementsRPC(t *testing.T) { + t.Parallel() + + const ddl = "CREATE TABLE t (id INT64) PRIMARY KEY (id)" + commitTS := time.Date(2026, 9, 12, 15, 0, 0, 0, time.UTC) + + t.Run("create error", func(t *testing.T) { + t.Parallel() + server := &ddlAdminTestServer{updateErr: status.Error(codes.InvalidArgument, "bad DDL")} + session := newDDLAdminSession(t, server) + before := session.SchemaGeneration() + _, err := executeDdlStatements(t.Context(), session, []string{ddl}) + if err == nil || !strings.Contains(err.Error(), "error on create op") { + t.Fatalf("error = %v, want create-op wrap", err) + } + if !strings.Contains(err.Error(), "bad DDL") { + t.Fatalf("error = %v, want underlying InvalidArgument", err) + } + if session.SchemaGeneration() != before { + t.Fatalf("schema generation = %d, want unchanged %d", session.SchemaGeneration(), before) + } + if server.lastUpdate == nil { + t.Fatal("UpdateDatabaseDdl was not invoked") + } + if server.lastUpdate.GetDatabase() != session.DatabasePath() { + t.Fatalf("Database = %q, want %q", server.lastUpdate.GetDatabase(), session.DatabasePath()) + } + if diff := strings.Join(server.lastUpdate.GetStatements(), "\n"); diff != ddl { + t.Fatalf("Statements = %v, want %q", server.lastUpdate.GetStatements(), ddl) + } + }) + + t.Run("sync echo executed DDL", func(t *testing.T) { + t.Parallel() + server := newCompletedDDLServer(ddl, commitTS) + session := newDDLAdminSession(t, server) + session.systemVariables.Feature.EchoExecutedDDL = true + before := session.SchemaGeneration() + got, err := executeDdlStatements(t.Context(), session, []string{ddl}) + if err != nil { + t.Fatalf("executeDdlStatements() error = %v", err) + } + if session.SchemaGeneration() != before+1 { + t.Fatalf("schema generation = %d, want %d", session.SchemaGeneration(), before+1) + } + if !got.CommitTimestamp.Equal(commitTS) { + t.Fatalf("CommitTimestamp = %v, want %v", got.CommitTimestamp, commitTS) + } + if got.TableHeader == nil { + t.Fatal("TableHeader = nil, want echo columns") + } + if len(got.Rows) != 1 { + t.Fatalf("len(Rows) = %d, want 1", len(got.Rows)) + } + if got.Rows[0][0].RawText() != ddl+";" { + t.Fatalf("executed DDL = %q, want %q", got.Rows[0][0].RawText(), ddl+";") + } + if got.Rows[0][1].RawText() != commitTS.Format(time.RFC3339Nano) { + t.Fatalf("commit timestamp cell = %q, want %q", got.Rows[0][1].RawText(), commitTS.Format(time.RFC3339Nano)) + } + }) + + t.Run("async returns operation rows", func(t *testing.T) { + t.Parallel() + server := newCompletedDDLServer(ddl, commitTS) + server.done = false + session := newDDLAdminSession(t, server) + session.systemVariables.Feature.AsyncDDL = true + before := session.SchemaGeneration() + got, err := executeDdlStatements(t.Context(), session, []string{ddl}) + if err != nil { + t.Fatalf("executeDdlStatements() error = %v", err) + } + if session.SchemaGeneration() != before+1 { + t.Fatalf("schema generation = %d, want %d", session.SchemaGeneration(), before+1) + } + if got.AffectedRows != 1 || len(got.Rows) != 1 { + t.Fatalf("async result = %+v", got) + } + if got.Rows[0][0].RawText() != "op-ddl" { + t.Fatalf("OPERATION_ID = %q, want op-ddl", got.Rows[0][0].RawText()) + } + if got.Rows[0][1].RawText() != ddl+";" { + t.Fatalf("STATEMENTS = %q, want %q", got.Rows[0][1].RawText(), ddl+";") + } + if got.Rows[0][2].RawText() != "false" { + t.Fatalf("DONE = %q, want false", got.Rows[0][2].RawText()) + } + if server.getCalls.Load() != 0 { + t.Fatalf("async path polled GetOperation %d times, want 0", server.getCalls.Load()) + } + }) + + t.Run("async metadata type error", func(t *testing.T) { + t.Parallel() + wrong, err := anypb.New(&emptypb.Empty{}) + if err != nil { + t.Fatal(err) + } + server := &ddlAdminTestServer{ + opName: ddlRPCOpName, + metadata: wrong, + done: false, + } + session := newDDLAdminSession(t, server) + session.systemVariables.Feature.AsyncDDL = true + _, execErr := executeDdlStatements(t.Context(), session, []string{ddl}) + if execErr == nil || !strings.Contains(execErr.Error(), "failed to get operation metadata") { + t.Fatalf("error = %v, want metadata unmarshal failure", execErr) + } + }) + + t.Run("poll cancellation hints SHOW OPERATION", func(t *testing.T) { + t.Parallel() + server := newCompletedDDLServer(ddl, commitTS) + server.done = false + server.getErr = status.Error(codes.Canceled, "context canceled") + session := newDDLAdminSession(t, server) + before := session.SchemaGeneration() + _, err := executeDdlStatements(t.Context(), session, []string{ddl}) + if err == nil || !strings.Contains(err.Error(), "SHOW OPERATION 'op-ddl'") { + t.Fatalf("error = %v, want SHOW OPERATION cancellation hint", err) + } + if session.SchemaGeneration() != before+1 { + t.Fatalf("schema generation = %d, want %d after cancel", session.SchemaGeneration(), before+1) + } + }) + + t.Run("poll invalid argument keeps original error", func(t *testing.T) { + t.Parallel() + server := newCompletedDDLServer(ddl, commitTS) + server.done = false + server.getErr = status.Error(codes.InvalidArgument, "syntax error") + session := newDDLAdminSession(t, server) + before := session.SchemaGeneration() + _, err := executeDdlStatements(t.Context(), session, []string{ddl}) + if err == nil || strings.Contains(err.Error(), "SHOW OPERATION") { + t.Fatalf("error = %v, want original DDL failure", err) + } + if status.Code(err) != codes.InvalidArgument && !strings.Contains(err.Error(), "syntax error") { + t.Fatalf("error = %v, want InvalidArgument", err) + } + if session.SchemaGeneration() != before+1 { + t.Fatalf("schema generation = %d, want %d after accepted op", session.SchemaGeneration(), before+1) + } + }) + + t.Run("wait loop cancellation", func(t *testing.T) { + t.Parallel() + server := newCompletedDDLServer(ddl, commitTS) + server.stayPending = true + session := newDDLAdminSession(t, server) + ctx, cancel := context.WithTimeout(t.Context(), 200*time.Millisecond) + defer cancel() + before := session.SchemaGeneration() + _, err := executeDdlStatements(ctx, session, []string{ddl}) + if err == nil || !strings.Contains(err.Error(), "SHOW OPERATION 'op-ddl'") { + t.Fatalf("error = %v, want canceled wait hint", err) + } + if session.SchemaGeneration() != before+1 { + t.Fatalf("schema generation = %d, want %d", session.SchemaGeneration(), before+1) + } + }) + + t.Run("progress bar forced complete", func(t *testing.T) { + t.Parallel() + server := newCompletedDDLServer(ddl, commitTS) + server.metadata = mustDDLMetadata(ddl, commitTS, 40, 100) + session := newDDLAdminSession(t, server) + session.systemVariables.Display.EnableProgressBar = true + tty, err := os.CreateTemp(t.TempDir(), "ddl-bar-*.txt") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = tty.Close() }) + session.systemVariables.StreamManager = streamio.NewStreamManager(io.NopCloser(strings.NewReader("")), io.Discard, io.Discard) + session.systemVariables.StreamManager.SetTtyStream(tty) + got, err := executeDdlStatements(t.Context(), session, []string{ddl}) + if err != nil { + t.Fatalf("executeDdlStatements() error = %v", err) + } + if !got.CommitTimestamp.Equal(commitTS) { + t.Fatalf("CommitTimestamp = %v, want %v", got.CommitTimestamp, commitTS) + } + }) +} + +type ddlAdminTestServer struct { + databasepb.UnimplementedDatabaseAdminServer + longrunningpb.UnimplementedOperationsServer + + mu sync.Mutex + updateErr error + getErr error + stayPending bool + done bool + opName string + metadata *anypb.Any + lastUpdate *databasepb.UpdateDatabaseDdlRequest + getCalls atomic.Int32 +} + +func newCompletedDDLServer(ddl string, commitTS time.Time) *ddlAdminTestServer { + return &ddlAdminTestServer{ + opName: ddlRPCOpName, + done: true, + metadata: mustDDLMetadata(ddl, commitTS, 100), + } +} + +func mustDDLMetadata(ddl string, commitTS time.Time, percents ...int32) *anypb.Any { + progress := make([]*databasepb.OperationProgress, len(percents)) + for i, p := range percents { + progress[i] = &databasepb.OperationProgress{ProgressPercent: p} + } + md, err := anypb.New(&databasepb.UpdateDatabaseDdlMetadata{ + Statements: []string{ddl}, + CommitTimestamps: []*timestamppb.Timestamp{timestamppb.New(commitTS)}, + Progress: progress, + }) + if err != nil { + panic(err) + } + return md +} + +func (s *ddlAdminTestServer) operation() *longrunningpb.Operation { + s.mu.Lock() + defer s.mu.Unlock() + op := &longrunningpb.Operation{ + Name: s.opName, + Done: s.done && !s.stayPending, + Metadata: s.metadata, + } + if op.Done { + resp, err := anypb.New(&emptypb.Empty{}) + if err != nil { + panic(err) + } + op.Result = &longrunningpb.Operation_Response{Response: resp} + } + return op +} + +func (s *ddlAdminTestServer) UpdateDatabaseDdl(_ context.Context, req *databasepb.UpdateDatabaseDdlRequest) (*longrunningpb.Operation, error) { + s.mu.Lock() + s.lastUpdate = proto.Clone(req).(*databasepb.UpdateDatabaseDdlRequest) + err := s.updateErr + s.mu.Unlock() + if err != nil { + return nil, err + } + return s.operation(), nil +} + +func (s *ddlAdminTestServer) GetOperation(context.Context, *longrunningpb.GetOperationRequest) (*longrunningpb.Operation, error) { + s.getCalls.Add(1) + s.mu.Lock() + err := s.getErr + s.mu.Unlock() + if err != nil { + return nil, err + } + return s.operation(), nil +} + +func newDDLAdminSession(t *testing.T, server *ddlAdminTestServer) *Session { + t.Helper() + listener := bufconn.Listen(1 << 20) + grpcServer := grpc.NewServer() + databasepb.RegisterDatabaseAdminServer(grpcServer, server) + longrunningpb.RegisterOperationsServer(grpcServer, server) + go func() { + if err := grpcServer.Serve(listener); err != nil && !errors.Is(err, grpc.ErrServerStopped) { + t.Errorf("serve ddl admin: %v", err) + } + }() + t.Cleanup(func() { + grpcServer.Stop() + _ = listener.Close() + }) + conn, err := grpc.NewClient("passthrough:///ddl-admin", + grpc.WithContextDialer(func(context.Context, string) (net.Conn, error) { return listener.Dial() }), + grpc.WithTransportCredentials(insecure.NewCredentials()), + ) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = conn.Close() }) + adminClient, err := adminapi.NewDatabaseAdminClient(t.Context(), option.WithGRPCConn(conn)) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = adminClient.Close() }) + + sysVars := newSystemVariablesWithDefaultsForTest() + identity := ConnectionVars{Project: "test", Instance: "test", Database: "test"} + sysVars.Connection = identity + session := &Session{ + adminClient: adminClient, + systemVariables: sysVars, + connection: identity, + } + return session +} diff --git a/internal/mycli/execute_ddl_test.go b/internal/mycli/execute_ddl_test.go index 0be4c838..75d258e5 100644 --- a/internal/mycli/execute_ddl_test.go +++ b/internal/mycli/execute_ddl_test.go @@ -18,9 +18,14 @@ import ( "context" "errors" "fmt" + "io" + "os" "strings" "testing" + "cloud.google.com/go/spanner" + "github.com/apstndb/spanner-mycli/internal/mycli/streamio" + "github.com/google/go-cmp/cmp" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) @@ -108,3 +113,118 @@ func TestIsCancellationError(t *testing.T) { }) } } + +func TestBufferOrExecuteDdlStatements(t *testing.T) { + t.Parallel() + + t.Run("rejects active batch DML", func(t *testing.T) { + t.Parallel() + session := newSessionForLocalVarTest(t) + session.batch.SetCurrent(&BatchDMLStatement{}) + _, err := bufferOrExecuteDdlStatements(t.Context(), session, []string{"CREATE TABLE t (id INT64) PRIMARY KEY (id)"}) + if err == nil || !strings.Contains(err.Error(), "active batch DML") { + t.Fatalf("error = %v, want active batch DML", err) + } + }) + + t.Run("buffers into active bulk DDL", func(t *testing.T) { + t.Parallel() + session := newSessionForLocalVarTest(t) + bulk := &BulkDdlStatement{Ddls: []string{"CREATE TABLE t1 (id INT64) PRIMARY KEY (id)"}} + session.batch.SetCurrent(bulk) + got, err := bufferOrExecuteDdlStatements(t.Context(), session, []string{"CREATE TABLE t2 (id INT64) PRIMARY KEY (id)"}) + if err != nil { + t.Fatalf("bufferOrExecuteDdlStatements() error = %v", err) + } + if got == nil || got.KeepVariables { + t.Fatalf("result = %+v, want empty Result", got) + } + want := []string{ + "CREATE TABLE t1 (id INT64) PRIMARY KEY (id)", + "CREATE TABLE t2 (id INT64) PRIMARY KEY (id)", + } + if diff := strings.Join(bulk.Ddls, "\n"); diff != strings.Join(want, "\n") { + t.Fatalf("buffered DDLs = %v, want %v", bulk.Ddls, want) + } + current, ok := session.batch.Current().(*BulkDdlStatement) + if !ok || current != bulk { + t.Fatalf("batch.Current() = %T, want original *BulkDdlStatement", session.batch.Current()) + } + }) + + t.Run("rejects queued automatic DML", func(t *testing.T) { + t.Parallel() + session := newSessionForLocalVarTest(t) + session.txn.autoDML = []spanner.Statement{{SQL: "INSERT INTO t (id) VALUES (1)"}} + _, err := bufferOrExecuteDdlStatements(t.Context(), session, []string{"CREATE TABLE t (id INT64) PRIMARY KEY (id)"}) + if err == nil || !strings.Contains(err.Error(), "active batch DML") { + t.Fatalf("error = %v, want active batch DML", err) + } + }) +} + +func TestExecuteDdlStatementsEmpty(t *testing.T) { + t.Parallel() + + t.Run("no echo header", func(t *testing.T) { + t.Parallel() + session := newSessionForLocalVarTest(t) + got, err := executeDdlStatements(t.Context(), session, nil) + if err != nil { + t.Fatalf("executeDdlStatements() error = %v", err) + } + if got.TableHeader != nil { + t.Fatalf("TableHeader = %v, want nil", got.TableHeader) + } + }) + + t.Run("echo header without rows", func(t *testing.T) { + t.Parallel() + session := newSessionForLocalVarTest(t) + session.systemVariables.Feature.EchoExecutedDDL = true + got, err := executeDdlStatements(t.Context(), session, nil) + if err != nil { + t.Fatalf("executeDdlStatements() error = %v", err) + } + want := toTableHeader("Executed", "Commit Timestamp") + if diff := cmp.Diff(want, got.TableHeader); diff != "" { + t.Fatalf("TableHeader mismatch (-want +got):\n%s", diff) + } + if len(got.Rows) != 0 { + t.Fatalf("Rows = %v, want empty", got.Rows) + } + }) +} + +func TestNewProgressWithTTY(t *testing.T) { + t.Parallel() + + if p := newProgressWithTTY(t.Context(), nil); p != nil { + t.Fatal("nil session: got progress, want nil") + } + if p := newProgressWithTTY(t.Context(), &Session{}); p != nil { + t.Fatal("nil systemVariables: got progress, want nil") + } + + session := newSessionForLocalVarTest(t) + if p := newProgressWithTTY(t.Context(), session); p != nil { + t.Fatal("nil StreamManager: got progress, want nil") + } + + session.systemVariables.StreamManager = streamio.NewStreamManager(io.NopCloser(strings.NewReader("")), io.Discard, io.Discard) + if p := newProgressWithTTY(t.Context(), session); p != nil { + t.Fatal("non-TTY output: got progress, want nil") + } + + tty, err := os.CreateTemp(t.TempDir(), "ddl-progress-*.txt") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = tty.Close() }) + session.systemVariables.StreamManager.SetTtyStream(tty) + p := newProgressWithTTY(t.Context(), session) + if p == nil { + t.Fatal("TTY stream: got nil progress") + } + p.Wait() +} diff --git a/internal/mycli/statements_explain_describe_rpc_test.go b/internal/mycli/statements_explain_describe_rpc_test.go new file mode 100644 index 00000000..ee518c28 --- /dev/null +++ b/internal/mycli/statements_explain_describe_rpc_test.go @@ -0,0 +1,104 @@ +// Copyright 2026 apstndb +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package mycli + +import ( + "errors" + "strings" + "testing" +) + +func TestExplainDescribeStatementsRPC(t *testing.T) { + t.Parallel() + + stats := map[string]any{ + "elapsed_time": "1 msec", + "rows_returned": "1", + "query_text": "SELECT 1", + "cpu_time": "1 msec", + "rows_scanned": "0", + "optimizer_version": "7", + } + + t.Run("EXPLAIN SELECT", func(t *testing.T) { + t.Parallel() + session, _ := newQueryCacheRPCSession(t, testQueryPlan(t), stats, nil) + got, err := (&ExplainStatement{Explain: "SELECT 1"}).Execute(t.Context(), session) + if err != nil { + t.Fatalf("EXPLAIN: %v", err) + } + if got.AffectedRows == 0 || len(got.Rows) == 0 { + t.Fatalf("EXPLAIN result = %+v, want plan rows", got) + } + joined := rowText(got.Rows[0]) + if !strings.Contains(joined, "Serialize Result") { + t.Fatalf("EXPLAIN row = %q, want Serialize Result", joined) + } + }) + + t.Run("EXPLAIN emulator without plan", func(t *testing.T) { + t.Parallel() + session, _ := newQueryCacheRPCSession(t, nil, stats, nil) + _, err := (&ExplainStatement{Explain: "SELECT 1"}).Execute(t.Context(), session) + if err == nil || !strings.Contains(err.Error(), "EXPLAIN statement is not supported for Cloud Spanner Emulator") { + t.Fatalf("error = %v, want emulator EXPLAIN rejection", err) + } + }) + + t.Run("EXPLAIN ANALYZE SELECT", func(t *testing.T) { + t.Parallel() + session, live := newQueryCacheRPCSession(t, testQueryPlan(t), stats, nil) + got, err := (&ExplainAnalyzeStatement{Query: "SELECT 1"}).Execute(t.Context(), session) + if err != nil { + t.Fatalf("EXPLAIN ANALYZE: %v", err) + } + if got.AffectedRows != 1 { + t.Fatalf("AffectedRows = %d, want 1 scanned data row", got.AffectedRows) + } + if live.LastResult.QueryCache == nil || live.LastResult.QueryCache.QueryPlan == nil { + t.Fatal("EXPLAIN ANALYZE did not publish LastQueryCache") + } + if len(got.Rows) == 0 || !strings.Contains(rowText(got.Rows[0]), "Serialize Result") { + t.Fatalf("ANALYZE rows = %v", got.Rows) + } + }) + + t.Run("EXPLAIN ANALYZE emulator without plan", func(t *testing.T) { + t.Parallel() + session, _ := newQueryCacheRPCSession(t, nil, stats, nil) + _, err := (&ExplainAnalyzeStatement{Query: "SELECT 1"}).Execute(t.Context(), session) + if !errors.Is(err, errExplainAnalyzeUnsupportedOnEmulator) { + t.Fatalf("error = %v, want %v", err, errExplainAnalyzeUnsupportedOnEmulator) + } + }) + + t.Run("DESCRIBE SELECT", func(t *testing.T) { + t.Parallel() + session, _ := newQueryCacheRPCSession(t, testQueryPlan(t), stats, nil) + got, err := (&DescribeStatement{Statement: "SELECT 1"}).Execute(t.Context(), session) + if err != nil { + t.Fatalf("DESCRIBE: %v", err) + } + if got.AffectedRows != 1 || len(got.Rows) != 1 { + t.Fatalf("DESCRIBE result = %+v", got) + } + if got.Rows[0][0].RawText() != "id" { + t.Fatalf("Column_Name = %q, want id", got.Rows[0][0].RawText()) + } + if !strings.Contains(got.Rows[0][1].RawText(), "INT64") { + t.Fatalf("Column_Type = %q, want INT64", got.Rows[0][1].RawText()) + } + }) +} diff --git a/internal/mycli/statements_explain_describe_test.go b/internal/mycli/statements_explain_describe_test.go index 795e0917..d1901795 100644 --- a/internal/mycli/statements_explain_describe_test.go +++ b/internal/mycli/statements_explain_describe_test.go @@ -1259,4 +1259,212 @@ func TestShowLastQueryPlanStatement_Execute(t *testing.T) { t.Fatal("Execute() error = nil, want empty-path rejection") } }) + + t.Run("into NUL rejected", func(t *testing.T) { + t.Parallel() + _, err := (&ShowLastQueryPlanStatement{IntoPath: "plan\x00.json"}).Execute(context.Background(), &Session{ + systemVariables: &systemVariables{LastResult: LastResult{QueryCache: &LastQueryCache{QueryPlan: plan}}}, + }) + if err == nil || !strings.Contains(err.Error(), "NUL") { + t.Fatalf("Execute() error = %v, want NUL rejection", err) + } + }) + + t.Run("into non-regular file rejected", func(t *testing.T) { + t.Parallel() + _, err := (&ShowLastQueryPlanStatement{IntoPath: os.DevNull}).Execute(context.Background(), &Session{ + systemVariables: &systemVariables{LastResult: LastResult{QueryCache: &LastQueryCache{QueryPlan: plan}}}, + }) + if err == nil || !strings.Contains(err.Error(), "not a regular file") { + t.Fatalf("Execute() error = %v, want non-regular rejection", err) + } + }) + + t.Run("into missing parent rejected", func(t *testing.T) { + t.Parallel() + path := filepath.Join(t.TempDir(), "missing", "plan.json") + _, err := (&ShowLastQueryPlanStatement{IntoPath: path}).Execute(context.Background(), &Session{ + systemVariables: &systemVariables{LastResult: LastResult{QueryCache: &LastQueryCache{QueryPlan: plan}}}, + }) + if err == nil || !strings.Contains(err.Error(), "invalid INTO path parent") { + t.Fatalf("Execute() error = %v, want missing-parent rejection", err) + } + }) +} + +func TestShowPlanNodeStatementMissingCache(t *testing.T) { + t.Parallel() + + t.Run("no cache", func(t *testing.T) { + t.Parallel() + _, err := (&ShowPlanNodeStatement{NodeID: 0}).Execute(context.Background(), &Session{ + systemVariables: &systemVariables{}, + }) + if err == nil || !strings.Contains(err.Error(), "no query plan cached") { + t.Fatalf("error = %v, want missing-cache", err) + } + }) + + t.Run("node out of range", func(t *testing.T) { + t.Parallel() + _, err := (&ShowPlanNodeStatement{NodeID: 99}).Execute(context.Background(), &Session{ + systemVariables: &systemVariables{LastResult: LastResult{QueryCache: &LastQueryCache{ + QueryPlan: selectProfileResultSet.GetStats().GetQueryPlan(), + }}}, + }) + if err == nil || !strings.Contains(err.Error(), "node with ID 99 not found") { + t.Fatalf("error = %v, want out-of-range", err) + } + }) +} + +func TestParseAlignment(t *testing.T) { + t.Parallel() + for _, tt := range []struct { + in string + want tw.Align + wantErr bool + }{ + {in: "RIGHT", want: tw.AlignRight}, + {in: "ALIGN_RIGHT", want: tw.AlignRight}, + {in: "LEFT", want: tw.AlignLeft}, + {in: "CENTER", want: tw.AlignCenter}, + {in: "NONE", want: tw.AlignNone}, + {in: "DEFAULT", want: tw.AlignDefault}, + {in: "SIDEWAYS", wantErr: true}, + } { + t.Run(tt.in, func(t *testing.T) { + t.Parallel() + got, err := parseAlignment(tt.in) + if tt.wantErr { + if err == nil { + t.Fatalf("parseAlignment(%q) error = nil, want error", tt.in) + } + return + } + if err != nil { + t.Fatalf("parseAlignment(%q) error = %v", tt.in, err) + } + if got != tt.want { + t.Fatalf("parseAlignment(%q) = %q, want %q", tt.in, got, tt.want) + } + }) + } +} + +func TestCustomListToTableRenderDefsAndInlineStats(t *testing.T) { + t.Parallel() + + t.Run("explicit left alignment", func(t *testing.T) { + t.Parallel() + got, err := customListToTableRenderDefs("Rows:{{.Rows.Total}}:LEFT") + if err != nil { + t.Fatal(err) + } + if len(got) != 1 || got[0].Name != "Rows" || got[0].Alignment != tw.AlignLeft { + t.Fatalf("got %+v", got) + } + row := plantree.RowWithPredicates{ExecutionStats: stats.ExecutionStats{Rows: stats.ExecutionStatsValue{Total: "9"}}} + cell, err := got[0].MapFunc(row) + if err != nil || cell != "9" { + t.Fatalf("MapFunc = %q, %v", cell, err) + } + }) + + t.Run("invalid alignment", func(t *testing.T) { + t.Parallel() + _, err := customListToTableRenderDefs("Rows:{{.Rows.Total}}:SIDEWAYS") + if err == nil || !strings.Contains(err.Error(), "failed to parseAlignment") { + t.Fatalf("error = %v, want parseAlignment failure", err) + } + }) + + t.Run("invalid field count", func(t *testing.T) { + t.Parallel() + _, err := customListToTableRenderDefs("Rows") + if err == nil || !strings.Contains(err.Error(), "invalid format") { + t.Fatalf("error = %v, want invalid format", err) + } + }) + + t.Run("inline stats missing colon", func(t *testing.T) { + t.Parallel() + _, err := parseInlineStatsDefs("rows") + if err == nil || !strings.Contains(err.Error(), "invalid inline stats format") { + t.Fatalf("error = %v, want format error", err) + } + }) +} + +func TestExtractIndexAdvice(t *testing.T) { + t.Parallel() + plan := &sppb.QueryPlan{ + QueryAdvice: &sppb.QueryAdvisorResult{ + IndexAdvice: []*sppb.QueryAdvisorResult_IndexAdvice{ + {Ddl: nil, ImprovementFactor: 2}, + {Ddl: []string{"CREATE INDEX idx ON t (id)"}, ImprovementFactor: 4}, + }, + }, + } + got := extractIndexAdvice(plan) + want := []QueryIndexAdvice{{ + DDL: []string{"CREATE INDEX idx ON t (id)"}, + ImprovementFactor: 4, + }} + if diff := cmp.Diff(want, got); diff != "" { + t.Fatalf("extractIndexAdvice mismatch (-want +got):\n%s", diff) + } +} + +func TestProcessPlanNodesCompactAndTraditional(t *testing.T) { + t.Parallel() + nodes := hangingIndentPlanNodes() + + compact, err := processPlanNodes(nodes, nil, enums.ExplainFormatCompact, 0, false) + if err != nil { + t.Fatalf("compact: %v", err) + } + if len(compact) == 0 { + t.Fatal("compact plan produced no rows") + } + + traditional, err := processPlanNodes(nodes, nil, enums.ExplainFormatTraditional, 0, false) + if err != nil { + t.Fatalf("traditional: %v", err) + } + if len(traditional) == 0 { + t.Fatal("traditional plan produced no rows") + } +} + +func TestGenerateExplainResultLintsPlan(t *testing.T) { + t.Parallel() + sysVars := newSystemVariablesWithDefaultsForTest() + sysVars.Query.LintPlan = true + got, err := generateExplainResult(sysVars, selectProfileResultSet.GetStats().GetQueryPlan(), enums.ExplainFormatUnspecified, 0, nil) + if err != nil { + t.Fatalf("generateExplainResult: %v", err) + } + want := lintPlan(selectProfileResultSet.GetStats().GetQueryPlan()) + if diff := cmp.Diff(want, got.LintResults, cmpopts.EquateEmpty()); diff != "" { + t.Fatalf("LintResults mismatch (-want +got):\n%s", diff) + } +} + +func TestBuildExplainAnalyzeResultIndexAdvice(t *testing.T) { + t.Parallel() + sysVars := newSystemVariablesWithDefaultsForTest() + plan := proto.Clone(selectProfileResultSet.GetStats().GetQueryPlan()).(*sppb.QueryPlan) + plan.QueryAdvice = &sppb.QueryAdvisorResult{ + IndexAdvice: []*sppb.QueryAdvisorResult_IndexAdvice{ + {Ddl: []string{"CREATE INDEX idx ON t (id)"}, ImprovementFactor: 3}, + }, + } + got, err := buildExplainAnalyzeResult(sysVars, plan, QueryStats{}, enums.ExplainFormatUnspecified, 0, nil) + if err != nil { + t.Fatalf("buildExplainAnalyzeResult: %v", err) + } + if len(got.IndexAdvice) != 1 || got.IndexAdvice[0].ImprovementFactor != 3 { + t.Fatalf("IndexAdvice = %+v", got.IndexAdvice) + } } From ac976b70fda4f62e25708080d664743ea9c89cba Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 12 Sep 2026 16:18:49 +0000 Subject: [PATCH 2/3] Fix DDL RPC tests for Codex review findings Cancel the wait-loop test only after the fake accepts UpdateDatabaseDdl and the first GetOperation poll, using a 30s timeout as a deadlock guard instead of a 200ms deadline on the initial admin RPC. Assert both InvalidArgument and the injected poll error message. No production or coverage-policy changes. Refs #941. Co-authored-by: apstndb --- internal/mycli/execute_ddl_rpc_test.go | 86 +++++++++++++++++++++----- 1 file changed, 71 insertions(+), 15 deletions(-) diff --git a/internal/mycli/execute_ddl_rpc_test.go b/internal/mycli/execute_ddl_rpc_test.go index aea2cbdd..ecaeae3a 100644 --- a/internal/mycli/execute_ddl_rpc_test.go +++ b/internal/mycli/execute_ddl_rpc_test.go @@ -183,8 +183,11 @@ func TestExecuteDdlStatementsRPC(t *testing.T) { if err == nil || strings.Contains(err.Error(), "SHOW OPERATION") { t.Fatalf("error = %v, want original DDL failure", err) } - if status.Code(err) != codes.InvalidArgument && !strings.Contains(err.Error(), "syntax error") { - t.Fatalf("error = %v, want InvalidArgument", err) + if status.Code(err) != codes.InvalidArgument { + t.Fatalf("status.Code = %v, want InvalidArgument; err = %v", status.Code(err), err) + } + if !strings.Contains(err.Error(), "syntax error") { + t.Fatalf("error = %v, want injected message %q", err, "syntax error") } if session.SchemaGeneration() != before+1 { t.Fatalf("schema generation = %d, want %d after accepted op", session.SchemaGeneration(), before+1) @@ -195,11 +198,33 @@ func TestExecuteDdlStatementsRPC(t *testing.T) { t.Parallel() server := newCompletedDDLServer(ddl, commitTS) server.stayPending = true + server.accepted = make(chan struct{}) + server.polled = make(chan struct{}) session := newDDLAdminSession(t, server) - ctx, cancel := context.WithTimeout(t.Context(), 200*time.Millisecond) + ctx, cancel := context.WithCancel(t.Context()) defer cancel() + // Generous outer timeout is a deadlock guard only; cancellation is + // synchronized with the fake observing an accepted op and first poll. + guard, guardCancel := context.WithTimeout(t.Context(), 30*time.Second) + defer guardCancel() + + errc := make(chan error, 1) before := session.SchemaGeneration() - _, err := executeDdlStatements(ctx, session, []string{ddl}) + go func() { + _, err := executeDdlStatements(ctx, session, []string{ddl}) + errc <- err + }() + + waitForClosed(t, guard, server.accepted, "accepted UpdateDatabaseDdl") + waitForClosed(t, guard, server.polled, "first GetOperation poll") + cancel() + + var err error + select { + case err = <-errc: + case <-guard.Done(): + t.Fatal("timed out waiting for canceled DDL wait") + } if err == nil || !strings.Contains(err.Error(), "SHOW OPERATION 'op-ddl'") { t.Fatalf("error = %v, want canceled wait hint", err) } @@ -235,15 +260,42 @@ type ddlAdminTestServer struct { databasepb.UnimplementedDatabaseAdminServer longrunningpb.UnimplementedOperationsServer - mu sync.Mutex - updateErr error - getErr error - stayPending bool - done bool - opName string - metadata *anypb.Any - lastUpdate *databasepb.UpdateDatabaseDdlRequest - getCalls atomic.Int32 + mu sync.Mutex + updateErr error + getErr error + stayPending bool + done bool + opName string + metadata *anypb.Any + lastUpdate *databasepb.UpdateDatabaseDdlRequest + getCalls atomic.Int32 + accepted chan struct{} + acceptedOnce sync.Once + polled chan struct{} + pollOnce sync.Once +} + +func (s *ddlAdminTestServer) notifyAccepted() { + if s.accepted == nil { + return + } + s.acceptedOnce.Do(func() { close(s.accepted) }) +} + +func (s *ddlAdminTestServer) notifyPolled() { + if s.polled == nil { + return + } + s.pollOnce.Do(func() { close(s.polled) }) +} + +func waitForClosed(t *testing.T, ctx context.Context, ch <-chan struct{}, what string) { + t.Helper() + select { + case <-ch: + case <-ctx.Done(): + t.Fatalf("timed out waiting for %s", what) + } } func newCompletedDDLServer(ddl string, commitTS time.Time) *ddlAdminTestServer { @@ -296,7 +348,9 @@ func (s *ddlAdminTestServer) UpdateDatabaseDdl(_ context.Context, req *databasep if err != nil { return nil, err } - return s.operation(), nil + op := s.operation() + s.notifyAccepted() + return op, nil } func (s *ddlAdminTestServer) GetOperation(context.Context, *longrunningpb.GetOperationRequest) (*longrunningpb.Operation, error) { @@ -307,7 +361,9 @@ func (s *ddlAdminTestServer) GetOperation(context.Context, *longrunningpb.GetOpe if err != nil { return nil, err } - return s.operation(), nil + op := s.operation() + s.notifyPolled() + return op, nil } func newDDLAdminSession(t *testing.T, server *ddlAdminTestServer) *Session { From c458db271387fb26b421d9f03771a9d2a76cd208 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 12 Sep 2026 16:28:10 +0000 Subject: [PATCH 3/3] Enter the DDL wait loop before canceling the test After the fake accepts UpdateDatabaseDdl and serves the first GetOperation, wait briefly so executeDdlStatements leaves op.Poll and blocks in the wait-loop select. Canceling at poll-handler return was racing the in-flight Poll and skipping the ctx.Done branch. No production or coverage-policy changes. Refs #941. Co-authored-by: apstndb --- internal/mycli/execute_ddl_rpc_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/internal/mycli/execute_ddl_rpc_test.go b/internal/mycli/execute_ddl_rpc_test.go index ecaeae3a..2131a553 100644 --- a/internal/mycli/execute_ddl_rpc_test.go +++ b/internal/mycli/execute_ddl_rpc_test.go @@ -217,6 +217,14 @@ func TestExecuteDdlStatementsRPC(t *testing.T) { waitForClosed(t, guard, server.accepted, "accepted UpdateDatabaseDdl") waitForClosed(t, guard, server.polled, "first GetOperation poll") + // The fake has served GetOperation. Wait until executeDdlStatements has + // left op.Poll and blocked in the wait-loop select before canceling. + // This is not a production deadline; cancel remains explicit. + select { + case err := <-errc: + t.Fatalf("executeDdlStatements returned before wait-loop cancel: %v", err) + case <-time.After(100 * time.Millisecond): + } cancel() var err error