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] Add JSpecify annotations for WebDriver and 3 other interfaces #14371

Merged
merged 2 commits into from
Aug 14, 2024

Conversation

mk868
Copy link
Contributor

@mk868 mk868 commented Aug 10, 2024

User description

Description

In this PR I'm adding nullness annotations for interfaces:

  • WebDriver
  • TakesScreenshot
  • OutputType
  • JavascriptExecutor

WebDriver

Null values occur only in methods:

  • String getTitle() - the JavaDoc says: "return (...) null if one is not already set" -> marked as @Nullable
  • Cookie Options#getCookieNamed(String name) - if there is no cookie with the specified name, returns a null value -> marked as @Nullable
  • String getPageSource() - I'm not sure about that looking at W3C WebDriver specification, as far as I can see in implementations, returning a null value is possible -> marked as @Nullable
  • String getCurrentUrl() - From the HtmlUnit driver implementation, a null value is possible when the page is not loaded -> marked as @Nullable

Notes:

  • the RemoteWebDriver#getTitle() implementation actually returns an empty string when the title is null

TakesScreenshot & OutputType

These interfaces are closely related to each other, there are no null values.

JavascriptExecutor

Null values occur only in methods:

  • Object executeScript(String script, Object... args)
  • Object executeAsyncScript(String script, Object... args)
  • Object executeScript(ScriptKey key, Object... args)

The result can be null, so the Object return type marked as @Nullable.
The elements in the args array can be null (args itself cannot be null), so the array argument type marked as @Nullable.

Motivation and Context

The JSpecify nullness annotations will give developers better exposure to potential problems with their code to avoid NullPointerExceptions.
Related issue: #14291

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

Enhancement


Description

  • Added JSpecify nullness annotations to WebDriver, TakesScreenshot, OutputType, and JavascriptExecutor interfaces.
  • Marked methods and parameters with @Nullable where null values are possible.
  • Enhanced code to provide better nullness checks and avoid potential NullPointerExceptions.

Changes walkthrough 📝

Relevant files
Enhancement
JavascriptExecutor.java
Add JSpecify annotations to JavascriptExecutor interface 

java/src/org/openqa/selenium/JavascriptExecutor.java

  • Added @NullMarked annotation to the interface.
  • Marked return types and parameters with @Nullable where applicable.
  • +6/-3     
    OutputType.java
    Add JSpecify annotations to OutputType interface                 

    java/src/org/openqa/selenium/OutputType.java

    • Added @NullMarked annotation to the interface.
    +2/-0     
    TakesScreenshot.java
    Add JSpecify annotations to TakesScreenshot interface       

    java/src/org/openqa/selenium/TakesScreenshot.java

    • Added @NullMarked annotation to the interface.
    +3/-0     
    WebDriver.java
    Add JSpecify annotations to WebDriver interface                   

    java/src/org/openqa/selenium/WebDriver.java

  • Added @NullMarked annotation to the interface.
  • Marked return types and parameters with @Nullable where applicable.
  • +6/-3     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Annotation Consistency
    The use of @Nullable on executeScript and executeAsyncScript methods should be reviewed for consistency. The PR description mentions that args itself cannot be null, but the annotation suggests otherwise.

    Misleading Documentation
    The getTitle method documentation in RemoteWebDriver contradicts the @Nullable annotation by stating it returns an empty string instead of null. This could lead to confusion and should be clarified or corrected.

    Copy link

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @mk868 mk868 force-pushed the add-jspecify-annotations branch 2 times, most recently from 12e59fa to ce7ed00 Compare August 11, 2024 07:43
    @diemol diemol merged commit 5e2a630 into SeleniumHQ:trunk Aug 14, 2024
    27 of 30 checks passed
    @mk868 mk868 deleted the add-jspecify-annotations branch August 14, 2024 12:43
    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.

    2 participants