Skip to content

Support client.Apply() test expectations#645

Merged
scothis merged 2 commits intoreconcilerio:mainfrom
scothis:client-apply
Sep 14, 2025
Merged

Support client.Apply() test expectations#645
scothis merged 2 commits intoreconcilerio:mainfrom
scothis:client-apply

Conversation

@scothis
Copy link
Member

@scothis scothis commented Sep 3, 2025

Controller runtime added Apply as a client operation, which is sugar around a patch operation. Testing with patch is a bit painful today as the test needs to provide the json bytes of the patch. Diffs in the expected and actual bytes are very annoying to debug. The Apply operation uses a runtime.ApplyConfiguration object which can hold semantic structure and should make failing tests easier to debug.

The ExpectConfig object now has an ExpectApplies slice that is compared to the Apply operation recorded on the fake client for the test. Likewise, an "apply" action is reacted during client requests. The assertion compares the ApplyConfiguration object as a json map.

Controller runtime added `Apply` as a client operation, which is sugar
around a patch operation. Testing with patch is a bit painful today as
the test needs to provide the json bytes of the patch. Diffs in the
expected and actual bytes are very annoying to debug. The Apply
operation uses a runtime.ApplyConfiguration object which can hold
semantic structure and should make failing tests easier to debug.

The ExpectConfig object now has an `ExpectApplies` slice that is
compared to the Apply operation recorded on the fake client for the
test. Likewise, an "apply" action is reacted during client requests. The
assertion compares the ApplyConfiguration object as a json map.

Signed-off-by: Scott Andrews <scott@andrews.me>
}

func (*differ) ApplyRef(expected, actual ApplyRef) string {
return cmp.Diff(expected, actual, NormalizeApplyConfiguration)
Copy link
Member Author

Choose a reason for hiding this comment

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

normalizing the ApplyConfiguration as json is technically correct, but I wonder if the usability cost is too high as all other type info on the object being compared is lost.

A user can define a custom differ to change the behavior, but I'm wondering if the default should be to not apply the NormalizeApplyConfiguration transformer.

@codecov
Copy link

codecov bot commented Sep 3, 2025

Codecov Report

❌ Patch coverage is 77.02703% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.34%. Comparing base (15d1a6b) to head (263aa03).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
testing/config.go 80.55% 4 Missing and 3 partials ⚠️
testing/actions.go 84.00% 2 Missing and 2 partials ⚠️
testing/client.go 62.50% 2 Missing and 1 partial ⚠️
testing/reconciler.go 0.00% 1 Missing ⚠️
testing/subreconciler.go 0.00% 1 Missing ⚠️
testing/webhook.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #645      +/-   ##
==========================================
+ Coverage   56.96%   57.34%   +0.38%     
==========================================
  Files          37       38       +1     
  Lines        4431     4504      +73     
==========================================
+ Hits         2524     2583      +59     
- Misses       1799     1807       +8     
- Partials      108      114       +6     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

type DeleteAction = clientgotesting.DeleteAction
type DeleteCollectionAction = clientgotesting.DeleteCollectionAction

type ApplyAction interface {
Copy link
Member Author

Choose a reason for hiding this comment

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

Defining a new action locally because "apply" isn't an api action in client-go.

Copy link
Contributor

@mamachanko mamachanko left a comment

Choose a reason for hiding this comment

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

lgtm

Signed-off-by: Scott Andrews <scott@andrews.me>
@scothis scothis marked this pull request as ready for review September 14, 2025 18:15
@scothis scothis merged commit d578435 into reconcilerio:main Sep 14, 2025
4 checks passed
@scothis scothis deleted the client-apply branch September 14, 2025 18:17
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