Fix HasBeenTested for all corner cases

api/v2/snapshot_types.go

@@ -49,6 +49,9 @@ const (
 	// snapshotTestPhaseFailed indicates that the test of the release the snapshot
 	// was taken from has failed.
 	snapshotTestPhaseFailed = "Failed"
+	// snapshotTestPhaseSucceeded indicates that the test of the release the snapshot
+	// was taken from has succeeded.
+	snapshotTestPhaseSucceeded = "Succeeded"
 )
 
 // Snapshots is a list of Snapshot objects.
@@ -209,10 +212,26 @@ func (in *Snapshot) VersionedChartName() string {
 	return fmt.Sprintf("%s@%s", in.ChartName, in.ChartVersion)
 }
 
-// HasBeenTested returns true if TestHooks is not nil. This includes an empty
-// map, which indicates the chart has no tests.
+// HasBeenTested returns true if test results have been observed. This includes
+// an empty map, which indicates the chart has no selected tests.
 func (in *Snapshot) HasBeenTested() bool {
-	return in != nil && in.TestHooks != nil
+	if in == nil || in.TestHooks == nil {
+		return false
+	}
+	if len(*in.TestHooks) == 0 {
+		return true
+	}
+	for _, th := range *in.TestHooks {
+		if th != nil && th.Phase == snapshotTestPhaseFailed {
+			return true
+		}
+	}
+	for _, th := range *in.TestHooks {
+		if th == nil || th.Phase != snapshotTestPhaseSucceeded {
+			return false
+		}
+	}
+	return true
 }
 
 // GetTestHooks returns the TestHooks for the release if not nil.
@@ -227,7 +246,7 @@ func (in *Snapshot) GetTestHooks() map[string]*TestHookStatus {
 func (in *Snapshot) HasTestInPhase(phase string) bool {
 	if in != nil {
 		for _, h := range in.GetTestHooks() {
-			if h.Phase == phase {
+			if h != nil && h.Phase == phase {
 				return true
 			}
 		}

api/v2/snapshot_types_test.go

@@ -95,6 +95,98 @@ func TestSnapshots_Latest(t *testing.T) {
 	}
 }
 
+func TestSnapshots_HasBeenTested(t *testing.T) {
+	tests := []struct {
+		name string
+		in   Snapshot
+		want bool
+	}{
+		{
+			name: "no testhooks",
+			in:   Snapshot{Version: 2, Status: "deployed"},
+			want: false,
+		},
+		{
+			name: "empty testhooks",
+			in:   Snapshot{Version: 2, Status: "deployed", TestHooks: &map[string]*TestHookStatus{}},
+			want: true,
+		},
+		{
+			name: "all testhooks are zero",
+			in: Snapshot{Version: 2, Status: "deployed", TestHooks: &map[string]*TestHookStatus{
+				"test1": &TestHookStatus{},
+			}},
+			want: false,
+		},
+		{
+			name: "nil testhook",
+			in: Snapshot{Version: 2, Status: "deployed", TestHooks: &map[string]*TestHookStatus{
+				"test1": nil,
+			}},
+			want: false,
+		},
+		{
+			name: "all testhooks succeeded",
+			in: Snapshot{Version: 2, Status: "deployed", TestHooks: &map[string]*TestHookStatus{
+				"test1": {Phase: "Succeeded"},
+			}},
+			want: true,
+		},
+		{
+			name: "all testhooks failed",
+			in: Snapshot{Version: 2, Status: "deployed", TestHooks: &map[string]*TestHookStatus{
+				"test1": {Phase: "Failed"},
+			}},
+			want: true,
+		},
+		{
+			name: "mixed terminal testhooks",
+			in: Snapshot{Version: 2, Status: "deployed", TestHooks: &map[string]*TestHookStatus{
+				"test1": {Phase: "Succeeded"},
+				"test2": {Phase: "Failed"},
+			}},
+			want: true,
+		},
+		{
+			name: "mixed failed and zero testhooks",
+			in: Snapshot{Version: 2, Status: "deployed", TestHooks: &map[string]*TestHookStatus{
+				"test1": {Phase: "Failed"},
+				"test2": {},
+			}},
+			want: true,
+		},
+		{
+			name: "mixed succeeded and zero testhooks",
+			in: Snapshot{Version: 2, Status: "deployed", TestHooks: &map[string]*TestHookStatus{
+				"test1": {Phase: "Succeeded"},
+				"test2": {},
+			}},
+			want: false,
+		},
+		{
+			name: "testhook still running",
+			in: Snapshot{Version: 2, Status: "deployed", TestHooks: &map[string]*TestHookStatus{
+				"test1": {Phase: "Running"},
+			}},
+			want: false,
+		},
+		{
+			name: "testhook phase unknown",
+			in: Snapshot{Version: 2, Status: "deployed", TestHooks: &map[string]*TestHookStatus{
+				"test1": {Phase: "Unknown"},
+			}},
+			want: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := tt.in.HasBeenTested(); got != tt.want {
+				t.Errorf("HasBeenTested() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
 func TestSnapshots_Previous(t *testing.T) {
 	tests := []struct {
 		name        string

internal/reconcile/state_test.go

@@ -220,6 +220,44 @@ func Test_DetermineReleaseState(t *testing.T) {
 				Status: ReleaseStatusUntested,
 			},
 		},
+		{
+			name: "incomplete test result",
+			releases: []*helmrelease.Release{
+				testutil.BuildRelease(&helmrelease.MockReleaseOptions{
+					Name:      mockReleaseName,
+					Namespace: mockReleaseNamespace,
+					Version:   1,
+					Status:    helmreleasecommon.StatusDeployed,
+					Chart:     testutil.BuildChart(),
+				}, testutil.ReleaseWithConfig(map[string]any{"foo": "bar"})),
+			},
+			spec: func(spec *v2.HelmReleaseSpec) {
+				spec.Test = &v2.Test{
+					Enable: true,
+				}
+			},
+			status: func(releases []*helmrelease.Release) v2.HelmReleaseStatus {
+				cur := release.ObservedToSnapshot(release.ObserveRelease(releases[0]))
+				cur.SetTestHooks(map[string]*v2.TestHookStatus{
+					"passing-test": {
+						Phase: helmrelease.HookPhaseSucceeded.String(),
+					},
+					"never-run-test": {},
+				})
+
+				return v2.HelmReleaseStatus{
+					History: v2.Snapshots{
+						cur,
+					},
+					LastAttemptedReleaseAction: v2.ReleaseActionUpgrade,
+				}
+			},
+			chart:  testutil.BuildChart(),
+			values: map[string]any{"foo": "bar"},
+			want: ReleaseState{
+				Status: ReleaseStatusUntested,
+			},
+		},
 		{
 			name: "failed test",
 			releases: []*helmrelease.Release{
@@ -265,6 +303,47 @@ func Test_DetermineReleaseState(t *testing.T) {
 				Status: ReleaseStatusFailed,
 			},
 		},
+		{
+			name: "failed test with unstarted remaining test hooks",
+			releases: []*helmrelease.Release{
+				testutil.BuildRelease(
+					&helmrelease.MockReleaseOptions{
+						Name:      mockReleaseName,
+						Namespace: mockReleaseNamespace,
+						Version:   2,
+						Status:    helmreleasecommon.StatusDeployed,
+						Chart:     testutil.BuildChart(),
+					},
+					testutil.ReleaseWithConfig(map[string]any{"foo": "bar"}),
+				),
+			},
+			spec: func(spec *v2.HelmReleaseSpec) {
+				spec.Test = &v2.Test{
+					Enable: true,
+				}
+			},
+			status: func(releases []*helmrelease.Release) v2.HelmReleaseStatus {
+				cur := release.ObservedToSnapshot(release.ObserveRelease(releases[0]))
+				cur.SetTestHooks(map[string]*v2.TestHookStatus{
+					"failure-tests": {
+						Phase: helmrelease.HookPhaseFailed.String(),
+					},
+					"never-run-test": {},
+				})
+
+				return v2.HelmReleaseStatus{
+					History: v2.Snapshots{
+						cur,
+					},
+					LastAttemptedReleaseAction: v2.ReleaseActionUpgrade,
+				}
+			},
+			chart:  testutil.BuildChart(),
+			values: map[string]any{"foo": "bar"},
+			want: ReleaseState{
+				Status: ReleaseStatusFailed,
+			},
+		},
 		{
 			name: "failed test with ignore failures set",
 			releases: []*helmrelease.Release{

internal/reconcile/test.go

@@ -207,7 +207,7 @@ func observeTest(obj *v2.HelmRelease) storage.ObserveFunc {
 		// Update the latest snapshot with the test result.
 		latest := obj.Status.History.Latest()
 		tested := release.ObservedToSnapshot(releaseToObservation(rls, latest, latest.GetAction()))
-		tested.SetTestHooks(release.TestHooksFromRelease(rls))
+		tested.SetTestHooks(release.TestHooksFromRelease(rls, obj.GetTest().GetFilters()...))
 		obj.Status.History[0] = tested
 	}
 }

internal/reconcile/test_test.go

@@ -410,6 +410,46 @@ func Test_observeTest(t *testing.T) {
 		}))
 	})
 
+	t.Run("test with filters", func(t *testing.T) {
+		g := NewWithT(t)
+
+		filters := []v2.Filter{
+			{Name: "passing-test"},
+		}
+		obj := &v2.HelmRelease{
+			Spec: v2.HelmReleaseSpec{
+				Test: &v2.Test{
+					Filters: &filters,
+				},
+			},
+			Status: v2.HelmReleaseStatus{
+				History: v2.Snapshots{
+					&v2.Snapshot{
+						Name:      mockReleaseName,
+						Namespace: mockReleaseNamespace,
+						Version:   1,
+					},
+				},
+			},
+		}
+		rls := testutil.BuildRelease(&helmrelease.MockReleaseOptions{
+			Name:      mockReleaseName,
+			Namespace: mockReleaseNamespace,
+			Version:   1,
+		}, testutil.ReleaseWithHooks(testHookFixtures))
+
+		expect := release.ObservedToSnapshot(release.ObserveRelease(rls))
+		expect.SetTestHooks(release.TestHooksFromRelease(rls, filters...))
+
+		observeTest(obj)(rls)
+		g.Expect(obj.Status.History).To(testutil.Equal(v2.Snapshots{
+			expect,
+		}))
+		g.Expect(obj.Status.History.Latest().GetTestHooks()).To(HaveKey("passing-test"))
+		g.Expect(obj.Status.History.Latest().GetTestHooks()).ToNot(HaveKey("never-run-test"))
+		g.Expect(obj.Status.History.Latest().GetTestHooks()).ToNot(HaveKey("failing-test"))
+	})
+
 	t.Run("test with current preserves action", func(t *testing.T) {
 		g := NewWithT(t)
 

internal/release/observation.go

@@ -182,10 +182,14 @@ func ObservedToSnapshot(rls Observation) *v2.Snapshot {
 }
 
 // TestHooksFromRelease returns the list of v2.TestHookStatus for the
-// given release, indexed by name.
-func TestHooksFromRelease(rls *helmrelease.Release) map[string]*v2.TestHookStatus {
+// given release, indexed by name. When filters are provided, only hooks
+// selected by the filters are returned.
+func TestHooksFromRelease(rls *helmrelease.Release, filters ...v2.Filter) map[string]*v2.TestHookStatus {
 	hooks := make(map[string]*v2.TestHookStatus)
 	for k, v := range GetTestHooks(rls) {
+		if !isTestHookSelected(k, filters) {
+			continue
+		}
 		var h *v2.TestHookStatus
 		if v != nil {
 			h = &v2.TestHookStatus{
@@ -198,3 +202,23 @@ func TestHooksFromRelease(rls *helmrelease.Release) map[string]*v2.TestHookStatu
 	}
 	return hooks
 }
+
+func isTestHookSelected(name string, filters []v2.Filter) bool {
+	var (
+		hasInclude bool
+		included   bool
+	)
+	for _, f := range filters {
+		if f.Exclude {
+			if f.Name == name {
+				return false
+			}
+			continue
+		}
+		hasInclude = true
+		if f.Name == name {
+			included = true
+		}
+	}
+	return !hasInclude || included
+}