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

api: add tests that ensure types and reasons are registered correctly #122

Merged

Conversation

joelanford
Copy link
Member

We currently export constants for the universe of types and reasons that the operator controller uses to set conditions. And we append them to a list that is used in reconciler tests to ensure that Reconcile sets each condition type during any given call. See here.

However there's still a small gap. It's possible to add a new type/reason without appending it into the respective operatorutil list, which means the assumptions we make (i.e. that the operatorutil lists are exhaustive) are not enforced. And that means our invariant test for "all types must be set at the end of a reconcile call" might produce false negatives.

This PR closes that gap by adding a test that ensures that:

  1. For every constant that begins with Type or Reason, its value exists in the respective operatorutil slice.
  2. For every value in the operatorutil slices, there exists a respective Type or Reason prefixed constant.

})
})

func parseConstants(prefix string) ([]string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice! could be cool to add a couple of comments for those unfamiliar with the the ast package

Copy link
Contributor

Choose a reason for hiding this comment

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

👨‍🍳 💋

Copy link
Contributor

@perdasilva perdasilva left a comment

Choose a reason for hiding this comment

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

lgtm - just one small non-blocking suggestion

@perdasilva
Copy link
Contributor

@joelanford could you please rebase?

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
@joelanford
Copy link
Member Author

Done! Thanks for the review @perdasilva!

@perdasilva perdasilva merged commit 07ae744 into operator-framework:main Mar 1, 2023
perdasilva pushed a commit to perdasilva/operator-controller that referenced this pull request Mar 3, 2023
…operator-framework#122)

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
Signed-off-by: perdasilva <perdasilva@redhat.com>
perdasilva pushed a commit to perdasilva/operator-controller that referenced this pull request Mar 3, 2023
…operator-framework#122)

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
Signed-off-by: perdasilva <perdasilva@redhat.com>
@joelanford joelanford deleted the test-register-types-reasons branch August 29, 2023 12:12
@tmshort tmshort mentioned this pull request Sep 7, 2023
4 tasks
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