Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[java] SpotBugs exclude NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE from the firefox.AddHasExtensions$1 #14766

Merged
merged 2 commits into from
Nov 28, 2024

Conversation

mk868
Copy link
Contributor

@mk868 mk868 commented Nov 17, 2024

User description

Description

As described in this comment, SpotBugs found some additional bugs in the code.
In this PR I fix part of the found problems.

SpotBugs error:

Executing tests from //java/src/org/openqa/selenium/firefox:firefox-lib-spotbugs
-----------------------------------------------------------------------------
M D NP: Possible null pointer dereference in org.openqa.selenium.firefox.AddHasExtensions$1.zipDirectory(Path) due to return value of called method  Dereferenced at AddHasExtensions.java:[line 104]

documentation

Solution:
Exclude this anonymous class from SpotBugs checks. A similar exclusion already exists for org.openqa.selenium.grid.config.Configs.

Motivation and Context

Fixing the actual problems is necessary before enabling full SpotBugs analysis, in order to not break the build.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Bug fix


Description

  • Added a SpotBugs exclusion for the NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE bug pattern in the org.openqa.selenium.firefox.AddHasExtensions.* class.
  • This change prevents SpotBugs from reporting possible null pointer dereference issues in this class, aligning with existing exclusions for similar cases.
  • The update is part of efforts to fix issues before enabling full SpotBugs analysis to avoid build failures.

Changes walkthrough 📝

Relevant files
Bug fix
spotbugs-excludes.xml
Exclude null pointer dereference bug for AddHasExtensions

java/spotbugs-excludes.xml

  • Added exclusion for NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE bug
    pattern.
  • Applied exclusion to org.openqa.selenium.firefox.AddHasExtensions.*
    class.
  • +5/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Technical Debt
    Suppressing SpotBugs warning instead of fixing the underlying null pointer issue. Consider addressing the root cause in AddHasExtensions class.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use precise class pattern matching to avoid over-exclusion of SpotBugs checks

    Consider using a more specific class pattern instead of wildcard (*) to avoid
    accidentally excluding unintended classes. Target only the specific inner class that
    needs the exclusion.

    java/spotbugs-excludes.xml [57-60]

     <Match>
    -  <Class name="~org.openqa.selenium.firefox.AddHasExtensions.*"/>
    +  <Class name="org.openqa.selenium.firefox.AddHasExtensions$1"/>
       <Bug pattern="NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE"/>
     </Match>
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The suggestion to use a more specific class pattern instead of wildcard is a good practice to prevent unintended exclusions of SpotBugs checks. However, without context about the specific inner classes involved, we can't be certain this won't exclude necessary cases.

    6

    💡 Need additional feedback ? start a PR chat

    @pujagani pujagani merged commit a96f6aa into SeleniumHQ:trunk Nov 28, 2024
    1 check passed
    @mk868 mk868 deleted the fix-firefox-AddHasExtensions branch November 28, 2024 11:40
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants