Skip to content

Add several test and dev configurations#1684

Closed
jeffalder wants to merge 5 commits intofossas:masterfrom
jeffalder:patch-1
Closed

Add several test and dev configurations#1684
jeffalder wants to merge 5 commits intofossas:masterfrom
jeffalder:patch-1

Conversation

@jeffalder
Copy link
Copy Markdown
Contributor

@jeffalder jeffalder commented Apr 7, 2026

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

  1. Examined a Gradle project to see what the configuration names were.
  2. Read the Gradle and Spotbugs Gradle Plugin docs to ensure this was accurate.

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 jacocoAgent and jacocoAnt configurations

https://docs.gradle.org/current/userguide/jacoco_report_aggregation_plugin.html
-> lists jacocoAggregation and aggregateCodeCoverageReportResults configurations

https://github.com/spotbugs/spotbugs-gradle-plugin?tab=readme-ov-file#configure-spotbugs-plugin
-> lists spotbugs and spotbugsPlugins configurations

https://github.com/spotbugs/spotbugs-gradle-plugin/blob/0244688bb553707a1954145c8fd1c14a593a20e1/src/main/kotlin/com/github/spotbugs/snom/SpotBugsTask.kt#L385
-> lists spotbugsSlf4j configuration

Checklist

  • I added tests for this PR's change (or explained in the PR description why tests don't make sense).
  • If this PR introduced a user-visible change, I added documentation into docs/.
  • n/a If this PR added docs, I added links as appropriate to the user manual's ToC in docs/README.ms and gave consideration to how discoverable or not my documentation is.
  • If this change is externally visible, I updated Changelog.md. If this PR did not mark a release, I added my changes into an ## Unreleased section at the top.
  • n/a If I made changes to .fossa.yml or fossa-deps.{json.yml}, I updated docs/references/files/*.schema.json AND I have updated example files used by fossa init command. 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).
  • n/a If I made changes to a subcommand's options, I updated docs/references/subcommands/<subcommand>.md.

@jeffalder jeffalder requested a review from a team as a code owner April 7, 2026 12:41
@jeffalder jeffalder requested a review from nficca April 7, 2026 12:41
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 7, 2026

Walkthrough

The configNameToLabel function in src/Strategy/Gradle/Common.hs was updated to map Gradle config names from a newly introduced internal list knownDevConfigs to the EnvDevelopment environment. This extends the existing mapping logic that already handles explicit "compileOnly" configurations, test configs via knownTestConfigs, and Android platform-specific checks. The knownTestConfigs list was expanded to include Jacoco-related configuration names (aggregateCodeCoverageReportResults, jacocoAgent, jacocoAggregation, jacocoAnt). The knownDevConfigs list contains spotbugs-related configurations. No changes were made to exported or public entity signatures.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately captures the main purpose of the PR—adding support for several test and development configurations in Gradle—and directly relates to the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check ✅ Passed The PR description covers all required sections with comprehensive detail including overview, acceptance criteria, testing plan, risks, metrics, references, and completed checklist.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

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 | 🟠 Major

Add 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 | 🟠 Major

Refactor configNameToLabel to 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 conf

Additionally, add unit tests for configNameToLabel covering the new configurations in knownDevConfigs and knownTestConfigs to 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

📥 Commits

Reviewing files that changed from the base of the PR and between 3341930 and 9574bc0.

📒 Files selected for processing (1)
  • src/Strategy/Gradle/Common.hs

@jeffalder
Copy link
Copy Markdown
Contributor Author

@nficca I suspect that workflows are awaiting your approval. LMK if I need to sign a CLA.

@jeffalder
Copy link
Copy Markdown
Contributor Author

Some comments are outside the diff and can’t be posted inline due to platform limitations.

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.

Copy link
Copy Markdown
Member

@zlav zlav left a comment

Choose a reason for hiding this comment

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

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.

@zlav
Copy link
Copy Markdown
Member

zlav commented Apr 8, 2026

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.
#1685

@zlav zlav closed this Apr 8, 2026
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.

2 participants