Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Expand documentation on fresh browser per test #1671

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

Closed
jflorez wants to merge 7 commits into SeleniumHQ:trunk from jflorez:patch-1

Conversation

@jflorez
Copy link

@jflorez jflorez commented Apr 15, 2024 •
edited by qodo-merge-pro bot
Loading

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

Added an extended explanation and code examples on how to implement a fresh browser per test. Tried to avoid specific framework code like JUnit vs TestNG.

Motivation and Context

I see a lot of Selenium automation project in the industry with a static WebDriver that gets shared between tests and across all these teams there has always been confusion on how to get started with a fresh browser per test. This is made more prominent with requirements to run tests is parallel and a lot of automation coders not understanding the implications a static driver (without ThreadLocal which brings its own complications) has in attempts to run parallel tests in multi-threaded languages like Java or C#

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.

Type

documentation, enhancement

Description

  • Added comprehensive examples in Java and Python across English, Japanese, Portuguese, and Chinese documentation for setting up a fresh browser instance for each test.
  • Discussed the drawbacks of using a static WebDriver, especially in the context of parallel testing, and recommended using ThreadLocal for maintaining thread safety.
  • Aimed at clarifying best practices for test isolation and reducing confusion around setting up WebDriver instances for Selenium tests.

Changes walkthrough

Relevant files
Documentation
fresh_browser_per_test.en.md
Enhance Documentation with Fresh Browser Per Test Examples

website_and_docs/content/documentation/test_practices/encouraged/fresh_browser_per_test.en.md

  • Added detailed explanation and Java, Python code examples for
    implementing a fresh browser per test.
  • Highlighted issues with using a static WebDriver and recommended using
    ThreadLocal for thread safety.
    • +54/-1
    fresh_browser_per_test.ja.md
    Add Fresh Browser Per Test Examples in Japanese Documentation

    website_and_docs/content/documentation/test_practices/encouraged/fresh_browser_per_test.ja.md

  • Added Java code examples for setting up a fresh browser per test.
  • Discussed the drawbacks of using a static WebDriver in parallel test
    execution.
    • +50/-1
    fresh_browser_per_test.pt-br.md
    Update Portuguese Documentation with Browser Per Test Strategy

    website_and_docs/content/documentation/test_practices/encouraged/fresh_browser_per_test.pt-br.md

  • Introduced Java and Python examples for initiating a new browser
    instance per test.
  • Explained the complications of using static WebDriver with ThreadLocal
    for thread safety.
    • +49/-1
    fresh_browser_per_test.zh-cn.md
    Enhance Chinese Documentation with New Browser Per Test Examples

    website_and_docs/content/documentation/test_practices/encouraged/fresh_browser_per_test.zh-cn.md

  • Provided Java, Python examples for fresh browser setup per test.
  • Addressed the issue of using static WebDriver and the importance of
    ThreadLocal in test isolation.
    • +49/-1

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

    Copy link

    netlify bot commented Apr 15, 2024 •
    edited
    Loading

    👷 Deploy request for selenium-dev pending review.

    Visit the deploys page to approve it

    Name Link
    🔨 Latest commit 8d72b74

    Copy link

    CLAassistant commented Apr 15, 2024 •
    edited
    Loading

    CLA assistant check
    All committers have signed the CLA.

    @qodo-merge-pro qodo-merge-pro bot added documentation Improvements or additions to documentation enhancement New feature or request labels Apr 15, 2024
    Copy link
    Contributor

    PR Description updated to latest commit (7f2aa0a)

    Copy link
    Contributor

    PR Review

    ⏱️ Estimated effort to review [1-5]

    2, because the PR primarily focuses on expanding documentation with code examples across multiple languages. The changes are straightforward and involve adding explanations and code snippets to illustrate the concept of using a fresh browser for each test. The complexity is low, and the main effort would be in verifying the correctness and clarity of the examples provided.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Clarity: The documentation could benefit from a brief explanation of why using a static WebDriver can lead to issues, especially for readers who might not be familiar with concepts like thread safety and parallel test execution.

    Consistency: Ensure that the code examples provided are consistent across different language versions of the documentation. This includes not only the code syntax but also the comments and explanations that accompany them.

    🔒 Security concerns

    No
    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

    [pr_reviewer]
some_config1=...
some_config2=...

    See the review usage page for a comprehensive guide on using this tool.
    Copy link
    Contributor

    PR Code Suggestions

    CategorySuggestions
    Maintainability
    Improve code indentation for better readability.

    Consider using proper indentation to enhance code readability. In Java code blocks, it's
    common practice to use 4 spaces or a tab for indentation. The inconsistent indentation in
    the BaseTest class and ThreadLocal example makes the code harder to read.

    website_and_docs/content/documentation/test_practices/encouraged/fresh_browser_per_test.en.md [22-64] 

     public abstract class BaseTest {
-protected WebDriver driver;
-...
+ protected WebDriver driver;
+ ...
    Bug
    Correct variable reference in Python fixture to prevent AttributeError.

    In the Python fixture example, there's a mistake where yield self.driver is used instead
    of yield driver. This could lead to an AttributeError since self.driver is not defined
    within the fixture's scope.

    website_and_docs/content/documentation/test_practices/encouraged/fresh_browser_per_test.en.md [48] 

    -yield self.driver
+yield driver
    Enhancement
    Clarify the use of ThreadLocal for parallel test execution.

    For the ThreadLocal example, consider explaining the necessity of using ThreadLocal for
    WebDriver instances in the context of parallel test execution. This will help readers
    understand why ThreadLocal is used instead of a simpler approach.

    website_and_docs/content/documentation/test_practices/encouraged/fresh_browser_per_test.en.md [59] 

    -This forces the ThreadLocal<WebDriver> variable to call driver.get() every time the driver wants to be used.
+This forces the ThreadLocal<WebDriver> variable to call driver.get() every time the driver wants to be used, which is essential for thread-safe operations during parallel test execution.
    Enhance Python fixture with error handling for improved debugging.

    To enhance the Python fixture example for better test isolation and debugging, consider
    adding a try-except block around the yield statement. This can be used to catch and log
    exceptions from the test, potentially adding screenshots or additional debugging
    information before the browser quits.

    website_and_docs/content/documentation/test_practices/encouraged/fresh_browser_per_test.en.md [48-51] 

    -yield driver
-driver.quit()
+try:
+ yield driver
+finally:
+ # Optional: Add error handling or screenshots here
+ driver.quit()
    Best practice
    Ensure WebDriver is properly closed with a finally block.

    To avoid potential resource leaks, it's a good practice to wrap the driver.quit() call in
    a finally block within the teardownTest method. This ensures that the WebDriver is
    properly closed even if an exception occurs.

    website_and_docs/content/documentation/test_practices/encouraged/fresh_browser_per_test.en.md [33-35] 

     public void teardownTest() {
- ...
- driver.quit();
+ try {
+ ...
+ } finally {
+ driver.quit();
+ }
 }
    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

    [pr_code_suggestions]
some_config1=...
some_config2=...

    See the improve usage page for a comprehensive guide on using this tool.

    Copy link
    Member

    @diemol diemol 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 for improving our docs, @jflorez!

    We are trying to move away from code in markdown files as it is hard to maintain. We prefer to link the actual code through the Hugo shortcode. Here is an example.

    A1exKH reacted with thumbs up emoji
    Comment on lines +42 to +52
    @pytest.fixture(autouse=True, scope='function')
    def driver(self, request, page: Page):
    # Create the driver
    driver = webdriver.Firefox()

    # Return control to the test
    yield self.driver

    # Test ends driver quits
    driver.quit()
    ```
    Copy link
    Member

    Choose a reason for hiding this comment

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

    A1exKH reacted with thumbs up emoji
    Comment on lines +59 to +71
    # This forces the ThreadLocal<WebDriver> variable to call driver.get() every time the driver wants to be used.

    # In general static variables in non-thread safe code can have unintended consequences and increase the maintanance effort in the code base.

    public abstract class BaseTest {
    protected ThreadLocal<WebDriver> driver;
    ...
    // Before each test hook
    public void setupTest() {
    BaseTest.driver = ThreadLocal.withInitial(()->new FirefoxDriver());
    ...
    }
    }
    Copy link
    Member

    Choose a reason for hiding this comment

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

    Copy link
    Author

    jflorez commented Apr 15, 2024

    I was wondering about showing ThreadLocal or not given its many issues. I'll remove the references to existing code in examples

    Copy link
    Member

    diemol commented Jun 18, 2025

    We never got a reply to the comments. Closing.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @diemol diemol diemol requested changes

    Assignees

    No one assigned

    Labels

    documentation Improvements or additions to documentation enhancement New feature or request Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    AltStyle によって変換されたページ (->オリジナル) /