change require.protoEquals to support compare options

[fretz12]

What changed?

Added ProtoEqualIgnoreFields to protoassert and protorequire packages. Refactored 3 call sites in standalone_activity_test.go to use the new helper.

Why?

Comparing proto messages while ignoring specific fields required a verbose pattern (cmp.Diff + protocmp.Transform() + protocmp.IgnoreFields() + require.Empty). The new option extends ProtoEqual with a clean call site that infers the message type automatically. The Option func type and config struct are designed to support additional comparison options in the future without changing the ProtoEqual signature.

How did you test it?

• built
• run locally and tested manually
• covered by existing tests
• added new unit test(s)
• added new functional test(s)

[fretz12]

Food for thought.
There's some other interesting options we can potentially expose:

protocmp.SortRepeatedFields
protocmp.IgnoreEnums
cmpopts.EquateApproxTime
Also another approach is to make this a really generic function:
func ProtoEqualWithOptions(t assert.TestingT, a proto.Message, b proto.Message, opts ...cmp.Option) bool

and it can be used as such:
protorequire.ProtoEqualWithOptions(t, expected, actual, protocmp.IgnoreFields(&activitypb.ActivityExecutionInfo{}, "state_transition_count"), cmpopts.EquateApproxTime(5*time.Second), )

However, that leaks proto/cmp abstractions making the helper harder to use. I decided to make the helper focused on ignore fields, as that's our majority use case and the interface is very clean. I propose for other useful options, like EquateApproxTime, we create separate helpers. But open to thoughts

[spkane31]

By not exporting config all of the future Option definitions have to be here, which removes the ability to have a one-off Option for a specific test. Thoughts on exporting?

[fretz12]

I could see that case. Ideally, I don't want to leak these configs. What if we do an exported config helper:

func WithCmpOptions(opts ...cmp.Option) Option {
	return func(_ proto.Message, cfg *config) {
		cfg.cmpOpts = append(cfg.cmpOpts, opts...)
	}
}

And then call it like:

protorequire.ProtoEqual(t, a, b,
      protorequire.WithCmpOptions(protocmp.SortRepeatedFields(msg, "items")),                                                                                            
  )   

wdyt?

[long-nt-tran]

+1 on leaking less than more, I think it's also ok to punt on how to expose one-off option to the future as well when there's a use case

[spkane31]

Agree with @long-nt-tran , we can defer to when we have a usecase

[long-nt-tran]

Looks like the negative test's expected failure is leaking as the suite failure. We might need a mock for require.testingT interface, i.e.,

type mockT struct {
    failed bool
}

func (m *mockT) Errorf(format string, args ...interface{}) {
    m.failed = true
}

func (m *mockT) FailNow() {
    m.failed = true
}

then this test can be

	t.Run("partial ignore still fails", func(t *testing.T) {
		mt := &mockT{}
		protorequire.ProtoEqual(mt, a, b,
			protorequire.IgnoreFields(
				"status",
			),
		)
		if !mt.failed {
			t.Fatal("expected comparison to fail when not all differing fields are ignored")
		}
	})

[fretz12]

good call.. changed

[long-nt-tran]

LGTM, thanks for adding this!

[long-nt-tran]

+1 on leaking less than more, I think it's also ok to punt on how to expose one-off option to the future as well when there's a use case