test: Refresh stale test fixtures OD-497 - #207
Conversation
Not up to standards ⛔
|
8674f67 to
2713816
Compare
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
This PR successfully refreshes test fixtures for Trivy, Lizard, PMD, and Semgrep while pinning Lizard patterns to reduce external API dependencies. Codacy analysis indicates the changes are up to standards with no coverage regressions.
However, a recurring inconsistency was found in the Lizard configuration files where the rule explanations still reference a threshold of 5, despite the logic being updated to 8. Additionally, the Trivy integration tests remain brittle due to their reliance on a live, daily-updating vulnerability database. There are also reported flakes related to network dependencies during OpenGrep pagination that should be monitored.
About this PR
- Systemic inconsistency: Across updated lizard.yaml files, rule explanations for 'parameter-count-medium' still cite a threshold of 5, while the configuration and descriptions have been updated to 8.
- The 'Manual Testing' section in the PR description is incomplete (currently a TODO). Ensure testing is documented or the section is updated before merging.
Test suggestions
- SARIF unit tests for Lizard use a pinned pattern set to avoid dependency on the live Codacy API.
- Tool configuration discovery for Trivy correctly identifies 'go.sum' as a relevant file.
- PMD ruleset generation supports the PMD 7 schema and category-based rule references.
- Trivy integration tests pass with the latest vulnerability database entries.
Low confidence findings
- Note that Trivy SARIF fixtures and OpenGrep pagination remain dependent on live databases and network connections, which may cause brittle tests or intermittent CI failures.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| Lizard_parameter-count-medium: | ||
| category: Complexity | ||
| description: This rule checks the number of parameters passed to a function and raises an issue if it exceeds a medium threshold, which by default is 8 parameters. | ||
| description: Reports a Medium issue when a function's parameter count reaches the configured threshold, which defaults to 8. A signature this wide is hard to call correctly and usually signals that the function has taken on too many responsibilities. |
There was a problem hiding this comment.
⚪ LOW RISK
The rule description specifies a default threshold of 8, but the explanation (visible in the surrounding context) still incorrectly references 5. Update the explanation to match the threshold and adopt the 'exceeded' terminology used in the updated Lizard_ccn-minor rule.
| warningCode = 0 | ||
|
|
||
| rules = ["blank-imports", "context-as-argument", "context-keys-type", "dot-imports", "empty-block", "error-naming", "error-return", "error-strings", "errorf", "exported", "increment-decrement", "indent-error-flow", "package-comments", "range", "receiver-naming", "redefines-builtin-id", "superfluous-else", "time-naming", "unexported-return", "unreachable-code", "unused-parameter", "var-declaration", "var-naming"] | ||
| rules = ["blank-imports", "context-as-argument", "context-keys-type", "dot-imports", "empty-block", "errorf", "error-naming", "error-return", "error-strings", "exported", "increment-decrement", "indent-error-flow", "package-comments", "range", "receiver-naming", "redefines-builtin-id", "superfluous-else", "time-naming", "unexported-return", "unreachable-code", "unused-parameter", "var-declaration", "var-naming"] |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: The errorf rule is out of alphabetical order. It should follow the error- prefixed rules (error-naming, error-return, error-strings) to maintain consistent sorting.
2713816 to
77e2459
Compare
1 participant
Changes
tools/lizard/test/lizard.yaml(new): pins the Lizard pattern set for the SARIF unit test. Without it the test fetched default patterns from the live API, so any upstream pattern change broke it.expected.sarifis unchanged.integration-tests/{init-without-token,config-discover}/expected/tools-configs/*: regenerated against the current API — trivy's file list gainedgo.sum, Lizard's patterns were renamed and re-levelled (ccn-medium→ccn-minor), and the opengrep/pmd/pylint/revive rule sets moved.plugins/tools/trivy/test/expected.sarif: regenerated — new CVEs in the Trivy DB added rules and shifted everyruleIndex.None of these are behaviour changes; the fixtures were last touched in March and the data behind them has moved since.
mainfails the same way today.Known remaining rot
plugins/tools/trivy/test/expected.sarifis compared against a vulnerability DB that updates daily, so this refresh buys days, not months. The opengrep tool test also failed CI on aconnection reset by peerwhile paginating patterns fromapp.codacy.com— a flake, not a fixture problem, but it shows how much of this suite depends on live network data. Both want a real fix (pinned DB / assert on a subset), tracked separately.Manual Testing