Repository navigation
test(estimation): move fixtures into testdata - #17241
Conversation
Move the five inline workflow YAML fixtures from estimator_factory_test.go into testdata/estimator_factory/. Workflow-typed fixtures are loaded via the wfv1.MustUnmarshalWorkflow "@testdata" convention; the four fixtures that must stay *unstructured.Unstructured (the estimator type-asserts informer index objects) load through a small file-reading helper reusing testutil.MustUnmarshalUnstructured. Fixture content is byte-identical to the literals they replace; no test logic or assertions changed. Part of argoproj#13610 Signed-off-by: augustkevinsanjaya <120900788+incr3d1ble@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: moving estimation test fixtures into testdata. Description check The description covers the required sections, explains the motivation and modifications, records verification results and limitations, addresses documentation, and declares AI use. The unchecked draft… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description covers the required sections, explains the motivation and modifications, records verification results and limitations, addresses documentation, and declares AI use. The unchecked draft status is explained and the issue reference is provided.
-
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17241 +/- ##
==========================================
+ Coverage 46.19% 46.34% +0.15%
==========================================
Files 588 591 +3
Lines 40185 40336 +151
==========================================
+ Hits 18562 18693 +131
- Misses 21621 21641 +20
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Could we just make MustUnmarshalUnstrucutred understand the @ convention instead of this - so that future tests can also use it?
There was a problem hiding this comment.
Done — testutil.MustUnmarshalUnstructured now understands the @ convention by delegating to wfv1.MustUnmarshal (which already implements @file/JSON/strict-YAML), and the package-local mustUnmarshalUnstructuredFile helper is removed. The estimation test loads its four unstructured fixtures via @testdata/... directly: d079bff. Behavior for the other caller (workflow/gccontroller/heap_test.go, inline YAML) is unchanged — still strict-YAML unmarshal. Package tests for both callers pass locally.
Make testutil.MustUnmarshalUnstructured delegate to wfv1.MustUnmarshal so it understands the "@file" convention (and JSON) instead of a package-local helper, letting future tests load unstructured fixtures from testdata directly. The estimation test now loads its four unstructured fixtures via "@testdata/..." paths; no test logic changed. Part of argoproj#13610 Fullscanned with [Cerberus](https://github.com/code-yeongyu/oh-my-open-pentest) Co-authored-by: Cerberus <clio-agent@cerberuslabs.ai> Signed-off-by: Kevin <120900788+incr3d1ble@users.noreply.github.com>
make pre-commit -B— not runnable on this machine (Windows, no Linux toolchain);gofmtis clean on the changed files and the package tests pass locally, see Verificationmake feature-new) — N/A, test-only refactorPart of #13610
Motivation
workflow/controller/estimation/estimator_factory_test.goholds five workflow fixtures as inline string literals, which is the pattern #13610 asks to remove. This takes that file as one slice, following the shape of the merged slices (#17059, #17061, #17062, #17067).Modifications
Moved the five fixtures into
workflow/controller/estimation/testdata/estimator_factory/:wfFailedfailed-baseline.yamlcwft-baseline.yamlcwf-baseline.yamlwftmpl-baseline.yamlarchived-wftmpl-baseline.yamlAll five fixtures are loaded with the
@testdata/...file convention:wfv1.MustUnmarshalWorkflow("@testdata/...");*unstructured.Unstructured(the estimator type-asserts the informer index objects) viatestutil.MustUnmarshalUnstructured, which now understands the same convention by delegating towfv1.MustUnmarshal(per review feedback — the package-local file-reading helper is gone, so future tests can use@paths directly).Fixture content is byte-identical to the literals they replace. No test logic or assertions changed.
Verification
gofmtclean on the changed files (committed content).make pre-commit -Bcould not run locally (Windows); CI will cover lint/codegen.Documentation
Not needed — test-only refactor with no user-facing change.
AI
Cerberus (OhMyOpenCode agent, GLM model) was used for the mechanical fixture extraction and test-file rewrite, directed by @incr3d1ble, who reviewed the result and ran the tests above.
Summary by CodeRabbit