refactor(kclshim): decouple Synthesize from stdin/stdout I/O#575
Open
refactor(kclshim): decouple Synthesize from stdin/stdout I/O#575
Conversation
Accept structured input parameter and return []client.Object instead of reading from stdin and writing to stdout. Add comprehensive tests with fixture-based KCL synthesizer covering deployment and service account assertions, and error handling for bad synthesizer input.
wonderyl
reviewed
Mar 5, 2026
| return nil, fmt.Errorf("error unmarshaling output to ResourceList: %w", err) | ||
| } | ||
|
|
||
| var objects []client.Object |
Contributor
There was a problem hiding this comment.
I wonder why you don't return krmv1.ResourceList directly?
Author
There was a problem hiding this comment.
I was doing that previously, but then I found function.Main expects SynthFunc to return ([]client.Object, error), so I added this one more step to convert krmv1.ResourceList to []client.Object
wonderyl
reviewed
Mar 5, 2026
| s = strings.ReplaceAll(s, whitespace, "") | ||
| } | ||
| return s | ||
| t.Run("successful synthesis", func(t *testing.T) { |
Contributor
There was a problem hiding this comment.
It should use https://go.dev/wiki/TableDrivenTests instead.
Author
There was a problem hiding this comment.
Thank you very much, Lei! I will update them to use table-driven tests.
Author
There was a problem hiding this comment.
Hi, Lei! When updating the test, I have 2 questions about when to use table-driven tests:
- Is it correct that we should use table-driven tests when testing the same logic across different scenarios (e.g., varying inputs, expected outputs, and error conditions)?
- As we can no longer compare the output as a single string, but we do need to validate individual fields (e.g., spec.containers[0].image , spec.containers[0].name ). For cases like this where there are only 2 fields to check, if I understood Table-Driven tests correctly, I felt the overhead of defining struct & writing loop is a bit heavier than just writing inline assertions? May I know would you recommend sticking with table-driven tests for these small cases, or is it better to use inline assertions when the number of items is very small?
Thank you very much, Lei!
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.
Accept structured input parameter and return []client.Object instead of reading from stdin and writing to stdout. Add comprehensive tests with fixture-based KCL synthesizer covering deployment and service account assertions, and error handling for bad synthesizer input.