feat: improve e2e test for ArtifactGC (#9448)

* feat: improve e2e test for artifactgc Signed-off-by: Julie Vogelman <[email protected]> * fix: merge conflict Signed-off-by: Julie Vogelman <[email protected]> Signed-off-by: Julie Vogelman <[email protected]>
argoproj · Aug 31, 2022 · b941fbc · b941fbc
1 parent 94608d1
commit b941fbc
Show file tree

Hide file tree

Showing 2 changed files with 69 additions and 25 deletions.
diff --git a/test/e2e/artifacts_test.go b/test/e2e/artifacts_test.go
@@ -57,9 +57,10 @@ func (s *ArtifactsSuite) TestArtifactPassing() {
 }
 
 type artifactState struct {
-	key        string
-	bucketName string
-	deleted    bool
+	key                   string
+	bucketName            string
+	deletedAtWFCompletion bool
+	deletedAtWFDeletion   bool
 }
 
 func (s *ArtifactsSuite) TestArtifactGC() {
@@ -70,37 +71,41 @@ func (s *ArtifactsSuite) TestArtifactGC() {
 		CreateWorkflowTemplates()
 
 	for _, tt := range []struct {
-		workflowFile      string
-		expectedArtifacts []artifactState
+		workflowFile                 string
+		expectedArtifacts            []artifactState
+		expectedGCPodsOnWFCompletion int
 	}{
 		{
-			workflowFile: "@testdata/artifactgc/artgc-multi-strategy-multi-anno.yaml",
+			workflowFile:                 "@testdata/artifactgc/artgc-multi-strategy-multi-anno.yaml",
+			expectedGCPodsOnWFCompletion: 2,
 			expectedArtifacts: []artifactState{
-				artifactState{"first-on-completion-1", "my-bucket-2", true},
-				artifactState{"first-on-completion-2", "my-bucket-3", true},
-				artifactState{"first-no-deletion", "my-bucket-3", false},
-				artifactState{"second-on-deletion", "my-bucket-3", true},
-				artifactState{"second-on-completion", "my-bucket-2", true},
+				artifactState{"first-on-completion-1", "my-bucket-2", true, false},
+				artifactState{"first-on-completion-2", "my-bucket-3", true, false},
+				artifactState{"first-no-deletion", "my-bucket-3", false, false},
+				artifactState{"second-on-deletion", "my-bucket-3", false, true},
+				artifactState{"second-on-completion", "my-bucket-2", true, false},
 			},
 		},
 		{
-			workflowFile: "@testdata/artifactgc/artgc-from-template.yaml",
+			workflowFile:                 "@testdata/artifactgc/artgc-from-template.yaml",
+			expectedGCPodsOnWFCompletion: 1,
 			expectedArtifacts: []artifactState{
-				artifactState{"on-completion", "my-bucket-2", true},
-				artifactState{"on-deletion", "my-bucket-2", true},
+				artifactState{"on-completion", "my-bucket-2", true, false},
+				artifactState{"on-deletion", "my-bucket-2", false, true},
 			},
 		},
 		{
-			workflowFile: "@testdata/artifactgc/artgc-step-wf-tmpl.yaml",
+			workflowFile:                 "@testdata/artifactgc/artgc-step-wf-tmpl.yaml",
+			expectedGCPodsOnWFCompletion: 1,
 			expectedArtifacts: []artifactState{
-				artifactState{"on-completion", "my-bucket-2", true},
-				artifactState{"on-deletion", "my-bucket-2", true},
+				artifactState{"on-completion", "my-bucket-2", true, false},
+				artifactState{"on-deletion", "my-bucket-2", false, true},
 			},
 		},
 	} {
 		// for each test make sure that:
 		// 1. the finalizer gets added
-		// 2. the artifacts are deleted
+		// 2. the artifacts are deleted at the right time
 		// 3. the finalizer gets removed after all artifacts are deleted
 		// (note that in order to verify that the finalizer has been added once the Workflow's been submitted,
 		// we need it to still be there after being submitted, so each of the following tests includes at least one
@@ -117,6 +122,32 @@ func (s *ArtifactsSuite) TestArtifactGC() {
 				assert.Contains(t, objectMeta.Finalizers, common.FinalizerArtifactGC)
 			})
 
+		// wait for all pods to have started and been completed and recouped
+		when.
+			WaitForWorkflow(
+				fixtures.WorkflowCompletionOkay(true),
+				fixtures.Condition(func(wf *wfv1.Workflow) (bool, string) {
+					return len(wf.Status.ArtifactGCStatus.PodsRecouped) >= tt.expectedGCPodsOnWFCompletion,
+						fmt.Sprintf("for all %d pods to have been recouped", tt.expectedGCPodsOnWFCompletion)
+				}))
+
+		then := when.Then()
+
+		// verify that the artifacts that should have been deleted at completion time were
+		for _, expectedArtifact := range tt.expectedArtifacts {
+			if expectedArtifact.deletedAtWFCompletion {
+				fmt.Printf("verifying artifact %s is deleted at completion time\n", expectedArtifact.key)
+				then.ExpectArtifactByKey(expectedArtifact.key, expectedArtifact.bucketName, func(t *testing.T, object minio.ObjectInfo, err error) {
+					assert.NotNil(t, err)
+				})
+			} else {
+				fmt.Printf("verifying artifact %s is not deleted at completion time\n", expectedArtifact.key)
+				then.ExpectArtifactByKey(expectedArtifact.key, expectedArtifact.bucketName, func(t *testing.T, object minio.ObjectInfo, err error) {
+					assert.Nil(t, err)
+				})
+			}
+		}
+
 		fmt.Println("deleting workflow; verifying that Artifact GC finalizer gets removed")
 
 		when.
@@ -125,10 +156,14 @@ func (s *ArtifactsSuite) TestArtifactGC() {
 
 		when = when.RemoveFinalizers(false) // just in case - if the above test failed we need to forcibly remove the finalizer for Artifact GC
 
-		then := when.Then()
+		then = when.Then()
 
 		for _, expectedArtifact := range tt.expectedArtifacts {
-			if expectedArtifact.deleted {
+
+			if expectedArtifact.deletedAtWFCompletion { // already checked this
+				continue
+			}
+			if expectedArtifact.deletedAtWFDeletion {
 				fmt.Printf("verifying artifact %s is deleted\n", expectedArtifact.key)
 				then.ExpectArtifactByKey(expectedArtifact.key, expectedArtifact.bucketName, func(t *testing.T, object minio.ObjectInfo, err error) {
 					assert.NotNil(t, err)

diff --git a/test/e2e/fixtures/when.go b/test/e2e/fixtures/when.go
@@ -233,12 +233,16 @@ var ToBeWaitingOnAMutex Condition = func(wf *wfv1.Workflow) (bool, string) {
 	return wf.Status.Synchronization != nil && wf.Status.Synchronization.Mutex != nil, "to be waiting on a mutex"
 }
 
+type WorkflowCompletionOkay bool
+
 // Wait for a workflow to meet a condition:
 // Options:
 // * `time.Duration` - change the timeout - 30s by default
 // * `string` - either:
 //    * the workflow's name (not spaces)
 //    * or a new message (if it contain spaces) - default "to finish"
+// * `WorkflowCompletionOkay`` (bool alias): if this is true, we won't stop checking for the other options
+//    * just because the Workflow completed
 // * `Condition` - a condition - `ToFinish` by default
 func (w *When) WaitForWorkflow(options ...interface{}) *When {
 	w.t.Helper()
@@ -248,6 +252,7 @@ func (w *When) WaitForWorkflow(options ...interface{}) *When {
 		workflowName = w.wf.Name
 	}
 	condition := ToBeDone
+	var workflowCompletionOkay WorkflowCompletionOkay
 	for _, opt := range options {
 		switch v := opt.(type) {
 		case time.Duration:
@@ -256,6 +261,8 @@ func (w *When) WaitForWorkflow(options ...interface{}) *When {
 			workflowName = v
 		case Condition:
 			condition = v
+		case WorkflowCompletionOkay:
+			workflowCompletionOkay = v
 		default:
 			w.t.Fatal("unknown option type: " + reflect.TypeOf(opt).String())
 		}
@@ -294,11 +301,13 @@ func (w *When) WaitForWorkflow(options ...interface{}) *When {
 					w.wf = wf
 					return w
 				}
-				// once done the workflow is done, the condition can never be met
-				// rather than wait maybe 30s for something that can never happen
-				if ok, _ = ToBeDone(wf); ok {
-					w.t.Errorf("condition never and cannot be met because the workflow is done")
-					return w
+				if !workflowCompletionOkay {
+					// once done the workflow is done, the condition can never be met
+					// rather than wait maybe 30s for something that can never happen
+					if ok, _ = ToBeDone(wf); ok {
+						w.t.Errorf("condition never and cannot be met because the workflow is done")
+						return w
+					}
 				}
 			} else {
 				w.t.Errorf("not ok")