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

chore(*): test resource builder #5123

Merged

Conversation

jakubdyszkiewicz
Copy link
Contributor

@jakubdyszkiewicz jakubdyszkiewicz commented Oct 7, 2022

This is the first attempt to implement resource builders.

Why we need test builders:

  • It simplifies test code. The test case often uses resources, but we don't really care about the details of those resources. It's not what the test case is about.
  • Proto structures are pretty verbose
  • It shortens the code. We can write our own WithX functions that sets multiple stuff on the proto object
  • Reasonable defaults. If we try to use default samples and something fundamental changes, it's much easier to execute this change if we only need to change the builder impl.
  • It's safer. Builder checks the validity of the object when .Build() is called, which means that we always use valid objects.

Rules

  • builders contain the logic of the builder itself. Just calling builders.Dataplane().Build() does not mean that we will get a valid Dataplane object.
  • samples contain sample valid resources that can be used across the project. The idea is to reuse sample objects, so once we get familiar with basic samples, we don't have to rebuild the same objects for every test.
  • builders have With methods. If With is a list, then it replaces the list. For example WithInboundOfTags replaces all inbounds with inbounds of specified tags. AddInboundOfTags however adds an element to the list. Fields that are not collections always have With only.
  • Introduce a new sample only if it's repeated enough times. Let's not have 100 samples for every single policy. I expect that anyone familiar with the codebase will be able to remember all the samples for at least basic resources.
  • It's totally ok to panic in Builder functions. They are used ONLY for tests.

Question Marks

  • Is Builder#Create() convenient or is it an overkill?

Ideas

  • Builder#YAML() can build and produce YAML of the resource. Also #JSON(), #KubeYAML()
  • Tell me your ideas!

xref #4133

Checklist prior to review

  • Link to docs PR or issue --
  • Link to UI issue or PR --
  • Is the issue worked on linked? --
  • The PR does not hardcode values that might break projects that depend on kuma (e.g. "kumahq" as a image registry) --
  • The PR will work for both Linux and Windows, system specific functions like syscall.Mkfifo have equivalent implementation on the other OS --
  • Unit Tests --
  • E2E Tests --
  • Manual Universal Tests --
  • Manual Kubernetes Tests --
  • Do you need to update UPGRADE.md? --
  • Does it need to be backported according to the backporting policy? --
  • Do you need to explicitly set a > Changelog: entry here or add a ci/ label to run fewer/more tests?

Changelog: chore(test): added resource builder

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
@jakubdyszkiewicz jakubdyszkiewicz requested a review from a team as a code owner October 7, 2022 15:43
Copy link
Contributor

@michaelbeaumont michaelbeaumont left a comment

Choose a reason for hiding this comment

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

LGTM as is, just some thoughts

…ilders

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
@jakubdyszkiewicz jakubdyszkiewicz enabled auto-merge (squash) October 20, 2022 13:19
@jakubdyszkiewicz jakubdyszkiewicz merged commit 1bf643d into kumahq:master Oct 20, 2022
@jakubdyszkiewicz jakubdyszkiewicz deleted the chore/resource-builders branch October 20, 2022 14:10
@lahabana lahabana mentioned this pull request Dec 13, 2022
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