Delete first run in AimUI (#195)

* fix: delete first run in aim ui (#158) * Add integration test to delete first run - row 0 * Change Errorf to New * Add unit test to test for negative row number in renumberRows * gofmt run_test * Add cases for (-1,0,1) * Use proper format for test --------- Co-authored-by: Geoffrey Wilson <[email protected]> Co-authored-by: Software Developer <[email protected]>
G-Research · Aug 22, 2023 · 3dc9e2b · 3dc9e2b
1 parent a92e205
commit 3dc9e2b
Show file tree

Hide file tree

Showing 6 changed files with 76 additions and 4 deletions.
diff --git a/go.mod b/go.mod
@@ -94,6 +94,7 @@ require (
 	golang.org/x/text v0.9.0 // indirect
 	golang.org/x/tools v0.9.1 // indirect
 	golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 // indirect
+	gopkg.in/DATA-DOG/go-sqlmock.v1 v1.3.0 // indirect
 	gopkg.in/ini.v1 v1.67.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 )
diff --git a/go.sum b/go.sum
@@ -616,6 +616,8 @@ google.golang.org/protobuf v1.23.0/go.mod h1:EGpADcykh3NcUnDUJcl1+ZksZNG86OlYog2
 google.golang.org/protobuf v1.23.1-0.20200526195155-81db48ad09cc/go.mod h1:EGpADcykh3NcUnDUJcl1+ZksZNG86OlYog2l/sGQquU=
 google.golang.org/protobuf v1.24.0/go.mod h1:r/3tXBNzIEhYS9I1OUVjXDlt8tc493IdKGjtUeSXeh4=
 google.golang.org/protobuf v1.25.0/go.mod h1:9JNX74DMeImyA3h4bdi1ymwjUzf21/xIlbajtzgsN7c=
+gopkg.in/DATA-DOG/go-sqlmock.v1 v1.3.0 h1:FVCohIoYO7IJoDDVpV2pdq7SgrMH6wHnuTyrdrxJNoY=
+gopkg.in/DATA-DOG/go-sqlmock.v1 v1.3.0/go.mod h1:OdE7CF6DbADk7lN8LIKRzRJTTZXIjtWgA5THM5lhBAw=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk=

diff --git a/pkg/api/mlflow/dao/repositories/experiment.go b/pkg/api/mlflow/dao/repositories/experiment.go
@@ -135,7 +135,7 @@ func (r ExperimentRepository) DeleteBatch(ctx context.Context, ids []*int32) err
 
 		// verify deletion
 		if len(experiments) != len(ids) {
-			return eris.Errorf("count of deleted experiments does not match length of ids input (invalid experiment ID?)")
+			return eris.New("count of deleted experiments does not match length of ids input (invalid experiment ID?)")
 		}
 
 		// renumbering the remainder runs

diff --git a/pkg/api/mlflow/dao/repositories/run.go b/pkg/api/mlflow/dao/repositories/run.go
@@ -172,7 +172,7 @@ func (r RunRepository) DeleteBatch(ctx context.Context, ids []string) error {
 		// verify deletion
 		// NOTE: tx.RowsAffected does not provide correct number of deleted, using the returning slice instead
 		if len(runs) != len(ids) {
-			return eris.Errorf("count of deleted runs does not match length of ids input (invalid run ID?)")
+			return eris.New("count of deleted runs does not match length of ids input (invalid run ID?)")
 		}
 
 		// renumber the remainder
@@ -266,8 +266,8 @@ func getMinRowNum(runs []models.Run) models.RowNum {
 
 // renumberRows will update the runs.row_num field with the correct ordinal
 func (r RunRepository) renumberRows(tx *gorm.DB, startWith models.RowNum) error {
-	if startWith <= models.RowNum(0) {
-		return eris.Errorf("attempting to renumber with 0 or less row number value")
+	if startWith < models.RowNum(0) {
+		return eris.New("attempting to renumber with less than 0 row number value")
 	}
 
 	if tx.Dialector.Name() == "postgres" {

diff --git a/pkg/api/mlflow/dao/repositories/run_test.go b/pkg/api/mlflow/dao/repositories/run_test.go
@@ -0,0 +1,64 @@
+package repositories
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"gopkg.in/DATA-DOG/go-sqlmock.v1"
+	"gorm.io/driver/postgres"
+	"gorm.io/gorm"
+
+	"github.com/G-Research/fasttrackml/pkg/api/mlflow/dao/models"
+)
+
+func Test_renumberRows(t *testing.T) {
+	testData := []struct {
+		name      string
+		startWith models.RowNum
+	}{
+		{
+			name:      "NegativeRowNumber",
+			startWith: models.RowNum(-1),
+		},
+		{
+			name:      "ZeroRowNumber",
+			startWith: models.RowNum(0),
+		},
+		{
+			name:      "PositiveRowNumber",
+			startWith: models.RowNum(1),
+		},
+	}
+
+	mockDb, mock, err := sqlmock.New()
+	if err != nil {
+		t.Fatalf("an error '%s' was not expected when opening a stub database connection", err)
+	}
+	defer mockDb.Close()
+
+	lockExpect := func() {
+		mock.ExpectExec("LOCK TABLE runs").WillReturnResult(sqlmock.NewResult(0, 1))
+		mock.ExpectExec(`UPDATE runs`).WillReturnResult(sqlmock.NewResult(0, 1))
+	}
+
+	dialector := postgres.New(postgres.Config{
+		Conn:       mockDb,
+		DriverName: "postgres",
+	})
+	db, _ := gorm.Open(dialector, &gorm.Config{})
+
+	repo := NewRunRepository(db)
+
+	for _, tc := range testData {
+		t.Run(tc.name, func(t *testing.T) {
+			if tc.startWith < 0 {
+				err := repo.renumberRows(db, tc.startWith)
+				assert.EqualError(t, err, "attempting to renumber with less than 0 row number value")
+			} else {
+				lockExpect()
+				err := repo.renumberRows(db, tc.startWith)
+				assert.NoError(t, err)
+			}
+		})
+	}
+}
diff --git a/tests/integration/golang/aim/run/delete_test.go b/tests/integration/golang/aim/run/delete_test.go
@@ -72,6 +72,11 @@ func (s *DeleteRunTestSuite) Test_Ok() {
 			request:          request.DeleteRunRequest{RunID: s.runs[1].ID},
 			expectedRunCount: 8,
 		},
+		{
+			name:             "DeleteFirstRun",
+			request:          request.DeleteRunRequest{RunID: s.runs[0].ID},
+			expectedRunCount: 7,
+		},
 	}
 	for _, tt := range tests {
 		s.T().Run(tt.name, func(T *testing.T) {