Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Formatting Cyclic Types produces infinite output #830

Open
meln5674 opened this issue Mar 9, 2025 · 5 comments · May be fixed by #831
Open

Formatting Cyclic Types produces infinite output #830

meln5674 opened this issue Mar 9, 2025 · 5 comments · May be fixed by #831

Comments

@meln5674
Copy link

meln5674 commented Mar 9, 2025

When using gomega (and, by extension, ginkgo) with cyclic types, the output from formatting the objects from e.g. Expect(foo).To(Equal(bar)) is infinite, and if the cyclic pointer field is early in the struct, the actual differing fields may never actually be printed. This renders this output nearly useless.

I would like to propose and implement adding a new struct tag gomega:"omit" which replaces the formatting output of that field with a message indicating it was omitted, plus the address if it was a pointer. This would allow cyclic types to be formatted finitely.

@onsi
Copy link
Owner

onsi commented Mar 10, 2025

hey thanks for bringing this up and proposing a solution (and including a PR!)

i'm open to the approach you've taken though we'll need to add some documentation to make it discoverable.

some thoughts though:

  • gomega:"omit" is scoped to formatting output - but on the face of it - i could imagine confusion about the scope. e.g. "I want Gomega's equality matcher to ignore this field when making comparisons - why doesn't gomega:"omit" work?"
  • for the specific issue of cyclic types it might make more sense to support this in format.Object "out of the box." Namely, Object could keep track of every pointer address it encounters and only descend into, and format, the pointer if it hasn't already seen it. This would solve the cyclic issue without the user having to explicitly annotate their code with a gomega tag.

Thoughts? Would you be up for giving the latter a try in the PR?

@thediveo
Copy link
Collaborator

thediveo commented Mar 10, 2025

That's a situation I'm constantly battling with in my lxkns and ghostwire packages, with their cyclic data structures. As onsi points out, an annotation probably doesn't help, because the annotation's placement would need to be test-specific, depending on where in the probably cyclic graph your test "starts". The cycle detection looks much more promising.

@meln5674
Copy link
Author

The cycle detection would be a better long-term solution, which I did consider at first, and I've be happy to try that as well, however, this was more of a quick-and-dirty solution to get me unblocked in my particular use case that didn't require re-doing any of the internals of the package. That said, I think the tag approach is still useful in the short term (albleit with a better name, maybe gomega/format:"omit"?), as well as a more general use case for hiding large, obstructive fields that the user knows they aren't interested in having in output.

@onsi
Copy link
Owner

onsi commented Mar 11, 2025

the more i think about it the less confident i am about an omit tag that controls output without also controlling equality. While I wholeheartedly agree about the irritating reality of noisy output the reality is that reflect.DeepEqual() which asserts equality even on those noisy bits will fail to match the noisy bits don't match. By hiding the noise we inadvertently make it harder to debug that sort of failure.

Someday™ building out Gomega's own equality library would be a good investment as it would enable this sort of usecase (so many times i've wanted to just suppress checking for equality on "uninteresting" fields like timestamps or coordinates). But that's too much scope for this, imo.

So... I think an approach that does the cycle detection, scoped to each invocation of format.Object() is probably the best approach. Would you be up for giving it a try?

@meln5674
Copy link
Author

Yeah, I can take a look at that, but it probably won't be right away.

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 a pull request may close this issue.

3 participants