feat: API roundtrip testing library #636
Merged
erhancagirici merged 11 commits intocrossplane:mainfrom May 7, 2026
Merged
Conversation
erhancagirici
force-pushed
the
roundtrip-test-library
branch
from
7030a72 to
97b875e
Compare
erhancagirici
force-pushed
the
roundtrip-test-library
branch
from
97b875e to
f9e11b6
Compare
erhancagirici
changed the title
sergenyalcin
reviewed
May 6, 2026
Member
sergenyalcin
left a comment
sergenyalcin
left a comment
There was a problem hiding this comment.
Thank you very much, @erhancagirici, for this library. This is very helpful. It will increase the confidence level in the generated APIs. I've left a few comments to discuss and consider.
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds a RoundTripTest framework, randfill-based fuzzers, metadata-normalizing cmp helpers (normalizeMeta, EquateEmptyAndSingleZeroSlice, EquateNilAndZeroValuePtr), comprehensive tests, and documentation for serialization and conversion round-trips. ChangesRound-Trip API Testing Framework
Sequence DiagramssequenceDiagram
actor TestSuite as Test Suite
participant RRT as RoundTripTest
participant Scheme as Scheme
participant Fuzzer as Fuzzer
participant Codec as CodecFactory
participant Serializer as Serializer
participant Comparer as Comparator
TestSuite->>RRT: NewRoundTripTest(provider, scheme, opts...)
RRT->>RRT: setupTestInfrastructure()
TestSuite->>RRT: TestSerializationRoundtrip()
RRT->>Fuzzer: getFuzzer(config)
Fuzzer-->>RRT: randfill.Filler
RRT->>Codec: Request encoder/decoder
Codec-->>RRT: Encoder/Decoder
RRT->>Serializer: Encode object -> bytes
Serializer-->>RRT: bytes
RRT->>Serializer: Decode bytes -> object
Serializer-->>RRT: object
RRT->>Comparer: cmpK8sObjects(original, deserialized)
Comparer-->>RRT: diff/result
RRT-->>TestSuite: Pass/Fail
sequenceDiagram
actor TestSuite as Test Suite
participant RRT as RoundTripTest
participant Scheme as Scheme
participant Spoke as SpokeVersion
participant Hub as HubVersion
participant Fuzzer as Fuzzer
participant Comparer as Comparator
TestSuite->>RRT: TestConversionRoundtrip()
RRT->>Scheme: Discover group/kind versions
Scheme-->>RRT: hub & spoke mappings
RRT->>Fuzzer: Create fuzzed spoke object
Fuzzer-->>Spoke: fuzzed spoke
Spoke->>Hub: ConvertTo(hub)
Hub-->>Spoke: hub object
Hub->>Spoke: ConvertFrom(hub)
Spoke-->>RRT: final spoke
RRT->>Comparer: cmpK8sObjects(originalSpoke, finalSpoke)
Comparer-->>RRT: diff/result
RRT-->>TestSuite: Pass/Fail
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Thanks — would you like me to highlight specific areas for reviewers to focus on (determinism, fuzzer choices, comparison tolerances, or filtering logic)? 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/api-roundtrip-testing.md`:
- Around line 132-136: The documented implicit fuzzer defaults are out of sync
with the code: update the prose for WithFuzzerConfig to match the implementation
(NilChance≈0.2, NumElements 0–1, iterations 5) instead of the current
"NumElements 0–3, 10 iterations", and make the same correction in the later
occurrence that mirrors lines 160–162 so both places reflect the actual defaults
used by the fuzzer.
- Around line 227-231: The example declares MyWorkload incorrectly; change the
type declaration to a proper Go struct by replacing "type MyWorkload {" with
"type MyWorkload struct {" and keep the fields Name string, State State, and
PendingJobCount int as-is (referencing MyWorkload, State, PendingJobCount) so
the example compiles when copy-pasted.
In `@pkg/apitesting/roundtrip/roundtrip.go`:
- Around line 486-505: The function nonRoundTrippableTypes currently returns nil
when no include/exclude filters are set which causes callers (e.g.,
TestSerializationRoundtrip) to treat that as "no skips" and exercise kinds this
package intends to ignore; change nonRoundTrippableTypes on RoundTripTest to
always build and return the skip map (i.e., compute allowed :=
rt.groupsToKindFromScheme() and populate result for every gvk from
rt.scheme.AllKnownTypes() except runtime.APIVersionInternal where the gvk.Group
is not in allowed or the GroupKind is not allowed) instead of returning nil when
hasFilter is false so the default exclusions remain active. Ensure callers that
expect a nil vs empty map still work by returning the constructed map (possibly
empty) rather than nil.
- Around line 122-124: Replace the public use of any by introducing a concrete
FuzzFunc type (e.g., type FuzzFunc interface or func wrapper representing the
randfill-compatible signature) and change the extraFuzzFns field from []any to
[]FuzzFunc; update the public constructor WithExtraFuzzFuncs(fns ...any) to
WithExtraFuzzFuncs(fns ...FuzzFunc) and adapt callers to pass the new FuzzFunc
wrappers, and update the customFuzzFuncs function signature/return type to
return []FuzzFunc so the dynamic registration is encapsulated internally while
keeping the public API typed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d5eb625-7249-480e-adc3-fa9845de02c3
⛔ Files ignored due to path filters (1)
go.modis excluded by none and included by none
📒 Files selected for processing (4)
docs/api-roundtrip-testing.mdpkg/apitesting/roundtrip/cmp.gopkg/apitesting/roundtrip/fuzzer.gopkg/apitesting/roundtrip/roundtrip.go
erhancagirici
force-pushed
the
roundtrip-test-library
branch
2 times, most recently
from
3f5c950 to
6ba19c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
pkg/apitesting/roundtrip/roundtrip_test.go (2)
201-201: ⚡ Quick winSubtest names should not contain underscores — inconsistent with the rest of the suite.
Several
t.Run(...)names use underscores as separators (_append,_true,_false,_appends,_sets_custom), while sibling subtests in the same test functions use pure PascalCase. Please use PascalCase without underscores throughout for consistency.♻️ Proposed rename
- t.Run("FuzzerSkipPatterns_append", func(t *testing.T) { + t.Run("FuzzerSkipPatternsAppend", func(t *testing.T) { - t.Run("FuzzerAllowUnexportedFields_true", func(t *testing.T) { + t.Run("FuzzerAllowUnexportedFieldsTrue", func(t *testing.T) { - t.Run("FuzzerAllowUnexportedFields_false", func(t *testing.T) { + t.Run("FuzzerAllowUnexportedFieldsFalse", func(t *testing.T) { - t.Run("WithFuzzerConfig_appends", func(t *testing.T) { + t.Run("WithFuzzerConfigAppends", func(t *testing.T) { - t.Run("WithComparisonOptions_appends", func(t *testing.T) { + t.Run("WithComparisonOptionsAppends", func(t *testing.T) { - t.Run("WithExtraFuzzFuncs_appends", func(t *testing.T) { + t.Run("WithExtraFuzzFuncsAppends", func(t *testing.T) { - t.Run("WithCodecFactory_sets_custom", func(t *testing.T) { + t.Run("WithCodecFactorySetsCustom", func(t *testing.T) {As per coding guidelines: "PascalCase test names (no underscores)".
Also applies to: 212-212, 220-220, 232-232, 286-286, 295-295, 304-304
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/apitesting/roundtrip/roundtrip_test.go` at line 201, The subtest name "FuzzerSkipPatterns_append" uses underscores and should be PascalCase like the rest of the suite; rename the t.Run call in this file (and the sibling t.Run calls at the other listed locations) to remove underscores and use PascalCase (e.g., FuzzerSkipPatternsAppend) so all subtest names are consistent; update the string passed to t.Run in the affected anonymous subtests (the ones invoking t.Run with names ending in _append, _true, _false, _appends, _sets_custom) to the new PascalCase names while leaving the test body and function identifiers unchanged.
535-549: ⚡ Quick winConsider strengthening the namespaced-filler assertion to check all iterations, not just the first.
The
sawNonEmpty/breakpattern verifies "at least once non-empty" in 50 runs, butobjectMetaNamespaceFuzzer(true)will always produce a non-empty namespace (the"ns"prefix guarantees this). A tighter loop that asserts every run produces a non-empty namespace would catch any regression where the filler silently stops settingNamespacewhile still occasionally producing one.♻️ Proposed fix
- t.Run("namespaced filler produces non-empty Namespace in most runs", func(t *testing.T) { + t.Run("namespaced filler always produces non-empty Namespace", func(t *testing.T) { fillers := rt.getFillers(true) - sawNonEmpty := false for range 50 { var meta metav1.ObjectMeta fillers[0].filler.Fill(&meta) - if meta.Namespace != "" { - sawNonEmpty = true - break + if meta.Namespace == "" { + t.Error("namespaced filler produced an empty Namespace") } } - if !sawNonEmpty { - t.Error("namespaced filler never produced non-empty Namespace in 50 runs") - } })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/apitesting/roundtrip/roundtrip_test.go` around lines 535 - 549, The test currently only requires at least one non-empty Namespace across 50 iterations; change it to assert that every iteration produces a non-empty Namespace by calling rt.getFillers(true), using the first filler’s Fill method (fillers[0].filler.Fill(&meta)) in a loop of 50 runs, and failing the test immediately if any meta.Namespace == "" so the namespaced filler (objectMetaNamespaceFuzzer via getFillers) is verified to always set Namespace.docs/api-roundtrip-testing.md (3)
185-187: 💤 Low valueAdd a blank line before the
---separator following the comparison-options table.Without a blank line, some Markdown parsers (e.g., GitHub's) can interpret
---immediately after a line of text as an underline-style H2 heading. In this position the last table row could theoretically be misread. Adding one blank line makes the horizontal rule unambiguous.♻️ Proposed fix
| `EquateEmptyAndSingleZeroSlice` | Considers empty slices and slices of length 1 with zero-value elements equal | + ---🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/api-roundtrip-testing.md` around lines 185 - 187, The Markdown lacks a blank line before the horizontal rule after the comparison-options table row for `EquateEmptyAndSingleZeroSlice`, which can cause some parsers to treat the `---` as an H2 underline; add a single blank line between the last table row (the `EquateEmptyAndSingleZeroSlice` entry) and the `---` separator so the horizontal rule is unambiguous.
192-200: 💤 Low valueThe
WithExtraFuzzFuncssection duplicates its own description — consider trimming the table.The prose at line 194–195 already explains what
WithExtraFuzzFuncsdoes; the table at lines 198–200 repeats the same text verbatim. Removing the table (or collapsing the intro prose into the table row) would reduce redundancy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/api-roundtrip-testing.md` around lines 192 - 200, The docs duplicate the description for WithExtraFuzzFuncs; remove the redundant table row or collapse the intro into it so the behavior is described once. Locate the header and paragraph mentioning WithExtraFuzzFuncs and either delete the table row for `WithExtraFuzzFuncs(fns...)` or replace the prose with a single concise table entry; ensure the unique identifier `WithExtraFuzzFuncs(fns...)` remains documented exactly once and that its signature and effect (`func(*T, randfill.Continue)` applied globally to every fuzzer configuration) are preserved.
9-9: 💤 Low valueMinor grammar nit: hyphenate "provider-managed".
"provider managed resources" should be "provider-managed resources" — the compound modifier before the noun needs a hyphen.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/api-roundtrip-testing.md` at line 9, Replace the unhyphenated phrase "provider managed resources" with the compound modifier "provider-managed resources" in the docs text (look for the exact string "provider managed resources" in docs/api-roundtrip-testing.md) so the compound adjective before "resources" is correctly hyphenated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/api-roundtrip-testing.md`:
- Around line 234-236: The example in myCustomWorkloadFuzzer calls
c.FillNoCustom() without the required target argument; update the call to pass
the workload pointer (f) so use c.FillNoCustom(f) within the
myCustomWorkloadFuzzer function (randfill.Continue c, *MyWorkload f) to match
the real API and avoid the compile error.
---
Nitpick comments:
In `@docs/api-roundtrip-testing.md`:
- Around line 185-187: The Markdown lacks a blank line before the horizontal
rule after the comparison-options table row for `EquateEmptyAndSingleZeroSlice`,
which can cause some parsers to treat the `---` as an H2 underline; add a single
blank line between the last table row (the `EquateEmptyAndSingleZeroSlice`
entry) and the `---` separator so the horizontal rule is unambiguous.
- Around line 192-200: The docs duplicate the description for
WithExtraFuzzFuncs; remove the redundant table row or collapse the intro into it
so the behavior is described once. Locate the header and paragraph mentioning
WithExtraFuzzFuncs and either delete the table row for
`WithExtraFuzzFuncs(fns...)` or replace the prose with a single concise table
entry; ensure the unique identifier `WithExtraFuzzFuncs(fns...)` remains
documented exactly once and that its signature and effect (`func(*T,
randfill.Continue)` applied globally to every fuzzer configuration) are
preserved.
- Line 9: Replace the unhyphenated phrase "provider managed resources" with the
compound modifier "provider-managed resources" in the docs text (look for the
exact string "provider managed resources" in docs/api-roundtrip-testing.md) so
the compound adjective before "resources" is correctly hyphenated.
In `@pkg/apitesting/roundtrip/roundtrip_test.go`:
- Line 201: The subtest name "FuzzerSkipPatterns_append" uses underscores and
should be PascalCase like the rest of the suite; rename the t.Run call in this
file (and the sibling t.Run calls at the other listed locations) to remove
underscores and use PascalCase (e.g., FuzzerSkipPatternsAppend) so all subtest
names are consistent; update the string passed to t.Run in the affected
anonymous subtests (the ones invoking t.Run with names ending in _append, _true,
_false, _appends, _sets_custom) to the new PascalCase names while leaving the
test body and function identifiers unchanged.
- Around line 535-549: The test currently only requires at least one non-empty
Namespace across 50 iterations; change it to assert that every iteration
produces a non-empty Namespace by calling rt.getFillers(true), using the first
filler’s Fill method (fillers[0].filler.Fill(&meta)) in a loop of 50 runs, and
failing the test immediately if any meta.Namespace == "" so the namespaced
filler (objectMetaNamespaceFuzzer via getFillers) is verified to always set
Namespace.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8857a51f-a53d-486f-87e5-d3357a06a65d
⛔ Files ignored due to path filters (1)
go.modis excluded by none and included by none
📒 Files selected for processing (7)
docs/api-roundtrip-testing.mdpkg/apitesting/roundtrip/cmp.gopkg/apitesting/roundtrip/cmp_test.gopkg/apitesting/roundtrip/fuzzer.gopkg/apitesting/roundtrip/fuzzer_test.gopkg/apitesting/roundtrip/roundtrip.gopkg/apitesting/roundtrip/roundtrip_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/apitesting/roundtrip/cmp_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/apitesting/roundtrip/cmp.go
- pkg/apitesting/roundtrip/roundtrip.go
sergenyalcin
approved these changes
May 7, 2026
Member
sergenyalcin
left a comment
sergenyalcin
left a comment
There was a problem hiding this comment.
Thanks @erhancagirici LGTM!
Signed-off-by: Erhan Cagirici <erhan@upbound.io>
Signed-off-by: Erhan Cagirici <erhan@upbound.io>
- do not override user-supplied codecFactory - fuzzer maxElement default to 1 - use error package for constructing error - run SerializationRoundtrip tests for each fuzzer config Signed-off-by: Erhan Cagirici <erhan@upbound.io>
- remove leftover obsolete info - add warning about singleton lists at fuzzer options - add references for custom comparison opts - add example for custom fuzz function Signed-off-by: Erhan Cagirici <erhan@upbound.io>
Signed-off-by: Erhan Cagirici <erhan@upbound.io>
Signed-off-by: Erhan Cagirici <erhan@upbound.io>
Signed-off-by: Erhan Cagirici <erhan@upbound.io>
Signed-off-by: Erhan Cagirici <erhan@upbound.io>
Signed-off-by: Erhan Cagirici <erhan@upbound.io>
Signed-off-by: Erhan Cagirici <erhan@upbound.io>
Signed-off-by: Erhan Cagirici <erhan@upbound.io>
erhancagirici
force-pushed
the
roundtrip-test-library
branch
from
fecb1d3 to
6b06ca1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of your changes
This PR introduces
pkg/apitesting/roundtrip, a reusable testing utility library, that providers built on upjetcan drop into their test suites to automatically verify the roundtrip correctness of their generated API types.
Similar to the approach in the Kubernetes ecosystem, the library utilizes fuzz-based round-trip testing: generate
randomly filled objects and assert they survive the full encode/decode or conversion cycle unchanged.
This library extracts that common boilerplate into a single, maintained place so providers can opt in with minimal setup.
Two categories of invariants are covered:
spoke → hub → spokeandhub → spoke → hub) with no data loss.Note: The tests focus on the serialization/conversion machinery aspect.
The test data is fuzz-based and does not focus on "semantically valid" API objects of the corresponding external provider API.
How it works
Serialization test
This test utilizes upstream k8s "k8s.io/apimachinery/pkg/api/apitesting/roundtrip" for serialization tests.
Conversion test
conversion.Hub/conversion.Convertibleinterfaces).GroupKinds with one available versiongo-cmpwith any registered comparison options.Wiring at providers
Provider wires the library once in a test file, supplying the upjet provider object and a runtime scheme with types registered:
The test runner discovers all types and API versions registered in the scheme automatically — no per-resource configuration is needed.
Optionally, some test behavior can be customized. See
Customizationsection.Customisation
When provider-specific quirks require it, the library exposes options to tune behavior without modifying shared code:
go-cmpoptions to handle known legitimate asymmetries in conversionsChanges
pkg/apitesting/roundtripwith three files:roundtrip.go— core test runner, options, and parallelism-safe sub-test structurefuzzer.go— Fuzzer helperscmp.go— k8s object comparison helpersdocs/api-roundtrip-testing.md.I have:
make reviewableto ensure this PR is ready for review.Addedbackport release-x.ylabels to auto-backport this PR if necessary.How has this code been tested