Fix remaining misuses of capturing the default file descriptors in fl…

…ytectl unit tests (#5950) * Search and replace remaining instances of testutils.Setup() Signed-off-by: Eduardo Apolinario <[email protected]> * Add cleanup in to the `testing.T` Cleanup method Signed-off-by: Eduardo Apolinario <[email protected]> * Revert unintended change to executor Signed-off-by: Eduardo Apolinario <[email protected]> --------- Signed-off-by: Eduardo Apolinario <[email protected]> Co-authored-by: Eduardo Apolinario <[email protected]>
flyteorg · Nov 4, 2024 · 636cc23 · 636cc23
1 parent f745030
commit 636cc23
Show file tree

Hide file tree

Showing 53 changed files with 671 additions and 428 deletions.
diff --git a/flytectl/cmd/compile/compile_test.go b/flytectl/cmd/compile/compile_test.go
@@ -6,7 +6,7 @@ import (
 
 	config "github.com/flyteorg/flyte/flytectl/cmd/config/subcommand/compile"
 	cmdCore "github.com/flyteorg/flyte/flytectl/cmd/core"
-	u "github.com/flyteorg/flyte/flytectl/cmd/testutils"
+	"github.com/flyteorg/flyte/flytectl/cmd/testutils"
 	"github.com/spf13/cobra"
 	"github.com/stretchr/testify/assert"
 )
@@ -29,9 +29,7 @@ func TestCompileCommand(t *testing.T) {
 	// compiling via cobra command
 	compileCfg := config.DefaultCompileConfig
 	compileCfg.File = "testdata/valid-package.tgz"
-	var setup = u.Setup
-	s := setup()
-	defer s.TearDown()
+	s := testutils.Setup(t)
 	compileCmd := CreateCompileCommand()["compile"]
 	err := compileCmd.CmdFunc(context.Background(), []string{}, s.CmdCtx)
 	assert.Nil(t, err, "compiling via cmd returns err")

diff --git a/flytectl/cmd/create/create_test.go b/flytectl/cmd/create/create_test.go
@@ -4,14 +4,11 @@ import (
 	"sort"
 	"testing"
 
-	"github.com/flyteorg/flyte/flytectl/cmd/testutils"
 	"github.com/stretchr/testify/assert"
 )
 
 const testDataFolder = "../testdata/"
 
-var setup = testutils.Setup
-
 func TestCreateCommand(t *testing.T) {
 	createCommand := RemoteCreateCommand()
 	assert.Equal(t, createCommand.Use, "create")

diff --git a/flytectl/cmd/create/execution_test.go b/flytectl/cmd/create/execution_test.go
@@ -19,18 +19,18 @@ type createSuite struct {
 	suite.Suite
 	testutils.TestStruct
 	originalExecConfig ExecutionConfig
+	t                  *testing.T
 }
 
 func (s *createSuite) SetupTest() {
-	s.TestStruct = setup()
+	s.TestStruct = testutils.Setup(s.t)
 
 	// TODO: migrate to new command context from testutils
 	s.CmdCtx = cmdCore.NewCommandContext(s.MockClient, s.MockOutStream)
 	s.originalExecConfig = *executionConfig
 }
 
 func (s *createSuite) TearDownTest() {
-	defer s.TearDown()
 	orig := s.originalExecConfig
 	executionConfig = &orig
 	s.MockAdminClient.AssertExpectations(s.T())
@@ -331,5 +331,5 @@ func (s *createSuite) Test_CreateTaskExecution_DryRun() {
 }
 
 func TestCreateSuite(t *testing.T) {
-	suite.Run(t, &createSuite{originalExecConfig: *executionConfig})
+	suite.Run(t, &createSuite{originalExecConfig: *executionConfig, t: t})
 }
diff --git a/flytectl/cmd/create/execution_util_test.go b/flytectl/cmd/create/execution_util_test.go
@@ -6,6 +6,7 @@ import (
 	"testing"
 
 	"github.com/flyteorg/flyte/flytectl/cmd/config"
+	"github.com/flyteorg/flyte/flytectl/cmd/testutils"
 	"github.com/flyteorg/flyte/flyteidl/gen/pb-go/flyteidl/admin"
 	"github.com/flyteorg/flyte/flyteidl/gen/pb-go/flyteidl/core"
 	"github.com/stretchr/testify/assert"
@@ -45,8 +46,7 @@ func createExecutionUtilSetup() {
 }
 
 func TestCreateExecutionForRelaunch(t *testing.T) {
-	s := setup()
-	defer s.TearDown()
+	s := testutils.Setup(t)
 
 	createExecutionUtilSetup()
 	s.MockAdminClient.OnRelaunchExecutionMatch(s.Ctx, relaunchRequest).Return(executionCreateResponse, nil)
@@ -55,8 +55,7 @@ func TestCreateExecutionForRelaunch(t *testing.T) {
 }
 
 func TestCreateExecutionForRelaunchNotFound(t *testing.T) {
-	s := setup()
-	defer s.TearDown()
+	s := testutils.Setup(t)
 
 	createExecutionUtilSetup()
 	s.MockAdminClient.OnRelaunchExecutionMatch(s.Ctx, relaunchRequest).Return(nil, errors.New("unknown execution"))
@@ -67,8 +66,7 @@ func TestCreateExecutionForRelaunchNotFound(t *testing.T) {
 }
 
 func TestCreateExecutionForRecovery(t *testing.T) {
-	s := setup()
-	defer s.TearDown()
+	s := testutils.Setup(t)
 
 	createExecutionUtilSetup()
 	s.MockAdminClient.OnRecoverExecutionMatch(s.Ctx, recoverRequest).Return(executionCreateResponse, nil)
@@ -77,8 +75,7 @@ func TestCreateExecutionForRecovery(t *testing.T) {
 }
 
 func TestCreateExecutionForRecoveryNotFound(t *testing.T) {
-	s := setup()
-	defer s.TearDown()
+	s := testutils.Setup(t)
 
 	createExecutionUtilSetup()
 	s.MockAdminClient.OnRecoverExecutionMatch(s.Ctx, recoverRequest).Return(nil, errors.New("unknown execution"))
@@ -89,8 +86,7 @@ func TestCreateExecutionForRecoveryNotFound(t *testing.T) {
 
 func TestCreateExecutionRequestForWorkflow(t *testing.T) {
 	t.Run("successful", func(t *testing.T) {
-		s := setup()
-		defer s.TearDown()
+		s := testutils.Setup(t)
 
 		createExecutionUtilSetup()
 		launchPlan := &admin.LaunchPlan{}
@@ -100,8 +96,7 @@ func TestCreateExecutionRequestForWorkflow(t *testing.T) {
 		assert.NotNil(t, execCreateRequest)
 	})
 	t.Run("successful with envs", func(t *testing.T) {
-		s := setup()
-		defer s.TearDown()
+		s := testutils.Setup(t)
 
 		createExecutionUtilSetup()
 		launchPlan := &admin.LaunchPlan{}
@@ -114,8 +109,7 @@ func TestCreateExecutionRequestForWorkflow(t *testing.T) {
 		assert.NotNil(t, execCreateRequest)
 	})
 	t.Run("successful with empty envs", func(t *testing.T) {
-		s := setup()
-		defer s.TearDown()
+		s := testutils.Setup(t)
 
 		createExecutionUtilSetup()
 		launchPlan := &admin.LaunchPlan{}
@@ -128,8 +122,7 @@ func TestCreateExecutionRequestForWorkflow(t *testing.T) {
 		assert.NotNil(t, execCreateRequest)
 	})
 	t.Run("successful with execution Cluster label and envs", func(t *testing.T) {
-		s := setup()
-		defer s.TearDown()
+		s := testutils.Setup(t)
 
 		createExecutionUtilSetup()
 		launchPlan := &admin.LaunchPlan{}
@@ -144,8 +137,7 @@ func TestCreateExecutionRequestForWorkflow(t *testing.T) {
 		assert.Equal(t, "cluster", execCreateRequest.Spec.ExecutionClusterLabel.Value)
 	})
 	t.Run("failed literal conversion", func(t *testing.T) {
-		s := setup()
-		defer s.TearDown()
+		s := testutils.Setup(t)
 
 		createExecutionUtilSetup()
 		launchPlan := &admin.LaunchPlan{
@@ -162,8 +154,7 @@ func TestCreateExecutionRequestForWorkflow(t *testing.T) {
 		assert.Equal(t, fmt.Errorf("parameter [nilparam] has nil Variable"), err)
 	})
 	t.Run("failed fetch", func(t *testing.T) {
-		s := setup()
-		defer s.TearDown()
+		s := testutils.Setup(t)
 
 		createExecutionUtilSetup()
 		s.FetcherExt.OnFetchLPVersionMatch(s.Ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, fmt.Errorf("failed"))
@@ -173,8 +164,7 @@ func TestCreateExecutionRequestForWorkflow(t *testing.T) {
 		assert.Equal(t, err, errors.New("failed"))
 	})
 	t.Run("with security context", func(t *testing.T) {
-		s := setup()
-		defer s.TearDown()
+		s := testutils.Setup(t)
 
 		createExecutionUtilSetup()
 		executionConfig.KubeServiceAcct = "default"
@@ -190,8 +180,7 @@ func TestCreateExecutionRequestForWorkflow(t *testing.T) {
 
 func TestCreateExecutionRequestForTask(t *testing.T) {
 	t.Run("successful", func(t *testing.T) {
-		s := setup()
-		defer s.TearDown()
+		s := testutils.Setup(t)
 
 		createExecutionUtilSetup()
 		task := &admin.Task{
@@ -205,8 +194,7 @@ func TestCreateExecutionRequestForTask(t *testing.T) {
 		assert.NotNil(t, execCreateRequest)
 	})
 	t.Run("successful with envs", func(t *testing.T) {
-		s := setup()
-		defer s.TearDown()
+		s := testutils.Setup(t)
 
 		createExecutionUtilSetup()
 		task := &admin.Task{
@@ -223,8 +211,7 @@ func TestCreateExecutionRequestForTask(t *testing.T) {
 		assert.NotNil(t, execCreateRequest)
 	})
 	t.Run("successful with empty envs", func(t *testing.T) {
-		s := setup()
-		defer s.TearDown()
+		s := testutils.Setup(t)
 
 		createExecutionUtilSetup()
 		task := &admin.Task{
@@ -241,8 +228,7 @@ func TestCreateExecutionRequestForTask(t *testing.T) {
 		assert.NotNil(t, execCreateRequest)
 	})
 	t.Run("failed literal conversion", func(t *testing.T) {
-		s := setup()
-		defer s.TearDown()
+		s := testutils.Setup(t)
 
 		createExecutionUtilSetup()
 		task := &admin.Task{
@@ -267,8 +253,7 @@ func TestCreateExecutionRequestForTask(t *testing.T) {
 		assert.Equal(t, fmt.Errorf("variable [nilvar] has nil type"), err)
 	})
 	t.Run("failed fetch", func(t *testing.T) {
-		s := setup()
-		defer s.TearDown()
+		s := testutils.Setup(t)
 
 		createExecutionUtilSetup()
 		s.FetcherExt.OnFetchTaskVersionMatch(s.Ctx, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil, fmt.Errorf("failed"))
@@ -278,8 +263,7 @@ func TestCreateExecutionRequestForTask(t *testing.T) {
 		assert.Equal(t, err, errors.New("failed"))
 	})
 	t.Run("with security context", func(t *testing.T) {
-		s := setup()
-		defer s.TearDown()
+		s := testutils.Setup(t)
 
 		createExecutionUtilSetup()
 		executionConfig.KubeServiceAcct = "default"
@@ -316,8 +300,7 @@ func Test_resolveOverrides(t *testing.T) {
 }
 
 func TestCreateExecutionForRelaunchOverwritingCache(t *testing.T) {
-	s := setup()
-	defer s.TearDown()
+	s := testutils.Setup(t)
 
 	createExecutionUtilSetup()
 	executionConfig.OverwriteCache = true

diff --git a/flytectl/cmd/create/project_test.go b/flytectl/cmd/create/project_test.go
@@ -8,6 +8,7 @@ import (
 	"github.com/flyteorg/flyte/flytectl/clierrors"
 	"github.com/flyteorg/flyte/flytectl/cmd/config"
 	"github.com/flyteorg/flyte/flytectl/cmd/config/subcommand/project"
+	"github.com/flyteorg/flyte/flytectl/cmd/testutils"
 	"github.com/flyteorg/flyte/flyteidl/gen/pb-go/flyteidl/admin"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
@@ -36,13 +37,12 @@ func createProjectSetup() {
 	project.DefaultProjectConfig.Description = ""
 	config.GetConfig().Project = ""
 }
+
 func TestCreateProjectFunc(t *testing.T) {
-	s := setup()
-	defer s.TearDown()
+	s := testutils.Setup(t)
 
 	createProjectSetup()
 	defer s.TearDownAndVerify(t, "project created successfully.")
-	defer s.TearDown()
 	project.DefaultProjectConfig.ID = projectValue
 	project.DefaultProjectConfig.Name = projectValue
 	project.DefaultProjectConfig.Labels = map[string]string{}
@@ -54,12 +54,10 @@ func TestCreateProjectFunc(t *testing.T) {
 }
 
 func TestEmptyProjectID(t *testing.T) {
-	s := setup()
-	defer s.TearDown()
+	s := testutils.Setup(t)
 
 	createProjectSetup()
 	defer s.TearDownAndVerify(t, "")
-	defer s.TearDown()
 	project.DefaultProjectConfig = &project.ConfigProject{}
 	s.MockAdminClient.OnRegisterProjectMatch(s.Ctx, projectRegisterRequest).Return(nil, nil)
 	err := createProjectsCommand(s.Ctx, []string{}, s.CmdCtx)
@@ -68,12 +66,10 @@ func TestEmptyProjectID(t *testing.T) {
 }
 
 func TestEmptyProjectName(t *testing.T) {
-	s := setup()
-	defer s.TearDown()
+	s := testutils.Setup(t)
 
 	createProjectSetup()
 	defer s.TearDownAndVerify(t, "")
-	defer s.TearDown()
 	project.DefaultProjectConfig.ID = projectValue
 	project.DefaultProjectConfig.Labels = map[string]string{}
 	project.DefaultProjectConfig.Description = ""

diff --git a/flytectl/cmd/delete/delete_test.go b/flytectl/cmd/delete/delete_test.go
@@ -4,7 +4,6 @@ import (
 	"sort"
 	"testing"
 
-	"github.com/flyteorg/flyte/flytectl/cmd/testutils"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -13,8 +12,6 @@ const (
 	testDataInvalidAttrFile = "testdata/invalid_attribute.yaml"
 )
 
-var setup = testutils.Setup
-
 func TestDeleteCommand(t *testing.T) {
 	deleteCommand := RemoteDeleteCommand()
 	assert.Equal(t, deleteCommand.Use, "delete")

diff --git a/flytectl/cmd/delete/execution_test.go b/flytectl/cmd/delete/execution_test.go
@@ -5,6 +5,7 @@ import (
 	"testing"
 
 	"github.com/flyteorg/flyte/flytectl/cmd/config"
+	"github.com/flyteorg/flyte/flytectl/cmd/testutils"
 	"github.com/flyteorg/flyte/flyteidl/gen/pb-go/flyteidl/admin"
 	"github.com/flyteorg/flyte/flyteidl/gen/pb-go/flyteidl/core"
 	"github.com/stretchr/testify/assert"
@@ -32,7 +33,8 @@ func terminateExecutionSetup() {
 }
 
 func TestTerminateExecutionFunc(t *testing.T) {
-	s := setup()
+	s := testutils.Setup(t)
+
 	terminateExecutionSetup()
 	terminateExecResponse := &admin.ExecutionTerminateResponse{}
 	s.MockAdminClient.OnTerminateExecutionMatch(s.Ctx, terminateExecRequests[0]).Return(terminateExecResponse, nil)
@@ -45,7 +47,8 @@ func TestTerminateExecutionFunc(t *testing.T) {
 }
 
 func TestTerminateExecutionFuncWithError(t *testing.T) {
-	s := setup()
+	s := testutils.Setup(t)
+
 	terminateExecutionSetup()
 	terminateExecResponse := &admin.ExecutionTerminateResponse{}
 	s.MockAdminClient.OnTerminateExecutionMatch(s.Ctx, terminateExecRequests[0]).Return(nil, errors.New("failed to terminate"))
@@ -58,7 +61,8 @@ func TestTerminateExecutionFuncWithError(t *testing.T) {
 }
 
 func TestTerminateExecutionFuncWithPartialSuccess(t *testing.T) {
-	s := setup()
+	s := testutils.Setup(t)
+
 	terminateExecutionSetup()
 	terminateExecResponse := &admin.ExecutionTerminateResponse{}
 	s.MockAdminClient.OnTerminateExecutionMatch(s.Ctx, terminateExecRequests[0]).Return(terminateExecResponse, nil)