Skip to content

Conversation

@UGURAKSAHIN
Copy link

@UGURAKSAHIN UGURAKSAHIN commented Mar 23, 2025

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Motivation and Context

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, Tests, Configuration changes


Description

  • Enhanced ExecutorServices with graceful shutdown and error handling.

  • Refactored test methods in ChromeDriverFunctionalTest and LocationContextTest.

  • Increased heap size configurations in .idea files for better performance.

  • Minor updates to AssemblyTeardown.cs with additional imports.


Changes walkthrough 📝

Relevant files
Enhancement
ExecutorServices.java
Enhance graceful shutdown and error handling                         

java/src/org/openqa/selenium/concurrent/ExecutorServices.java

  • Added DEFAULT_SHUTDOWN_TIMEOUT constant for shutdown timeout.
  • Introduced awaitTermination and forceShutdown methods for better
    shutdown handling.
  • Improved error handling and logging during service shutdown.
  • +18/-9   
    Tests
    ChromeDriverFunctionalTest.java
    Refactor ChromeDriver functional test methods                       

    java/test/org/openqa/selenium/chrome/ChromeDriverFunctionalTest.java

  • Refactored test to use helper methods for initialization.
  • Added initializeChromeDriver and assertDefaultChromeCapabilities
    methods.
  • +9/-2     
    LocationContextTest.java
    Refactor LocationContext test methods                                       

    java/test/org/openqa/selenium/html5/LocationContextTest.java

  • Refactored location tests to use helper method setAndRetrieveLocation.
  • Improved readability and reusability of test code.
  • +7/-7     
    Miscellaneous
    AssemblyTeardown.cs
    Minor updates to AssemblyTeardown.cs                                         

    dotnet/test/remote/AssemblyTeardown.cs

  • Added System namespace import.
  • Minor updates to improve code structure.
  • +1/-0     
    Configuration changes
    androidDexCompiler.xml
    Update Android Dex Compiler heap size                                       

    .idea/androidDexCompiler.xml

    • Increased MAX_HEAP_SIZE from 4096 to 8192.
    +1/-1     
    compiler.xml
    Update compiler heap size configurations                                 

    .idea/compiler.xml

  • Increased BUILD_PROCESS_HEAP_SIZE and MAXIMUM_HEAP_SIZE to 2048.
  • Improved build process configuration for better performance.
  • +3/-3     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @CLAassistant
    Copy link

    CLAassistant commented Mar 23, 2025

    CLA assistant check
    All committers have signed the CLA.

    @qodo-code-review
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Syntax Error

    The method call to Thread.currentThread().interrupt is missing parentheses and should be Thread.currentThread().interrupt(). Also, the variable 'e' is used but not defined in the catch block.

    Thread.currentThread().interrupt;
    LOG.log(WARNING, String.format("Failed to shutdown %s", name), e);
    Method Definition Error

    The forceShutDown method is incorrectly defined inside the awaitTermination method. It should be defined at the class level with proper method signature and closing braces.

      private static void forceShutDown(String name, ExecutorService, service){
        LOG.warning(String.format("Failed to shutdown %s", name), e);
        service.shutdownNow();
    
    }
    Missing Method

    The test references a setAndRetrieveLocation helper method that is used in multiple tests but is not defined in the provided code.

    import org.openqa.selenium.chromium.HasPermissions;

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Mar 23, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix method call syntax

    The interrupt() method is missing parentheses. It should be called as
    Thread.currentThread().interrupt() since it's a method call, not a property
    access.

    java/src/org/openqa/selenium/concurrent/ExecutorServices.java [42-45]

     }catch(InterruptedException ex){
    -  Thread.currentThread().interrupt;
    -  LOG.log(WARNING, String.format("Failed to shutdown %s", name), e);
    +  Thread.currentThread().interrupt();
    +  LOG.log(WARNING, String.format("Failed to shutdown %s", name), ex);
       return false;
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    __

    Why: This suggestion fixes a critical syntax error where the interrupt() method is missing parentheses, which would cause a compilation failure. The code would not run without this fix, making it a high-impact correction.

    High
    Fix variable reference

    The exception variable used in the log statement is e, but the caught exception
    is named ex. Use the correct variable name to properly log the exception.

    java/src/org/openqa/selenium/concurrent/ExecutorServices.java [44]

    -LOG.log(WARNING, String.format("Failed to shutdown %s", name), e);
    +LOG.log(WARNING, String.format("Failed to shutdown %s", name), ex);
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: This suggestion fixes an incorrect variable reference where the exception variable 'e' is used in the log statement but the caught exception is named 'ex'. This would cause a compilation error as 'e' is undefined, making it a critical fix.

    High
    • Update

    @UGURAKSAHIN UGURAKSAHIN changed the title Rcovering Recovering Mar 23, 2025
    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