Refactor how VisitPayloads is tested - #304
Open
chrsmith wants to merge 1 commit into
Open
Conversation
spkane31
self-requested a review
July 28, 2026 21:46
yuandrew
reviewed
Jul 29, 2026
| 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() { |
Contributor
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed?
When building
api-goafter picking up changes from a feature I was working on, I got an expected test failure... but figuring out why it failed, and what thefunc 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 newtype PayloadWalker struct. Which will keep track of how many messages it has seen, the number ofPayload(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
PayloadWalkertype will keep track of the payloads it has visited and provide much more detail about what would have caused a test failure.Example output:
How did you test it?
make testPotential 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.
FWIW, this also apparently fixes a bug that was already present in how maps were visited.