Add several test and dev configurations#1684
Add several test and dev configurations#1684jeffalder wants to merge 5 commits intofossas:masterfrom
Conversation
WalkthroughThe 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Strategy/Gradle/Common.hs (2)
49-74:⚠️ Potential issue | 🟠 MajorAdd unit tests for the new JaCoCo and SpotBugs configuration mappings.
These new literals directly affect environment labeling, but this change set shows no accompanying tests for
configNameToLabel(e.g.,jacocoAgent,jacocoAggregation,spotbugs,spotbugsPlugins,spotbugsSlf4j). Please add table-driven coverage for these cases to prevent regressions.As per coding guidelines, "Code should be thoroughly tested with appropriate unit tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Strategy/Gradle/Common.hs` around lines 49 - 74, The change adds new Gradle config literals but lacks unit tests for configNameToLabel; add table-driven unit tests exercising configNameToLabel for the new JaCoCo and SpotBugs entries ("jacocoAgent", "jacocoAggregation", "jacocoAnt", and the spotbugs entries "spotbugs", "spotbugsPlugins", "spotbugsSlf4j") and assert they map to the expected environment labels; place these cases alongside existing tests for configNameToLabel (or in the same test module for Strategy.Gradle.Common) so future regressions are caught.
39-47: 🛠️ Refactor suggestion | 🟠 MajorRefactor
configNameToLabelto avoid match guards.Lines 42–45 use match guards, which violates the coding guideline "avoid partial functions, list comprehensions, and match guards." Refactor using conditional logic instead:
Proposed refactor (no guards)
configNameToLabel :: Text -> GradleLabel configNameToLabel conf = - case conf of - x | x `elem` knownDevConfigs -> Env EnvDevelopment - x | x `elem` knownTestConfigs -> Env EnvTesting - x | isDefaultAndroidDevConfig x -> Env EnvDevelopment - x | isDefaultAndroidTestConfig x -> Env EnvTesting - x -> Env $ EnvOther x + if conf `elem` knownDevConfigs || isDefaultAndroidDevConfig conf + then Env EnvDevelopment + else if conf `elem` knownTestConfigs || isDefaultAndroidTestConfig conf + then Env EnvTesting + else Env $ EnvOther confAdditionally, add unit tests for
configNameToLabelcovering the new configurations inknownDevConfigsandknownTestConfigsto ensure correct classification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Strategy/Gradle/Common.hs` around lines 39 - 47, The function configNameToLabel currently uses match guards; refactor it to eliminate guards by using explicit conditional checks (if/then/else or nested case) that call knownDevConfigs, knownTestConfigs, isDefaultAndroidDevConfig and isDefaultAndroidTestConfig in sequence and return Env EnvDevelopment, Env EnvTesting or Env (EnvOther x) accordingly (preserve the special "compileOnly" branch). After refactoring, add unit tests for configNameToLabel that iterate over entries in knownDevConfigs and knownTestConfigs (and at least one default Android dev/test config and a fallback) to assert correct mapping to EnvDevelopment, EnvTesting, and EnvOther.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Strategy/Gradle/Common.hs`:
- Around line 49-74: The change adds new Gradle config literals but lacks unit
tests for configNameToLabel; add table-driven unit tests exercising
configNameToLabel for the new JaCoCo and SpotBugs entries ("jacocoAgent",
"jacocoAggregation", "jacocoAnt", and the spotbugs entries "spotbugs",
"spotbugsPlugins", "spotbugsSlf4j") and assert they map to the expected
environment labels; place these cases alongside existing tests for
configNameToLabel (or in the same test module for Strategy.Gradle.Common) so
future regressions are caught.
- Around line 39-47: The function configNameToLabel currently uses match guards;
refactor it to eliminate guards by using explicit conditional checks
(if/then/else or nested case) that call knownDevConfigs, knownTestConfigs,
isDefaultAndroidDevConfig and isDefaultAndroidTestConfig in sequence and return
Env EnvDevelopment, Env EnvTesting or Env (EnvOther x) accordingly (preserve the
special "compileOnly" branch). After refactoring, add unit tests for
configNameToLabel that iterate over entries in knownDevConfigs and
knownTestConfigs (and at least one default Android dev/test config and a
fallback) to assert correct mapping to EnvDevelopment, EnvTesting, and EnvOther.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1690f4a5-aff3-41c1-be2b-86f141d509d9
📒 Files selected for processing (1)
src/Strategy/Gradle/Common.hs
|
@nficca I suspect that workflows are awaiting your approval. LMK if I need to sign a CLA. |
Regarding match guards -- this is existing code, and I don't feel it's appropriate to lump in a refactor with this low-risk change. Regarding unit tests -- as specified in the description, unit testing of the branch logic already exists. This only augments existing lists for the branch logic, so the current tests should be sufficient. |
zlav
left a comment
There was a problem hiding this comment.
Thank you for the PR!
The behavior here looks correct and should fix the issue you are seeing. Also, thank you for filling out a solid PR description.
I am approving this, but I may have some issues running tests due to auth with internal tools that get compiled into the final binary. If that is the case, I will open a duplicate PR and merge that. Thank you, this should be in the next release of the FOSSA CLI.
|
Ok, I can't get this PR to run through the test suite, so I opened a duplicate PR that retains the same changes, comments, and your description. Apologies for not being able to get this merged, but ultimately thank you for the changes. |
Overview
This PR classifies certain Gradle configurations as being development-only or test-only configurations, in order to reduce false positives.
Acceptance criteria
When users run fossa analyze on a Gradle project with spotbugs or jacoco plugins applied, they should no longer see security warnings for dependencies only in those configurations.
Testing plan
This change modifies one list, adds one list, and modifies a branch to check the list instead of a single constant. These branches were already covered by existing tests, and comprehensive testing of list contents doesn't currently exist (and would be superfluous).
Risks
None that I'm aware of.
Metrics
None.
References
https://docs.gradle.org/current/userguide/jacoco_plugin.html
-> lists
jacocoAgentandjacocoAntconfigurationshttps://docs.gradle.org/current/userguide/jacoco_report_aggregation_plugin.html
-> lists
jacocoAggregationandaggregateCodeCoverageReportResultsconfigurationshttps://github.com/spotbugs/spotbugs-gradle-plugin?tab=readme-ov-file#configure-spotbugs-plugin
-> lists
spotbugsandspotbugsPluginsconfigurationshttps://github.com/spotbugs/spotbugs-gradle-plugin/blob/0244688bb553707a1954145c8fd1c14a593a20e1/src/main/kotlin/com/github/spotbugs/snom/SpotBugsTask.kt#L385
-> lists
spotbugsSlf4jconfigurationChecklist
docs/.docs/README.msand gave consideration to how discoverable or not my documentation is.Changelog.md. If this PR did not mark a release, I added my changes into an## Unreleasedsection at the top..fossa.ymlorfossa-deps.{json.yml}, I updateddocs/references/files/*.schema.jsonAND I have updated example files used byfossa initcommand. You may also need to update these if you have added/removed new dependency type (e.g.pip) or analysis target type (e.g.poetry).docs/references/subcommands/<subcommand>.md.