Skip to content

Refactor how VisitPayloads is tested - #304

Open
chrsmith wants to merge 1 commit into
mainfrom
chrsmith/payload-walker-refactor
Open

Refactor how VisitPayloads is tested#304
chrsmith wants to merge 1 commit into
mainfrom
chrsmith/payload-walker-refactor

Conversation

@chrsmith

@chrsmith chrsmith commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

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.

@chrsmith
chrsmith requested review from a team as code owners July 28, 2026 20:22
@spkane31
spkane31 self-requested a review July 28, 2026 21:46
func (w *PayloadWalker) walkMessageField(m protoreflect.Message, fd protoreflect.FieldDescriptor) {
// A field of the same type as its parent (e.g. Failure.cause) would recurse
// forever. Payloads underneath it are reachable through the parent anyway.
if fd.Message().FullName() == m.Descriptor().FullName() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants