From 3da058d79158df538a3daa6a4228f8b2ffcbdce9 Mon Sep 17 00:00:00 2001 From: Fred Tzeng <41805201+fretz12@users.noreply.github.com> Date: Tue, 16 Jun 2026 17:06:05 -0700 Subject: [PATCH] Fix flaky AttachLinksOnConflictUnionsLinks by comparing links as a set (#10740) ## What changed? - tests/activity_standalone_test.go: switched the assertion in AttachLinksOnConflictUnionsLinks from the order-sensitive protorequire.ProtoSliceEqual to the order-agnostic protorequire.ProtoElementsMatch. - common/testing/protorequire/require.go: added ProtoElementsMatch, and dropped the dependency on protoassert by inlining the comparison logic (using temporalproto.DeepEqual for element matching). - common/testing/protorequire/require_test.go: added TestProtoElementsMatch (in-order, out-of-order, struct-containing-proto, nested-proto, and a negative case) and TestDeepEqual (positive + negative). ## Why? The test was flaky: it passed in isolation but failed when run alongside the rest of the suite. Server-side, componentLinks (chasm/tree.go) builds the response by iterating a Go map keyed by request ID, so the order between request batches is non-deterministic. ProtoElementsMatch did not exist in protorequire, only in protoassert. Adding it (and removing the cross-package dependency) keeps protorequire self-contained and consistent with the other require-style helpers. ## How did you test it? - [X] built - [ ] run locally and tested manually - [X] covered by existing tests - [X] added new unit test(s) - [ ] added new functional test(s) --- common/testing/protorequire/require.go | 73 ++++++++-- common/testing/protorequire/require_test.go | 139 ++++++++++++++++++++ tests/activity_standalone_test.go | 3 +- 3 files changed, 200 insertions(+), 15 deletions(-) diff --git a/common/testing/protorequire/require.go b/common/testing/protorequire/require.go index e44e4fe3e0..ff035e0110 100644 --- a/common/testing/protorequire/require.go +++ b/common/testing/protorequire/require.go @@ -2,10 +2,11 @@ package protorequire import ( "fmt" + "reflect" "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/require" - "go.temporal.io/server/common/testing/protoassert" + "go.temporal.io/api/temporalproto" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/reflect/protoreflect" "google.golang.org/protobuf/testing/protocmp" @@ -60,8 +61,8 @@ func NotProtoEqual(t require.TestingT, a proto.Message, b proto.Message) { if th, ok := t.(helper); ok { th.Helper() } - if !protoassert.NotProtoEqual(t, a, b) { - t.FailNow() + if diff := cmp.Diff(a, b, protocmp.Transform()); diff == "" { + require.Fail(t, "Expected protos to differ but they did not") } } @@ -73,8 +74,57 @@ func ProtoSliceEqual[T proto.Message](t require.TestingT, a []T, b []T) { if th, ok := t.(helper); ok { th.Helper() } - if !protoassert.ProtoSliceEqual(t, a, b) { - t.FailNow() + if len(a) != len(b) { + require.Fail(t, fmt.Sprintf("Proto slice length mismatch: want %d, got %d", len(a), len(b))) + return + } + for i := range a { + if diff := cmp.Diff(a[i], b[i], protocmp.Transform()); diff != "" { + require.Fail(t, fmt.Sprintf("Proto mismatch at index %d (-want +got):\n%v", i, diff)) + return + } + } +} + +// ProtoElementsMatch behaves like require.ElementsMatch except in that it works for protobuf-generated structs. +// Both arguments must be slices or arrays. +func ProtoElementsMatch(t require.TestingT, a any, b any, msgAndArgs ...any) { + if th, ok := t.(helper); ok { + th.Helper() + } + aVal := reflect.ValueOf(a) + bVal := reflect.ValueOf(b) + if aVal.Len() != bVal.Len() { + require.Fail(t, fmt.Sprintf("element count mismatch: want %d, got %d\nA: %+v\nB: %+v", aVal.Len(), bVal.Len(), a, b), msgAndArgs...) + return + } + used := make([]bool, bVal.Len()) + for i := 0; i < aVal.Len(); i++ { + want := aVal.Index(i).Interface() + matched := false + for j := 0; j < bVal.Len(); j++ { + if used[j] { + continue + } + if temporalproto.DeepEqual(want, bVal.Index(j).Interface()) { + used[j] = true + matched = true + break + } + } + if !matched { + require.Fail(t, fmt.Sprintf("element not found in B: %+v\nA: %+v\nB: %+v", want, a, b), msgAndArgs...) + return + } + } +} + +func DeepEqual(t require.TestingT, a any, b any) { + if th, ok := t.(helper); ok { + th.Helper() + } + if !temporalproto.DeepEqual(a, b) { + require.Fail(t, "Values are not deeply equal") } } @@ -89,24 +139,19 @@ func (x ProtoAssertions) NotProtoEqual(a proto.Message, b proto.Message) { if th, ok := x.t.(helper); ok { th.Helper() } - if !protoassert.NotProtoEqual(x.t, a, b) { - x.t.FailNow() - } + NotProtoEqual(x.t, a, b) } func (x ProtoAssertions) DeepEqual(a any, b any) { if th, ok := x.t.(helper); ok { th.Helper() } - if !protoassert.DeepEqual(x.t, a, b) { - x.t.FailNow() - } + DeepEqual(x.t, a, b) } -func (x ProtoAssertions) ProtoElementsMatch(a any, b any) bool { +func (x ProtoAssertions) ProtoElementsMatch(a any, b any, msgAndArgs ...any) { if th, ok := x.t.(helper); ok { th.Helper() } - - return protoassert.ProtoElementsMatch(x.t, a, b) + ProtoElementsMatch(x.t, a, b, msgAndArgs...) } diff --git a/common/testing/protorequire/require_test.go b/common/testing/protorequire/require_test.go index 5488e509d5..7bf890ad4d 100644 --- a/common/testing/protorequire/require_test.go +++ b/common/testing/protorequire/require_test.go @@ -61,3 +61,142 @@ func TestProtoEqualIgnoreFields(t *testing.T) { } }) } + +type canHazProto struct { + A float64 + B *commonpb.WorkflowExecution +} + +func TestProtoElementsMatch(t *testing.T) { + for _, tc := range []struct { + Name string + A any + B any + }{{ + Name: "Shallow proto object - in order", + A: []*commonpb.WorkflowExecution{{ + WorkflowId: "some random workflow ID", + RunId: myUUID, + }, { + WorkflowId: "second workflow", + RunId: myUUID, + }}, + B: []*commonpb.WorkflowExecution{{ + WorkflowId: "some random workflow ID", + RunId: myUUID, + }, { + WorkflowId: "second workflow", + RunId: myUUID, + }}, + }, { + Name: "Shallow proto object - out of order", + A: []*commonpb.WorkflowExecution{{ + WorkflowId: "some random workflow ID", + RunId: myUUID, + }, { + WorkflowId: "second workflow", + RunId: myUUID, + }}, + B: []*commonpb.WorkflowExecution{{ + WorkflowId: "second workflow", + RunId: myUUID, + }, { + WorkflowId: "some random workflow ID", + RunId: myUUID, + }}, + }, { + Name: "Structs containing proto", + A: []canHazProto{{ + A: 13, + B: &commonpb.WorkflowExecution{ + WorkflowId: "some random workflow ID", + RunId: myUUID, + }, + }, { + A: 12, + B: &commonpb.WorkflowExecution{ + WorkflowId: "second random workflow ID", + RunId: myUUID, + }, + }}, + B: []canHazProto{{ + A: 12, + B: &commonpb.WorkflowExecution{ + WorkflowId: "second random workflow ID", + RunId: myUUID, + }, + }, { + A: 13, + B: &commonpb.WorkflowExecution{ + WorkflowId: "some random workflow ID", + RunId: myUUID, + }, + }}, + }, { + Name: "Nested proto struct", + A: []*workflowpb.WorkflowExecutionInfo{ + { + Execution: &commonpb.WorkflowExecution{ + WorkflowId: "some random workflow ID", + RunId: myUUID, + }, + }, { + Execution: &commonpb.WorkflowExecution{ + WorkflowId: "second random workflow ID", + RunId: myUUID, + }, + }}, + B: []*workflowpb.WorkflowExecutionInfo{ + { + Execution: &commonpb.WorkflowExecution{ + WorkflowId: "second random workflow ID", + RunId: myUUID, + }, + }, { + Execution: &commonpb.WorkflowExecution{ + WorkflowId: "some random workflow ID", + RunId: myUUID, + }, + }}, + }} { + t.Run(tc.Name, func(t *testing.T) { + mt := &mockT{} + protorequire.ProtoElementsMatch(mt, tc.A, tc.B) + if mt.failed { + t.Error("expected equality") + } + }) + } + + t.Run("mismatch fails", func(t *testing.T) { + a := []*commonpb.WorkflowExecution{{WorkflowId: "wf-a", RunId: myUUID}} + b := []*commonpb.WorkflowExecution{{WorkflowId: "wf-b", RunId: myUUID}} + mt := &mockT{} + protorequire.ProtoElementsMatch(mt, a, b) + if !mt.failed { + t.Fatal("expected comparison to fail for differing element sets") + } + }) +} + +func TestDeepEqual(t *testing.T) { + a := &commonpb.WorkflowExecution{WorkflowId: "wf-1", RunId: myUUID} + + t.Run("equal values pass", func(t *testing.T) { + b := &commonpb.WorkflowExecution{WorkflowId: "wf-1", RunId: myUUID} + mt := &mockT{} + protorequire.DeepEqual(mt, a, b) + if mt.failed { + t.Fatal("expected equal values to pass") + } + }) + + t.Run("differing values fail", func(t *testing.T) { + b := &commonpb.WorkflowExecution{WorkflowId: "wf-2", RunId: myUUID} + mt := &mockT{} + protorequire.DeepEqual(mt, a, b) + if !mt.failed { + t.Fatal("expected differing values to fail") + } + }) +} diff --git a/tests/activity_standalone_test.go b/tests/activity_standalone_test.go index 954efed009..7d89302867 100644 --- a/tests/activity_standalone_test.go +++ b/tests/activity_standalone_test.go @@ -885,7 +885,8 @@ func (s *standaloneActivityTestSuite) TestStart() { require.NoError(t, err) expected := append([]*commonpb.Link{}, firstLinks...) expected = append(expected, secondLinks...) - protorequire.ProtoSliceEqual(t, expected, descResp.GetInfo().GetLinks()) + // Links across requests are stored in a map keyed by request ID, so their relative order is non-deterministic. + protorequire.ProtoElementsMatch(t, expected, descResp.GetInfo().GetLinks()) }) t.Run("AttachLinksOnConflictStoresRawInput", func(t *testing.T) {