Skip to content

Conversation

@RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Nov 11, 2024

User description

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

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

Description

Adds an example for print page command in C#. I would add the recently-release BiDi version like Java has, but that surface area is not stable yet iirc.

Motivation and Context

Extra example

Types of changes

  • Change to the site (I have double-checked the Netlify deployment, and my changes look good)
  • Code example added (and I also added the example to all translated languages)
  • Improved translation
  • Added new translation (and I also added a notice to each document missing translation)

Checklist

  • I have read the contributing document.
  • I have used hugo to render the site/docs locally and I am sure it works.

PR Type

enhancement, documentation


Description

  • Added a new C# test method PrintWithPrintsPageTest to demonstrate printing a webpage using Selenium.
  • Updated the documentation in multiple languages (English, Japanese, Portuguese, Chinese) to include the new C# print example.

Changes walkthrough 📝

Relevant files
Enhancement
PrintOptionsTest.cs
Add C# test for printing a webpage using Selenium               

examples/dotnet/SeleniumDocs/Interactions/PrintOptionsTest.cs

  • Added a new test method PrintWithPrintsPageTest.
  • Utilizes PrintOptions and PrintDocument to print a page.
  • Includes an assertion to check the printed page is not null.
  • +10/-1   
    Documentation
    print_page.en.md
    Update English documentation with C# print example             

    website_and_docs/content/documentation/webdriver/interactions/print_page.en.md

    • Updated CSharp code block to reference new example.
    +1/-1     
    print_page.ja.md
    Update Japanese documentation with C# print example           

    website_and_docs/content/documentation/webdriver/interactions/print_page.ja.md

    • Updated CSharp code block to reference new example.
    +1/-1     
    print_page.pt-br.md
    Update Portuguese documentation with C# print example       

    website_and_docs/content/documentation/webdriver/interactions/print_page.pt-br.md

    • Updated CSharp code block to reference new example.
    +1/-1     
    print_page.zh-cn.md
    Update Chinese documentation with C# print example             

    website_and_docs/content/documentation/webdriver/interactions/print_page.zh-cn.md

    • Updated CSharp code block to reference new example.
    +1/-1     

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

    @netlify
    Copy link

    netlify bot commented Nov 11, 2024

    👷 Deploy request for selenium-dev pending review.

    Visit the deploys page to approve it

    Name Link
    🔨 Latest commit 988a769

    @CLAassistant
    Copy link

    CLAassistant commented Nov 11, 2024

    CLA assistant check
    All committers have signed the CLA.

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Nov 11, 2024

    PR Reviewer Guide 🔍

    (Review updated until commit 2c2d39b)

    Here are some key observations to aid the review process:

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

    Resource Cleanup
    The WebDriver instance is not properly disposed after test execution. This could lead to browser processes remaining open after tests complete.

    Type Consistency
    The test uses WebDriver type while other methods use IWebDriver interface. Consider using consistent type declarations across the test class.

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Nov 11, 2024

    PR Code Suggestions ✨

    Latest suggestions up to 2c2d39b
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Ensure proper resource cleanup by disposing WebDriver instances after test completion

    Add proper cleanup by disposing the WebDriver instance after the test using a
    try-finally block or IDisposable pattern to prevent resource leaks.

    examples/dotnet/SeleniumDocs/Interactions/PrintOptionsTest.cs [81-88]

     [TestMethod]
     public void PrintWithPrintsPageTest() 
     {
         WebDriver driver = new ChromeDriver();
    -    driver.Navigate().GoToUrl("https://www.selenium.dev/");
    -    PrintOptions printOptions = new PrintOptions();
    -    PrintDocument printedPage = driver.Print(printOptions);
    -    Assert.IsNotNull(printedPage.AsBase64EncodedString);
    +    try {
    +        driver.Navigate().GoToUrl("https://www.selenium.dev/");
    +        PrintOptions printOptions = new PrintOptions();
    +        PrintDocument printedPage = driver.Print(printOptions);
    +        Assert.IsNotNull(printedPage.AsBase64EncodedString);
    +    }
    +    finally {
    +        driver.Quit();
    +    }
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Not disposing WebDriver instances can lead to resource leaks and browser processes remaining open. This is a critical best practice for test cleanup.

    9
    Program to interfaces rather than concrete implementations to improve code flexibility

    Use the more generic IWebDriver interface instead of concrete WebDriver class to
    improve code flexibility and testability.

    examples/dotnet/SeleniumDocs/Interactions/PrintOptionsTest.cs [83]

    -WebDriver driver = new ChromeDriver();
    +IWebDriver driver = new ChromeDriver();
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using IWebDriver interface instead of concrete WebDriver class improves code maintainability and follows dependency injection principles, making the code more flexible and testable.

    6
    Possible issue
    Add error handling for potential failures during print operations

    Add error handling around the print operation since it could fail due to browser or
    network issues.

    examples/dotnet/SeleniumDocs/Interactions/PrintOptionsTest.cs [86-87]

    -PrintDocument printedPage = driver.Print(printOptions);
    -Assert.IsNotNull(printedPage.AsBase64EncodedString);
    +try {
    +    PrintDocument printedPage = driver.Print(printOptions);
    +    Assert.IsNotNull(printedPage.AsBase64EncodedString);
    +}
    +catch (WebDriverException ex) {
    +    Assert.Fail($"Failed to print page: {ex.Message}");
    +}
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding error handling is important as print operations can fail due to various reasons, and proper error messages would help in debugging test failures.

    7

    💡 Need additional feedback ? start a PR chat


    Previous suggestions

    Suggestions up to commit b09c5f0
    CategorySuggestion                                                                                                                                    Score
    Best practice
    Ensure proper resource cleanup by implementing a try-finally block with driver disposal

    Add a try-finally block to ensure the WebDriver is properly disposed of after the
    test, even if an exception occurs.

    examples/dotnet/SeleniumDocs/Interactions/PrintOptionsTest.cs [81-88]

     [TestMethod]
     public void PrintWithPrintsPageTest() 
     {
         WebDriver driver = new ChromeDriver();
    -    driver.Navigate().GoToUrl("https://www.selenium.dev/");
    -    PrintOptions printOptions = new PrintOptions();
    -    PrintDocument printedPage = driver.Print(printOptions);
    -    Assert.IsNotNull(printedPage.AsBase64EncodedString);
    +    try {
    +        driver.Navigate().GoToUrl("https://www.selenium.dev/");
    +        PrintOptions printOptions = new PrintOptions();
    +        PrintDocument printedPage = driver.Print(printOptions);
    +        Assert.IsNotNull(printedPage.AsBase64EncodedString);
    +    }
    +    finally {
    +        driver.Quit();
    +    }
     }
    Suggestion importance[1-10]: 9

    Why: This is a critical improvement for resource management, preventing memory leaks by ensuring the WebDriver is properly disposed of even if the test fails. This is especially important in test code to maintain clean test environments.

    9
    Program to interfaces rather than concrete implementations for better flexibility and maintainability

    Use the more generic IWebDriver interface type instead of the concrete WebDriver
    class to follow interface-based programming principles.

    examples/dotnet/SeleniumDocs/Interactions/PrintOptionsTest.cs [83]

    -WebDriver driver = new ChromeDriver();
    +IWebDriver driver = new ChromeDriver();
    Suggestion importance[1-10]: 7

    Why: Using IWebDriver interface instead of WebDriver concrete class follows good design principles and matches the pattern used in the TestShrinkToFit method above, making the code more consistent and maintainable.

    7
    Enhancement
    Strengthen test assertions to ensure valid output data

    Add a more comprehensive assertion to verify the base64 string is not only non-null
    but also contains valid data.

    examples/dotnet/SeleniumDocs/Interactions/PrintOptionsTest.cs [87]

    -Assert.IsNotNull(printedPage.AsBase64EncodedString);
    +Assert.IsTrue(!string.IsNullOrEmpty(printedPage.AsBase64EncodedString) && printedPage.AsBase64EncodedString.Length > 0);
    Suggestion importance[1-10]: 6

    Why: The enhanced assertion provides more thorough validation by checking both for non-null and non-empty string, which helps catch more potential issues in the printing functionality.

    6

    @RenderMichael RenderMichael marked this pull request as draft November 11, 2024 17:22
    @RenderMichael RenderMichael marked this pull request as ready for review November 11, 2024 17:29
    @qodo-merge-pro
    Copy link
    Contributor

    Persistent review updated to latest commit 2c2d39b

    Copy link
    Member

    @harsha509 harsha509 left a comment

    Choose a reason for hiding this comment

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

    HI @RenderMichael ,

    Thank you for the PR. Could you please work on the above comments

    Thanks,
    Sri

    @RenderMichael
    Copy link
    Contributor Author

    RenderMichael commented Nov 11, 2024

    Hi @harsha509 !

    Thank you for the review. Feedback has been addressed.

    Copy link
    Member

    @harsha509 harsha509 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 @RenderMichael !

    Copy link
    Member

    @harsha509 harsha509 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 @RenderMichael !

    @harsha509 harsha509 merged commit 188a509 into SeleniumHQ:trunk Nov 12, 2024
    9 checks passed
    selenium-ci added a commit that referenced this pull request Nov 12, 2024
    * [.net] Add page print example
    
    * PR feedback
    
    ---------
    
    Co-authored-by: Sri Harsha <[email protected]> 188a509
    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