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

feat(test): use builder to build test objects. #955

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

jvanz
Copy link
Member

@jvanz jvanz commented Jan 7, 2025

Description

Creates builders to allow dynamic object building during tests.

Part of #824

@kubewarden/kubewarden-developers this is one of a series of PR open to refactor the webhooks tests. This is a initial change to use builder pattern to build (hehe) test objects. This is not a 100% convertion. But it's a start to see if you are all fine with the current changes before the next one following the same idea.

@jvanz jvanz self-assigned this Jan 7, 2025
@jvanz jvanz requested a review from a team as a code owner January 7, 2025 17:50
@jvanz jvanz added the area/qa label Jan 7, 2025
@jvanz jvanz force-pushed the webhook-tests-refactor branch from 3963cac to c566103 Compare January 7, 2025 17:53
Copy link

codecov bot commented Jan 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.35%. Comparing base (7110ea8) to head (f17d163).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #955      +/-   ##
==========================================
- Coverage   67.51%   67.35%   -0.16%     
==========================================
  Files          30       30              
  Lines        3128     3128              
==========================================
- Hits         2112     2107       -5     
- Misses        848      852       +4     
- Partials      168      169       +1     
Flag Coverage Δ
integration-tests 56.59% <ø> (-0.17%) ⬇️
unit-tests 40.41% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fabriziosestito
Copy link
Contributor

fabriziosestito commented Jan 8, 2025

@jvanz this looks great, few comments below:

  • We could name the builders "factory"; this naming is commonly used in tests to describe generated test data (as opposed to fixtures). See for example https://github.com/beam-community/ex_machina
  • Reference link: We already use a factory for other objects, but we are not applying the builder pattern. It would be good to migrate to it.
  • It would be nice to place these in a separate file (e.g., factories_test.go).

@jvanz jvanz force-pushed the webhook-tests-refactor branch 2 times, most recently from 7b57b93 to 4afac1c Compare January 8, 2025 19:00
@jvanz
Copy link
Member Author

jvanz commented Jan 8, 2025

@kubewarden/kubewarden-developers I've rewrite all the factories function from the internal/controller/utils_test.go. Let me know what do you think.

right after pushing the code I had another idea. Which is using a factory method with variable argument to define the objects configuration. For example:

admissionPolicyFactory(withName("foo"), withPolicyServer("default"))
// or
admissionPolicyFactory(withName("foo"), withPolicyServer("default"), withMutating(true))

Something similar to what other go projects do to customize configuration. But I'm not sure what do you prefer. I do have have any string preference on either options. I just want to share all the paths with you.


Another thing, I'm not migrating the factories from the api/policies/v1/policy_utils_test.go file because I'm planning to do the validation of the webhook in the integration tests as described in the acceptance criteria of the issue #824

@jvanz jvanz requested a review from fabriziosestito January 8, 2025 19:12
internal/controller/factories_test.go Outdated Show resolved Hide resolved
internal/controller/factories_test.go Outdated Show resolved Hide resolved
internal/controller/factories_test.go Outdated Show resolved Hide resolved
internal/controller/factories_test.go Outdated Show resolved Hide resolved
Creates builders to allow dynamic object building during tests.

Signed-off-by: José Guilherme Vanz <[email protected]>
@jvanz jvanz force-pushed the webhook-tests-refactor branch from 4afac1c to f17d163 Compare January 9, 2025 11:48
@jvanz jvanz requested a review from flavio January 9, 2025 11:49
@fabriziosestito
Copy link
Contributor

admissionPolicyFactory(withName("foo"), withPolicyServer("default"))
// or
admissionPolicyFactory(withName("foo"), withPolicyServer("default"), withMutating(true))

I think that what we have now is clearer than functional options

Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes

@jvanz jvanz merged commit 73de594 into kubewarden:main Jan 10, 2025
9 checks passed
@jvanz jvanz deleted the webhook-tests-refactor branch January 10, 2025 11:20
@jvanz jvanz linked an issue Jan 10, 2025 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Refactor policy validation on create and update, including tests
4 participants