Copilot commented on code in PR #983: URL: https://github.com/apache/incubator-seata-go/pull/983#discussion_r2504334759
########## pkg/datasource/sql/undo/executor/mysql_undo_insert_executor_test.go: ########## @@ -0,0 +1,128 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 executor + +import ( + "context" + "testing" + + "github.com/DATA-DOG/go-sqlmock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "seata.apache.org/seata-go/pkg/datasource/sql/types" + "seata.apache.org/seata-go/pkg/datasource/sql/undo" +) + +func TestNewMySQLUndoInsertExecutor(t *testing.T) { + sqlUndoLog := undo.SQLUndoLog{ + TableName: "test_table", + SQLType: types.SQLType_INSERT, + } + + executor := newMySQLUndoInsertExecutor(sqlUndoLog) + assert.NotNil(t, executor) + assert.Equal(t, "test_table", executor.sqlUndoLog.TableName) +} + +func TestMySQLUndoInsertExecutor_BuildUndoSQL(t *testing.T) { + sqlUndoLog := undo.SQLUndoLog{ + TableName: "test_table", + SQLType: types.SQLType_INSERT, + AfterImage: &types.RecordImage{ + TableName: "test_table", + TableMeta: &types.TableMeta{ + TableName: "test_table", + }, + Rows: []types.RowImage{ + { + Columns: []types.ColumnImage{ + {ColumnName: "id", KeyType: types.IndexTypePrimaryKey, Value: 1}, + {ColumnName: "name", KeyType: types.IndexTypeNull, Value: "test"}, + }, + }, + }, + }, + } + + executor := newMySQLUndoInsertExecutor(sqlUndoLog) + sql, err := executor.buildUndoSQL(types.DBTypeMySQL) + assert.NoError(t, err) + assert.Contains(t, sql, "DELETE FROM") + assert.Contains(t, sql, "test_table") + assert.Contains(t, sql, "WHERE") +} + +func TestMySQLUndoInsertExecutor_BuildUndoSQL_EmptyRows(t *testing.T) { + sqlUndoLog := undo.SQLUndoLog{ + TableName: "test_table", + SQLType: types.SQLType_INSERT, + AfterImage: &types.RecordImage{ + TableName: "test_table", + Rows: []types.RowImage{}, + }, + } + + executor := newMySQLUndoInsertExecutor(sqlUndoLog) + sql, err := executor.buildUndoSQL(types.DBTypeMySQL) + assert.Error(t, err) + assert.Contains(t, err.Error(), "invalid undo log") + assert.Empty(t, sql) +} + +func TestMySQLUndoInsertExecutor_ExecuteOn(t *testing.T) { + db, mock, err := sqlmock.New() + require.NoError(t, err) + defer db.Close() + + sqlUndoLog := undo.SQLUndoLog{ + TableName: "test_table", + SQLType: types.SQLType_INSERT, + AfterImage: &types.RecordImage{ + TableName: "test_table", + TableMeta: &types.TableMeta{ + TableName: "test_table", + }, + Rows: []types.RowImage{ + { + Columns: []types.ColumnImage{ + {ColumnName: "id", KeyType: types.IndexTypePrimaryKey, Value: 1}, + {ColumnName: "name", KeyType: types.IndexTypeNull, Value: "test"}, + }, + }, + }, + }, + } + + executor := newMySQLUndoInsertExecutor(sqlUndoLog) + + // Mock the prepare and exec + mock.ExpectPrepare("DELETE FROM (.+)"). + ExpectExec(). + WithArgs(1). + WillReturnResult(sqlmock.NewResult(0, 1)) + + ctx := context.Background() + conn, err := db.Conn(ctx) + require.NoError(t, err) + defer conn.Close() + + err = executor.ExecuteOn(ctx, types.DBTypeMySQL, conn) + // Note: BaseExecutor.ExecuteOn is nil, so this will likely succeed Review Comment: This comment is unclear and potentially misleading. It suggests the test might not be testing the expected behavior. Either clarify what the test is actually verifying or fix the test to properly validate the ExecuteOn behavior. ```suggestion // Verify that ExecuteOn executes the expected SQL statement without error assert.NoError(t, err) ``` ########## pkg/datasource/sql/undo/mysql/undo_test.go: ########## @@ -0,0 +1,311 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 mysql + +import ( + "context" + "database/sql" + "database/sql/driver" + "testing" + + "github.com/DATA-DOG/go-sqlmock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "seata.apache.org/seata-go/pkg/datasource/sql/types" + "seata.apache.org/seata-go/pkg/datasource/sql/undo" +) + +func TestNewUndoLogManager(t *testing.T) { + manager := NewUndoLogManager() + assert.NotNil(t, manager) + assert.NotNil(t, manager.Base) +} + +func TestUndoLogManager_Init(t *testing.T) { + manager := NewUndoLogManager() + // Init should not panic + manager.Init() +} + +func TestUndoLogManager_DBType(t *testing.T) { + manager := NewUndoLogManager() + dbType := manager.DBType() + assert.Equal(t, types.DBTypeMySQL, dbType) +} + +func TestUndoLogManager_DeleteUndoLog(t *testing.T) { + db, mock, err := sqlmock.New() + require.NoError(t, err) + defer db.Close() + + manager := NewUndoLogManager() + ctx := context.Background() + xid := "test-xid-123" + branchID := int64(456) + + // Mock the connection + mock.ExpectBegin() + mock.ExpectPrepare("DELETE FROM (.+) WHERE branch_id = \\? AND xid = \\?"). + ExpectExec(). + WithArgs(branchID, xid). + WillReturnResult(sqlmock.NewResult(0, 1)) + mock.ExpectCommit() + + conn, err := db.Conn(ctx) + require.NoError(t, err) + defer conn.Close() + + // Start transaction to get the conn in proper state + tx, err := conn.BeginTx(ctx, nil) + require.NoError(t, err) + + err = manager.DeleteUndoLog(ctx, xid, branchID, conn) + assert.NoError(t, err) + + tx.Commit() + assert.NoError(t, mock.ExpectationsWereMet()) +} + +func TestUndoLogManager_DeleteUndoLog_PrepareError(t *testing.T) { + db, mock, err := sqlmock.New() + require.NoError(t, err) + defer db.Close() + + manager := NewUndoLogManager() + ctx := context.Background() + xid := "test-xid-123" + branchID := int64(456) + + // Mock connection and prepare error + mock.ExpectBegin() + mock.ExpectPrepare("DELETE FROM (.+) WHERE branch_id = \\? AND xid = \\?"). + WillReturnError(sql.ErrConnDone) + mock.ExpectRollback() + + conn, err := db.Conn(ctx) + require.NoError(t, err) + defer conn.Close() + + tx, err := conn.BeginTx(ctx, nil) + require.NoError(t, err) + + err = manager.DeleteUndoLog(ctx, xid, branchID, conn) + assert.Error(t, err) + + tx.Rollback() +} + +func TestUndoLogManager_BatchDeleteUndoLog(t *testing.T) { + db, mock, err := sqlmock.New() + require.NoError(t, err) + defer db.Close() + + manager := NewUndoLogManager() + ctx := context.Background() + xids := []string{"xid-1", "xid-2"} + branchIDs := []int64{100, 200} + + // Mock the batch delete + mock.ExpectBegin() + mock.ExpectPrepare("DELETE FROM (.+) WHERE branch_id IN (.+) AND xid IN (.+)"). + ExpectExec(). + WithArgs(sqlmock.AnyArg(), sqlmock.AnyArg()). + WillReturnResult(sqlmock.NewResult(0, 2)) + mock.ExpectCommit() + + conn, err := db.Conn(ctx) + require.NoError(t, err) + defer conn.Close() + + tx, err := conn.BeginTx(ctx, nil) + require.NoError(t, err) + + err = manager.BatchDeleteUndoLog(xids, branchIDs, conn) + assert.NoError(t, err) + + tx.Commit() + assert.NoError(t, mock.ExpectationsWereMet()) +} + +func TestUndoLogManager_BatchDeleteUndoLog_EmptySlices(t *testing.T) { + db, _, err := sqlmock.New() + require.NoError(t, err) + defer db.Close() + + manager := NewUndoLogManager() + ctx := context.Background() + + conn, err := db.Conn(ctx) + require.NoError(t, err) + defer conn.Close() + + // Test with empty slices + err = manager.BatchDeleteUndoLog([]string{}, []int64{}, conn) + assert.Error(t, err) +} + +func TestUndoLogManager_FlushUndoLog(t *testing.T) { + db, mock, err := sqlmock.New() + require.NoError(t, err) + defer db.Close() + + manager := NewUndoLogManager() + + // Create a mock driver.Conn + driverConn, err := db.Conn(context.Background()) + require.NoError(t, err) + defer driverConn.Close() + + // Create transaction context with empty round images + tranCtx := &types.TransactionContext{ + XID: "test-xid", + BranchID: 123, + RoundImages: &types.RecordImages{ + BeforeImages: []*types.RecordImage{}, + AfterImages: []*types.RecordImage{}, + }, + } + + // Get raw driver connection + var rawConn driver.Conn + err = driverConn.Raw(func(dc interface{}) error { + rawConn = dc.(driver.Conn) + return nil + }) + require.NoError(t, err) + + // FlushUndoLog should return nil for empty images + err = manager.FlushUndoLog(tranCtx, rawConn) + assert.NoError(t, err) + + assert.NoError(t, mock.ExpectationsWereMet()) +} + +func TestUndoLogManager_FlushUndoLog_WithImages(t *testing.T) { + db, mock, err := sqlmock.New() + require.NoError(t, err) + defer db.Close() + + manager := NewUndoLogManager() + + // Create a mock driver.Conn + driverConn, err := db.Conn(context.Background()) + require.NoError(t, err) + defer driverConn.Close() + + // Create transaction context with images + tranCtx := &types.TransactionContext{ + XID: "test-xid", + BranchID: 123, + RoundImages: &types.RecordImages{ + BeforeImages: []*types.RecordImage{ + { + TableName: "test_table", + SQLType: types.SQLType_INSERT, + Rows: []*types.Row{}, + }, + }, + AfterImages: []*types.RecordImage{ + { + TableName: "test_table", + SQLType: types.SQLType_INSERT, + Rows: []*types.Row{ + { + Fields: []*types.Field{ + {Name: "id", Value: 1}, + }, + }, + }, + }, + }, + }, + } + + // Mock the insert operation + mock.ExpectPrepare("INSERT INTO (.+) VALUES"). + ExpectExec(). + WillReturnResult(sqlmock.NewResult(1, 1)) + + // Get raw driver connection + var rawConn driver.Conn + err = driverConn.Raw(func(dc interface{}) error { + rawConn = dc.(driver.Conn) + return nil + }) + require.NoError(t, err) + + // FlushUndoLog should succeed + err = manager.FlushUndoLog(tranCtx, rawConn) + // Error expected as we're using sqlmock which doesn't fully implement all driver features + // The test verifies the method is callable and handles the input Review Comment: This comment indicates the test doesn't fully validate the expected behavior. The comment suggests an error is expected but line 257 checks for no error in mock expectations. This is confusing and suggests the test may not be properly validating the FlushUndoLog behavior. ```suggestion // FlushUndoLog is expected to return an error as sqlmock does not fully implement all driver features. err = manager.FlushUndoLog(tranCtx, rawConn) assert.Error(t, err, "Expected error due to sqlmock limitations") ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
