Skip to content

fix: reuse parseDocs helper to avoid YAML split/unmarshal duplication - #204

Open
andrewwhitecdw wants to merge 1 commit into
NVIDIA:mainfrom
andrewwhitecdw:codequality/sriov-render-test-reuse-parsedocs-helper-to-avoid-yaml
Open

fix: reuse parseDocs helper to avoid YAML split/unmarshal duplication#204
andrewwhitecdw wants to merge 1 commit into
NVIDIA:mainfrom
andrewwhitecdw:codequality/sriov-render-test-reuse-parsedocs-helper-to-avoid-yaml

Conversation

@andrewwhitecdw

@andrewwhitecdw andrewwhitecdw commented Aug 17, 2026

Copy link
Copy Markdown

This PR addresses the following issue in pkg/networkoperatorplugin/sriov_render_test.go: reuse parseDocs helper to avoid YAML split/unmarshal duplication.

Changes

  • pkg/networkoperatorplugin/sriov_render_test.go: consolidate strict multi-document YAML parsing in the existing parseDocs helper while preserving the failing document index in error messages.

Details

Greptile noted that the refactor removed the document index from parseDocs diagnostics, making it harder to locate the failing document in malformed multi-document YAML. The index is restored while keeping the helper reuse.

--- a/pkg/networkoperatorplugin/sriov_render_test.go
+++ b/pkg/networkoperatorplugin/sriov_render_test.go
@@ -112,13 +112,13 @@ func metaString(t *testing.T, doc map[string]any, key string) string {
 func parseDocs(t *testing.T, name, content string) []map[string]any {
 	t.Helper()
 	var docs []map[string]any
-	for _, raw := range splitYAMLDocuments(content) {
+	for i, raw := range splitYAMLDocuments(content) {
 		if strings.TrimSpace(raw) == "" {
 			continue
 		}
 		var doc map[string]any
 		err := yaml.UnmarshalStrict([]byte(raw), &doc)
-		require.NoErrorf(t, err, "%s contains a document that is not valid YAML (likely a glued separator):\n%s", name, raw)
+		require.NoErrorf(t, err, "file %s doc %d is not valid YAML (likely a glued separator):\n%s", name, i, raw)
 		docs = append(docs, doc)
 	}
 	return docs

Tests

go test ./pkg/networkoperatorplugin -count=1 -v

Result: PASS

Contributor guidelines

Per this repo's CONTRIBUTING.md:

  • All commits are signed off (Signed-off-by trailer, DCO).

@greptile-apps

greptile-apps Bot commented Aug 17, 2026

Copy link
Copy Markdown

Greptile Summary

The test now reuses the shared YAML document parser while preserving strict validation and document-index diagnostics.

  • Replaces duplicate split/unmarshal logic with parseDocs.
  • Uses the parsed document count for manifest and multirail assertions.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
pkg/networkoperatorplugin/sriov_render_test.go Consolidates multi-document YAML parsing through parseDocs; the previous diagnostic issue is resolved because failures retain the document index.

Reviews (2): Last reviewed commit: "fix: reuse parseDocs helper to avoid YAM..." | Re-trigger Greptile

Comment on lines +115 to +121
for _, raw := range splitYAMLDocuments(content) {
if strings.TrimSpace(raw) == "" {
continue
}
var doc map[string]any
err := yaml.UnmarshalStrict([]byte(raw), &doc)
require.NoErrorf(t, err, "file %s doc %d is not valid YAML:\n%s", name, i, raw)
require.NoErrorf(t, err, "%s contains a document that is not valid YAML (likely a glued separator):\n%s", name, raw)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Document index removed from diagnostics

When strict unmarshalling fails for a multi-document YAML file, parseDocs now reports the filename and raw YAML but not the document index, making similar multirail documents unnecessarily difficult to locate.

Suggested change
for _, raw := range splitYAMLDocuments(content) {
if strings.TrimSpace(raw) == "" {
continue
}
var doc map[string]any
err := yaml.UnmarshalStrict([]byte(raw), &doc)
require.NoErrorf(t, err, "file %s doc %d is not valid YAML:\n%s", name, i, raw)
require.NoErrorf(t, err, "%s contains a document that is not valid YAML (likely a glued separator):\n%s", name, raw)
for i, raw := range splitYAMLDocuments(content) {
if strings.TrimSpace(raw) == "" {
continue
}
var doc map[string]any
err := yaml.UnmarshalStrict([]byte(raw), &doc)
require.NoErrorf(t, err, "file %s doc %d is not valid YAML (likely a glued separator):\n%s", name, i, raw)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Signed-off-by: andrewwhitecdw <andrewwhitecdw@users.noreply.github.com>
@andrewwhitecdw
andrewwhitecdw force-pushed the codequality/sriov-render-test-reuse-parsedocs-helper-to-avoid-yaml branch from d9a9a94 to 2918013 Compare August 17, 2026 21:58
@andrewwhitecdw

Copy link
Copy Markdown
Author

@greptile-apps Good catch — I restored the document index in the parseDocs error message so strict-unmarshal failures report which document is invalid. The helper reuse is preserved and all pkg/networkoperatorplugin tests pass. Amended into the existing commit (2918013).

@greptile-apps

greptile-apps Bot commented Aug 17, 2026

Copy link
Copy Markdown

Confirmed — parseDocs at line 108 now includes i in the error format:

require.NoErrorf(t, err, "file %s doc %d is not valid YAML (likely a glued separator):\n%s", name, i, raw)

The document index is restored, the helper reuse is intact, and the error message is actually more precise than the original (which lacked the index entirely). LGTM.

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.

1 participant