Fix calculation for appending failures

Failures should be included in totals but not completed values (e.g. completed is non-failing count only). This is based on the e2e plugin implementation upstream. Best to be consistent with that more than anything else since so many different versions of that implementation already exist. Signed-off-by: John Schnake <jschnake@vmware.com>
vmware-tanzu · Jan 18, 2022 · 7c8e495 · 7c8e495
1 parent aea30a0
commit 7c8e495
Show file tree

Hide file tree

Showing 3 changed files with 13 additions and 7 deletions.
diff --git a/pkg/plugin/aggregation/aggregator_test.go b/pkg/plugin/aggregation/aggregator_test.go
@@ -417,13 +417,13 @@ func TestProcessProgressUpdates(t *testing.T) {
 					expectedProgressUpdates: map[string]plugin.ProgressUpdate{"type1/global": {PluginName: "type1", Node: "global", Message: "hi"}},
 				}, {
 					progressUpdate:          plugin.ProgressUpdate{PluginName: "type1", Node: "global", Message: "msg2", AppendTotals: true, AppendCompleted: 3, AppendFailing: []string{"foo"}},
-					expectedProgressUpdates: map[string]plugin.ProgressUpdate{"type1/global": {PluginName: "type1", Node: "global", Message: "msg2", Total: 3, Completed: 3, Failures: []string{"foo"}}},
+					expectedProgressUpdates: map[string]plugin.ProgressUpdate{"type1/global": {PluginName: "type1", Node: "global", Message: "msg2", Total: 4, Completed: 3, Failures: []string{"foo"}}},
 				}, {
 					progressUpdate:          plugin.ProgressUpdate{PluginName: "type1", Node: "global", Message: "msg3", AppendTotals: true, AppendCompleted: 4},
-					expectedProgressUpdates: map[string]plugin.ProgressUpdate{"type1/global": {PluginName: "type1", Node: "global", Message: "msg3", Total: 7, Completed: 7, Failures: []string{"foo"}}},
+					expectedProgressUpdates: map[string]plugin.ProgressUpdate{"type1/global": {PluginName: "type1", Node: "global", Message: "msg3", Total: 8, Completed: 7, Failures: []string{"foo"}}},
 				}, {
 					progressUpdate:          plugin.ProgressUpdate{PluginName: "type1", Node: "global", Message: "msg4", AppendTotals: true, AppendCompleted: 5, AppendFailing: []string{"bar"}},
-					expectedProgressUpdates: map[string]plugin.ProgressUpdate{"type1/global": {PluginName: "type1", Node: "global", Message: "msg4", Total: 12, Completed: 12, Failures: []string{"foo", "bar"}}},
+					expectedProgressUpdates: map[string]plugin.ProgressUpdate{"type1/global": {PluginName: "type1", Node: "global", Message: "msg4", Total: 14, Completed: 12, Failures: []string{"foo", "bar"}}},
 				},
 			},
 		}, {
@@ -435,7 +435,7 @@ func TestProcessProgressUpdates(t *testing.T) {
 					expectedProgressUpdates: map[string]plugin.ProgressUpdate{"type1/global": {PluginName: "type1", Node: "global", Message: "hi"}},
 				}, {
 					progressUpdate:          plugin.ProgressUpdate{PluginName: "type1", Node: "global", Message: "msg2", AppendTotals: true, AppendCompleted: 3, AppendFailing: []string{"foo"}},
-					expectedProgressUpdates: map[string]plugin.ProgressUpdate{"type1/global": {PluginName: "type1", Node: "global", Message: "msg2", Total: 3, Completed: 3, Failures: []string{"foo"}}},
+					expectedProgressUpdates: map[string]plugin.ProgressUpdate{"type1/global": {PluginName: "type1", Node: "global", Message: "msg2", Total: 4, Completed: 3, Failures: []string{"foo"}}},
 				}, {
 					progressUpdate:          plugin.ProgressUpdate{PluginName: "type1", Node: "global", Message: "NO_APPEND", Total: 33, Completed: 33},
 					expectedProgressUpdates: map[string]plugin.ProgressUpdate{"type1/global": {PluginName: "type1", Node: "global", Message: "NO_APPEND", Total: 33, Completed: 33}},

diff --git a/pkg/plugin/interface.go b/pkg/plugin/interface.go
@@ -133,6 +133,7 @@ func CombineUpdates(p1, p2 ProgressUpdate) ProgressUpdate {
 		p1.Completed += p2.AppendCompleted
 		if p2.AppendTotals {
 			p1.Total += p2.AppendCompleted
+			p1.Total += int64(len(p2.AppendFailing))
 		}
 		if len(p2.AppendFailing) > 0 {
 			p1.Failures = append(p1.Failures, p2.AppendFailing...)

diff --git a/pkg/plugin/interface_test.go b/pkg/plugin/interface_test.go
@@ -25,8 +25,8 @@ func TestCombineUpdates(t *testing.T) {
 		}, {
 			desc:   "p2 appends totals",
 			p1:     ProgressUpdate{Total: 1, Completed: 1, Message: "foo"},
-			p2:     ProgressUpdate{AppendTotals: true, AppendCompleted: 5, Message: "bar"},
-			expect: ProgressUpdate{Total: 6, Completed: 6, Message: "bar"},
+			p2:     ProgressUpdate{AppendTotals: true, AppendCompleted: 5, AppendFailing: []string{"a"}, Message: "bar"},
+			expect: ProgressUpdate{Total: 7, Completed: 6, Failures: []string{"a"}, Message: "bar"},
 		}, {
 			desc:   "both appending",
 			p1:     ProgressUpdate{AppendTotals: false, AppendCompleted: 2, AppendFailing: []string{"a", "b"}, Message: "foo"},
@@ -36,7 +36,12 @@ func TestCombineUpdates(t *testing.T) {
 			desc:   "starting from empty",
 			p1:     ProgressUpdate{},
 			p2:     ProgressUpdate{Node: "nonempty", AppendTotals: true, AppendCompleted: 5, AppendFailing: []string{"c", "d"}, Message: "bar"},
-			expect: ProgressUpdate{Node: "nonempty", Completed: 5, Total: 5, Failures: []string{"c", "d"}, Message: "bar"},
+			expect: ProgressUpdate{Node: "nonempty", Completed: 5, Total: 7, Failures: []string{"c", "d"}, Message: "bar"},
+		}, {
+			desc:   "appending failure included in totals but not completed",
+			p1:     ProgressUpdate{},
+			p2:     ProgressUpdate{Node: "nonempty", AppendTotals: true, AppendFailing: []string{"c", "d"}, Message: "bar"},
+			expect: ProgressUpdate{Node: "nonempty", Total: 2, Failures: []string{"c", "d"}, Message: "bar"},
 		},
 	}
 	for _, tc := range testCases {