Refactor how VisitPayloads is tested

[chrsmith]

What changed?

When building api-go after picking up changes from a feature I was working on, I got an expected test failure... but figuring out why it failed, and what the func populatePayload(root *proto.Message, msg proto.Message, require *require.Assertions, totalCount *int, count *int) function actually did was harder than I'd care to admit.

This PR refactors the test code such that the (kinda massive) populatePayload(...) function could be replaced with a new type PayloadWalker struct. Which will keep track of how many messages it has seen, the number of Payload(s) visited, and so on.

Why?

There are several motivations for this refactoring:

• Provide an actual error message instead of a stack overflow if encountering recursive proto types. (We don't have any today, but if you tried to add one... it wouldn't go over well.)

• Update the tests to use a more common "table-driven" style.

• Provide better contextual information in case the expected number of payloads changes.

Previously, you'd just see that "7 != 8". Now, the PayloadWalker type will keep track of the payloads it has visited and provide much more detail about what would have caused a test failure.

Example output:

expected 5 payloads reachable from temporal.api.workflowservice.v1.RespondWorkflowTaskFailedRequest, found 6

temporal.api.workflowservice.v1.RespondWorkflowTaskFailedRequest: 25 messages visited, 6 payloads seen
payloads seen:
1. temporal.api.workflowservice.v1.RespondWorkflowTaskFailedRequest.failure.encoded_attributes [temporal.api.common.v1.Payload]
2. temporal.api.workflowservice.v1.RespondWorkflowTaskFailedRequest.failure.application_failure_info.details [temporal.api.common.v1.Payloads]
3. temporal.api.workflowservice.v1.RespondWorkflowTaskFailedRequest.failure.timeout_failure_info.last_heartbeat_details [temporal.api.common.v1.Payloads]
4. temporal.api.workflowservice.v1.RespondWorkflowTaskFailedRequest.failure.canceled_failure_info.details [temporal.api.common.v1.Payloads]
5. temporal.api.workflowservice.v1.RespondWorkflowTaskFailedRequest.failure.reset_workflow_failure_info.last_heartbeat_details [temporal.api.common.v1.Payloads]
6. temporal.api.workflowservice.v1.RespondWorkflowTaskFailedRequest.messages.body [google.protobuf.Any]
subtrees skipped:
- temporal.api.workflowservice.v1.RespondWorkflowTaskFailedRequest.failure.cause (self-referential field)

How did you test it?

make test

Potential risks

The refactoring was done by my pal Claude. So it's possible this is GenAI slop and/or deeply flawed. But nothing seems out of place from a cursory review.

Meta point: I'm happy to cut down on the verbose comments Claude left, if that's something that bothers you. I agree it's kinda needless. But didn't want to spend more time massaging the code.

FWIW, this also apparently fixes a bug that was already present in how maps were visited.

The old code mapped Uint64Kind to protoreflect.ValueOf(int64(1)).MapKey(), which would have panicked on a uint64-keyed map; the new version correctly uses ValueOfUint64.

[yuandrew]

Could we test one level of self-reference here? I tried removing o.GetCause() from the production visitor, and the full proxy test suite still passed even though payloads in nested failures were no longer visited. Either walking one level before cutting the cycle or adding a focused test with a payload under Failure.Cause should cover this.

[chrsmith]

I don't think we want to modify the PayloadWalker to "visit deeper", since this test harness is really just using the type information. (So if T is sound, there is no need to check T again.)

But if I understand correctly, there is a testing gap here, in that PayloadWalker can only validate things known statically. Whereas the actual VisitPayloads(...) function actually operates on proto objects.

So I created a testcase that instantiates a failurepb.Failure with nested cause, failure_info, and encoded_attribute fields set. And it confirms that VisitPayloads(...) correctly visits each Payload.

[chrsmith]

@yuandrew PTAL.

I don't think it makes sense to tweak the logic of the PayloadWalker type, since it is just crawling the static type data. But I added a test for that recursive / self-referential field situation, that the PayloadWalker skips. (Since that's definitely a good edge case we want to cover!)

The updated diff is unintelligible, so just checkout the commit:
028c531