fix: decompose high-CRAP functions to reduce complexity#478
fix: decompose high-CRAP functions to reduce complexity#478gvauter merged 4 commits intocomplytime:mainfrom
Conversation
Reduce cyclomatic complexity of the run methods for scanOptions, generateOptions, getOptions, and providersOptions by extracting focused helper functions. This brings their CRAP scores below the CI threshold of 30. - scanOptions.run: complexity 18 → 4 - generateOptions.run: complexity 18 → 3 - getOptions.run: complexity 8 → 2 - providersOptions.run: complexity 8 → 4 Add cli_test.go with unit tests covering early-exit paths and helper functions (filterTargetsForPolicy, extractReqToControlMap). Signed-off-by: George Vauter <gvauter@redhat.com> Made-with: Cursor
Extract helper functions from PluginServer.Generate and PluginServer.Scan to reduce cyclomatic complexity below the CRAP threshold of 30. - Generate: complexity 9 → 2 - Scan: complexity 18 → 4 Add tests for parseARFFile, buildAssessmentsFromARF, findOVALCheckContentRef, and other extracted helpers. Signed-off-by: George Vauter <gvauter@redhat.com> Made-with: Cursor
Add mockPlugin field to LoadedPlugin with GetClient() accessor to enable test injection without requiring real plugin binaries. Add NewMockLoadedPlugin helper and comprehensive tests for Manager RouteGenerate and RouteScan methods covering specific plugin, broadcast, unknown evaluator, and error scenarios. - RouteGenerate CRAP: 8.2 (below 30) - RouteScan CRAP: 6.2 (below 30) Signed-off-by: George Vauter <gvauter@redhat.com> Made-with: Cursor Signed-off-by: George Vauter <gvauter@redhat.com> Made-with: Cursor
gvauter
force-pushed
the
fix/crap-export-coverage
branch
from
13c0c37 to
74cfca7
Compare
marcusburghardt
left a comment
marcusburghardt
left a comment
There was a problem hiding this comment.
AI-assisted review — two non-blocking nits flagged inline for future consideration.
| Client *Client | ||
| Info PluginInfo | ||
| Client *Client | ||
| mockPlugin Plugin // injected during tests; nil in production |
There was a problem hiding this comment.
Nit (non-blocking): This adds a test-only field to a production struct. An alternative would be to define a PluginClient interface at the Manager level and inject mocks through that, keeping LoadedPlugin free of test-only fields. This would better align with Principle II (Simplicity & Isolation) by separating test and production concerns.
Not in scope for this PR — just flagging for a potential follow-up.
| "slices" | ||
| "time" | ||
|
|
||
| "github.com/gemaraproj/go-gemara" |
There was a problem hiding this comment.
Nit (non-blocking): This introduces a direct import of go-gemara in scan.go for the gemara.EvaluationLog type used in the extracted report helpers. Previously, this type was only accessed transitively through the output package. Consider whether the report-writing helpers (writePrettyReport, writeSARIFReport, writeOSCALReport) could accept a narrower interface or remain closer to the output package to avoid spreading the direct dependency.
Not in scope for this PR — just a coupling observation.
There was a problem hiding this comment.
Thanks @marcusburghardt - I've added an additional commit to address these comments.
…ports Replace the test-only mockPlugin field in LoadedPlugin with an interface-typed Client field (Plugin instead of *Client). This removes the if-branch from GetClient(), eliminating the CRAP regression (2.148 -> 1.0) and keeping test concerns out of the production struct. Remove the direct go-gemara import from scan.go by refactoring the write helpers (writePrettyReport, writeSARIFReport, writeOSCALReport) to accept *output.Evaluator and call GemaraLog() internally, keeping the gemara dependency encapsulated in the output package. Signed-off-by: George Vauter <gvauter@redhat.com> Made-with: Cursor Signed-off-by: George Vauter <gvauter@redhat.com>
marcusburghardt
left a comment
marcusburghardt
left a comment
There was a problem hiding this comment.
LGTM. We can update the baseline in a separate PR. Thanks for addressing this @gvauter
| "github.com/complytime/complyctl/internal/policy" | ||
| ) | ||
|
|
||
| func chdirTemp(t *testing.T) { |
There was a problem hiding this comment.
Nit: I think testing.T has a function for this t.Chdir()
There was a problem hiding this comment.
I will fix in an upcoming PR. TY!
3 participants
Summary
Decomposes 10 functions on
mainthat exceed the CRAP threshold of 30, bringing all of them well below the limit. The primary strategy is aggressive extraction of focused helper functions to reduce cyclomatic complexity to ≤ 4 (which guarantees CRAP < 30 regardless of coverage), supplemented by targeted unit tests.(*scanOptions).run(*generateOptions).run(*getOptions).run(*providersOptions).run(*initOptions).run(*listOptions).run(*Manager).RouteGenerate(*Manager).RouteScan(*PluginServer).Generate(*PluginServer).ScanOverall CRAPload: 84 → 56.
Related Issues
gaze crapCI checkReview Hints
Best reviewed commit-by-commit in order:
8204be5— CLI decomposition (scan.go,generate.go,get.go,providers.go, newcli_test.go): This is the largest commit. Eachrunmethod now delegates to small, single-purpose helpers. No behavioral changes.0f7242e— OpenSCAP plugin decomposition (server.go,server_test.go): Same pattern applied to the OpenSCAP plugin module. Separate Go module, so fully isolated.13c0c37— Plugin manager test injection (manager.go,export_test.go,manager_test.go): Adds amockPluginfield toLoadedPluginso tests can inject a mockPluginwithout real binaries. Adds table-driven tests forRouteGenerateandRouteScan.All changes are pure refactoring — no behavioral changes, no new features, no API changes.
To verify locally: