SeleniumHQ / seleniumhq.github.io

Official Selenium website and documentation
https://selenium.dev
Apache License 2.0
1.09k stars 1.28k forks source link

Adding WaitForDownload test for .NET #1885

Open halex2005 opened 3 weeks ago

halex2005 commented 3 weeks ago

User description

Hi!

Description

I've added WaitForDownload test case for .NET. Review, please.

Note, that I've also added --no-sandbox switch to ChromeOptions because chrome crashes without it. It's to make sure that tests are green.

Types of changes

Checklist


PR Type

Tests, Enhancement


Description


Changes walkthrough ๐Ÿ“

Relevant files
Enhancement
BaseTest.cs
Add no-sandbox argument to ChromeOptions                                 

examples/dotnet/SeleniumDocs/BaseTest.cs
  • Added --no-sandbox argument to ChromeOptions.
  • Ensures Chrome does not crash during tests.
  • +1/-0     
    Tests
    NetworkTest.cs
    Add WaitForDownload test method for file downloads             

    examples/dotnet/SeleniumDocs/BiDi/CDP/NetworkTest.cs
  • Added WaitForDownload test method.
  • Implemented download behavior settings and event handling.
  • Verified file download completion and existence.
  • +47/-3   
    Documentation
    network.en.md
    Update CSharp tab with code block reference                           

    website_and_docs/content/documentation/webdriver/bidi/cdp/network.en.md - Updated CSharp tab with code block reference.
    +1/-1     
    network.ja.md
    Update CSharp tab with code block reference                           

    website_and_docs/content/documentation/webdriver/bidi/cdp/network.ja.md - Updated CSharp tab with code block reference.
    +1/-1     
    network.pt-br.md
    Update CSharp tab with code block reference                           

    website_and_docs/content/documentation/webdriver/bidi/cdp/network.pt-br.md - Updated CSharp tab with code block reference.
    +1/-1     
    network.zh-cn.md
    Update CSharp tab with code block reference                           

    website_and_docs/content/documentation/webdriver/bidi/cdp/network.zh-cn.md - Updated CSharp tab with code block reference.
    +1/-1     

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

    netlify[bot] commented 3 weeks ago

    Deploy request for selenium-dev pending review.

    Visit the deploys page to approve it

    Name Link
    Latest commit 167d479bb1663bcb93fea905cbc419f5a6b5fbfb
    CLAassistant commented 3 weeks ago

    CLA assistant check
    All committers have signed the CLA.

    codiumai-pr-agent-pro[bot] commented 3 weeks ago

    PR Reviewer Guide ๐Ÿ”

    โฑ๏ธ Estimated effort to review: 2 ๐Ÿ”ต๐Ÿ”ตโšชโšชโšช
    ๐Ÿงช PR contains tests
    ๐Ÿ”’ Security concerns

    Security concern:
    Adding '--no-sandbox' to ChromeOptions (examples/dotnet/SeleniumDocs/BaseTest.cs, line 42) disables the Chrome sandbox, which is a security feature. This can potentially expose the system to security risks if malicious code is executed. While it may be necessary for testing environments, it should be used with caution and not in production environments.
    โšก Key issues to review

    Security Concern
    Adding '--no-sandbox' argument to ChromeOptions may introduce security risks in production environments. Error Handling
    The WaitForDownload test method lacks proper error handling for potential exceptions during file operations.
    codiumai-pr-agent-pro[bot] commented 3 weeks ago

    PR Code Suggestions โœจ

    CategorySuggestion                                                                                                                                    Score
    Error handling
    Add error handling when deleting files to manage potential exceptions ___ **Consider using File.Delete() within a try-catch block to handle potential exceptions
    that may occur during file deletion, such as access denied or file not found errors.** [examples/dotnet/SeleniumDocs/BiDi/CDP/NetworkTest.cs [192]](https://github.com/SeleniumHQ/seleniumhq.github.io/pull/1885/files#diff-e31523c285a75f1ed30aba52eb3c36546df30c19cd434a14ffbf02c431da54fcR192-R192) ```diff -File.Delete(downloadedFilePath); +try +{ + File.Delete(downloadedFilePath); +} +catch (IOException ex) +{ + Console.WriteLine($"Error deleting file: {ex.Message}"); +} ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 9 Why: This suggestion significantly enhances the code by adding error handling for file deletion, which is crucial for managing exceptions like access denied or file not found, thereby improving reliability.
    9
    Add exception handling to the event handler to catch and log specific errors ___ **Consider using a more specific exception type instead of Exception when catching
    exceptions. This helps in better error handling and provides more information about
    the specific error that occurred.** [examples/dotnet/SeleniumDocs/BiDi/CDP/NetworkTest.cs [173-183]](https://github.com/SeleniumHQ/seleniumhq.github.io/pull/1885/files#diff-e31523c285a75f1ed30aba52eb3c36546df30c19cd434a14ffbf02c431da54fcR173-R183) ```diff session.DevToolsEventReceived += (sender, args) => { - var downloadState = args.EventData["state"]?.ToString(); - if (args.EventName == "downloadProgress" && - (string.Equals(downloadState, "completed") || - string.Equals(downloadState, "canceled"))) + try { - downloadId = args.EventData["guid"].ToString(); - downloaded = downloadState.Equals("completed"); - downloadCompleted.Set(); + var downloadState = args.EventData["state"]?.ToString(); + if (args.EventName == "downloadProgress" && + (string.Equals(downloadState, "completed") || + string.Equals(downloadState, "canceled"))) + { + downloadId = args.EventData["guid"].ToString(); + downloaded = downloadState.Equals("completed"); + downloadCompleted.Set(); + } + } + catch (KeyNotFoundException ex) + { + Console.WriteLine($"Error accessing EventData: {ex.Message}"); } }; ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: The suggestion correctly adds exception handling to the event handler, which enhances error management by catching specific exceptions and logging them, improving robustness and debugging capabilities.
    8
    Best practice
    Use null-coalescing operator when combining paths to handle potential null values ___ **Consider using Path.Combine() to construct the file path instead of string
    concatenation. This ensures that the correct path separator is used for the current
    operating system.** [examples/dotnet/SeleniumDocs/BiDi/CDP/NetworkTest.cs [190]](https://github.com/SeleniumHQ/seleniumhq.github.io/pull/1885/files#diff-e31523c285a75f1ed30aba52eb3c36546df30c19cd434a14ffbf02c431da54fcR190-R190) ```diff -var downloadedFilePath = Path.Combine(downloadPath, downloadId); +var downloadedFilePath = Path.Combine(downloadPath, downloadId ?? string.Empty); ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: The suggestion improves the code by using a null-coalescing operator to handle potential null values when combining paths, ensuring that the path construction is robust and error-free.
    7
    Documentation
    Add a comment to explain the usage of a potentially security-impacting browser argument ___ **Consider adding a comment explaining why the "--no-sandbox" argument is being used.
    This argument can have security implications, so it's important to document the
    reason for its usage.** [examples/dotnet/SeleniumDocs/BaseTest.cs [35]](https://github.com/SeleniumHQ/seleniumhq.github.io/pull/1885/files#diff-13e7f59b8933cf7834665df17e6282b912e7b6a94eb3d3ea51e02517365c77ceR35-R35) ```diff +// Adding --no-sandbox for CI/CD environments or when running with limited privileges options.AddArgument("--no-sandbox"); ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 6 Why: Adding a comment to explain the use of "--no-sandbox" is a good practice for documentation, especially given its security implications, but it is not critical to the functionality of the code.
    6
    diemol commented 3 weeks ago

    @halex2005 can you please sign the CLA?

    halex2005 commented 3 weeks ago

    @diemol, signed