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

Consider using ginkgo.BeComparableTo instead of ginkgo.Equal #8095

Closed
sbueringer opened this issue Feb 13, 2023 · 10 comments · Fixed by #9015
Closed

Consider using ginkgo.BeComparableTo instead of ginkgo.Equal #8095

sbueringer opened this issue Feb 13, 2023 · 10 comments · Fixed by #9015
Assignees
Labels
area/testing Issues or PRs related to testing help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@sbueringer
Copy link
Member

sbueringer commented Feb 13, 2023

A few days ago I found myself very frustrated (again) with the fact that the widely used Go test assertion library, gomega, has an Equal() matcher that, when the equality assertion fails, emits the entire contents of both structs being compared. This makes finding which specific field was different a chore, especially when working with very large structs. I started looking at a way to improve this, and I found that there is a better option! Gomega added a BeComparableTo() matcher earlier this year that uses the gocmp package and it outputs a diff of exactly which fields are different instead of the entire object! This is a huge time saver, so I thought I would share in case anyone else runs into something similar.
https://kubernetes.slack.com/archives/C8TSNPY4T/p1670011256246809

Sounds like a great improvement to me. Would be good if something can try it out and potentially open a PR if it works as expected in our case

Kudos @dthorsen for bringing this up

/kind cleanup
/area testing

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. area/testing Issues or PRs related to testing needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 13, 2023
@sbueringer
Copy link
Member Author

sbueringer commented Feb 13, 2023

cc @killianmuldoon @schrej (fyi, just in case it's interesting for you because of somewhat related work on komega in CR)

@schrej
Copy link
Member

schrej commented Feb 13, 2023

Thanks! komega.EqualObject() currently outputs a list of non-matching paths (which is useful when wanting to exclude them, but being too lazy to write them yourself), but shows both objects separately. I'll see if I can add a diff output similar to BeComparableTo().

@fabriziopandini
Copy link
Member

I understand go-cmp does much more than plain equality, but plain equality is what we need in most cases and using BeCompatableTo is less readable than Equal when checking equality

What about proposing to add a DeepEqual alias in gomega calling BeCompatableTo without options?

@dthorsen
Copy link
Contributor

The naming of BeComparableTo is unfortunate, but what it is really doing is calling go-cmp's Equal(). So we are actually testing deep equality.

The benefit here is that when comparing structs (especially large ones), the output of gomega.BeComparableTo is infinitely more readable than that of gomega.Equal. This is because it outputs only the different lines and a little context around them.

When not comparing large structs and simply comparing small values like most strings, int, bool, etc gomega.Equal is fine.

@sbueringer
Copy link
Member Author

sbueringer commented Feb 24, 2023

As someone who regularly has to compare large struct dumps by gomega manually I would really appreciate it if we find a way to use the functionality that BeComparableTo provides

@fabriziopandini
Copy link
Member

The naming of BeComparableTo is unfortunate, but what it is really doing is calling go-cmp's Equal(). So we are actually testing deep equality.

Given that we agree on this, +1 to propose a change/an alias into gomega

@fabriziopandini
Copy link
Member

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 21, 2023
@fabriziopandini
Copy link
Member

/help

@k8s-ci-robot
Copy link
Contributor

@fabriziopandini:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

  • Why are we solving this issue?
  • To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
  • Does this issue have zero to low barrier of entry?
  • How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Mar 21, 2023
@Dhairya-Arora01
Copy link
Contributor

/assign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing Issues or PRs related to testing help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants