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

[dotnet] ability to create tests using mocked driver #14249

Conversation

Indomitable
Copy link
Contributor

@Indomitable Indomitable commented Jul 10, 2024

User description

I noticed that all tests require real browser driver, but I think it can be useful to have ability to write tests which doesn't need it. We can use WireMock to mock driver responses and write more detailed tests.

It contains a test that tests my changes in PR: #14242

Unfortunately my knowledge with bazel is quite limited in order to add integration for bazel test but for this dotnet test should be sufficient.

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.

Description

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


Description

  • Added a new MockWebDriver class to facilitate testing without a real browser driver.
  • Implemented tests for MockWebDriver constructor using WireMock to mock server responses.
  • Included the new test project WebDriver.Mock.Tests in the solution file.
  • Created project file for WebDriver.Mock.Tests with necessary package references.

Changes walkthrough 📝

Relevant files
Enhancement
GlobalUsings.cs
Add global using directive for NUnit framework                     

dotnet/test/mocked/GlobalUsings.cs

  • Added global using directive for NUnit framework.
+1/-0     
MockWebDriver.cs
Implement MockWebDriver class for testing                               

dotnet/test/mocked/MockWebDriver.cs

  • Created MockWebDriver class inheriting from OpenQA.Selenium.WebDriver.
  • Implemented constructor to initialize with mocked server.
  • +12/-0   
    Tests
    WebDriverConstructorTests.cs
    Add tests for MockWebDriver constructor                                   

    dotnet/test/mocked/WebDriverConstructorTests.cs

  • Added tests for MockWebDriver constructor.
  • Included tests for session creation and error handling.
  • +91/-0   
    Configuration changes
    WebDriver.NET.sln
    Include MockWebDriver tests project in solution                   

    dotnet/WebDriver.NET.sln

    • Added WebDriver.Mock.Tests project to solution file.
    +6/-0     
    WebDriver.Mock.Tests.csproj
    Create project file for MockWebDriver tests                           

    dotnet/test/mocked/WebDriver.Mock.Tests.csproj

  • Created project file for WebDriver.Mock.Tests.
  • Added necessary package references for testing.
  • +28/-0   

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

    Currently tests are using real browser drivers, but we can use a library like wiremock to mock the driver responses
    and create tests that don't need a real driver
    @Indomitable Indomitable force-pushed the tests/selenium-dotnet-mocked-tests branch from 68a3add to 6efcb92 Compare July 10, 2024 20:55
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Inheritance Issue
    The MockWebDriver class incorrectly inherits from OpenQA.Selenium.WebDriver, which is not a class but an interface. This should be corrected to inherit from a concrete class that implements IWebDriver or directly implement IWebDriver itself.

    Exception Handling
    The test GivenCreateSessionThrowsShouldNotCallQuit does not verify if the WebDriverException thrown contains the expected message or details related to BadGateway. This could lead to false positives in testing if exceptions are not thrown for the expected reasons.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Refactor the test method to improve code structure and readability

    Refactor the test method ShouldCreateSessionWhenCreated to separate the setup,
    action, and assertion phases clearly. This improves readability and maintainability
    of the test code.

    dotnet/test/mocked/WebDriverConstructorTests.cs [30-43]

     public void ShouldCreateSessionWhenCreated()
     {
    +    // Arrange
         mockedServer.Given(Request.Create().WithPath("/session"))
             .RespondWith(Response.Create()
                 .WithHeader("Content-Type", "application/json")
                 .WithBodyAsJson(new { }));
         var capabilities = new RemoteSessionSettings();
     
    -    _ = new MockWebDriver(mockedServerUrl, capabilities);
    +    // Act
    +    var driver = new MockWebDriver(mockedServerUrl, capabilities);
     
    +    // Assert
         mockedServer.Should().HaveReceivedACall()
             .UsingPost()
             .And.AtUrl($"{mockedServerUrl}/session");
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves the readability and maintainability of the test by clearly separating the arrange, act, and assert phases. This is a good practice for writing clean and understandable test code.

    7
    Best practice
    Improve the readability and consistency of assertions by using lambda expressions

    Use Assert.That with a lambda expression for assertions in
    WhenCreateSessionCreatedSetSessionIdAndCapabilities to make the assertions more
    readable and consistent with NUnit best practices.

    dotnet/test/mocked/WebDriverConstructorTests.cs [67-69]

    -Assert.That(driver.SessionId.ToString(), Is.EqualTo("1-2-3"));
    -Assert.That(driver.Capabilities["browser"], Is.EqualTo("firefox"));
    -Assert.That(driver.Capabilities["platform"], Is.EqualTo("linux"));
    +Assert.That(() => driver.SessionId.ToString(), Is.EqualTo("1-2-3"));
    +Assert.That(() => driver.Capabilities["browser"], Is.EqualTo("firefox"));
    +Assert.That(() => driver.Capabilities["platform"], Is.EqualTo("linux"));
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using lambda expressions in assertions can improve readability and consistency with NUnit best practices. However, the improvement is minor and does not significantly impact the functionality or readability of the existing assertions.

    6
    Add error handling in the test to manage WebDriver creation failures more gracefully

    In the test GivenCreateSessionThrowsShouldNotCallQuit, ensure that the session
    creation failure is handled gracefully by adding a try-catch block around the
    WebDriver creation.

    dotnet/test/mocked/WebDriverConstructorTests.cs [86]

    -Assert.Throws<WebDriverException>(() => { _ = new MockWebDriver(mockedServerUrl, capabilities); });
    +try
    +{
    +    _ = new MockWebDriver(mockedServerUrl, capabilities);
    +}
    +catch (WebDriverException ex)
    +{
    +    // Assert exception type or message if needed
    +    Assert.Throws<WebDriverException>(() => throw ex);
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 3

    Why: The suggestion introduces a try-catch block in the test, which is generally not recommended as tests should fail fast and clearly. The existing code already handles the exception using Assert.Throws, which is the correct approach.

    3
    Enhancement
    Improve error handling in the constructor by catching and re-throwing a more specific exception

    Consider using a more specific exception type in the constructor of MockWebDriver to
    handle potential errors during the initialization of the base class. This can help
    in better error handling and debugging.

    dotnet/test/mocked/MockWebDriver.cs [8-10]

     public MockWebDriver(string remoteAddress, ICapabilities capabilities)
    -    : base(new HttpCommandExecutor(new Uri(remoteAddress), TimeSpan.FromSeconds(1)), capabilities)
     {
    +    try
    +    {
    +        base(new HttpCommandExecutor(new Uri(remoteAddress), TimeSpan.FromSeconds(1)), capabilities);
    +    }
    +    catch (SpecificException ex)
    +    {
    +        // Handle or log the exception
    +        throw new WebDriverInitializationException("Initialization failed.", ex);
    +    }
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: The suggestion to catch and re-throw a specific exception can improve error handling, but the proposed change incorrectly modifies the constructor syntax and introduces a new exception type without context. This reduces its effectiveness and correctness.

    4

    @nvborisenko
    Copy link
    Member

    Real browser or mocked browser?!.. This is conceptual question to the @SeleniumHQ/selenium-committers group. From my point of view, if we have an ability to test our code on real browser, let's use it. I can image some edge cases when we want to change the behavior of the browser, and it becomes tricky with real browsers. But from another hand introducing faked browser will introduce much more maintenance effort.

    @Indomitable
    Copy link
    Contributor Author

    I have no problem with real browsers. My problem is actually that in most of the cases they will work correctly and this way we only test positive path. But what if an endpoint returns a malformed response, a status code which was not expected. By mocking the the responses we can easily test the non happy path, check that driver doesn't throw unhandled exceprions, but a one with good message to the client.

    @github-actions github-actions bot added the Stale label Jan 9, 2025
    @github-actions github-actions bot closed this Jan 24, 2025
    @joerg1985
    Copy link
    Member

    But what if an endpoint returns a malformed response, a status code which was not expected. By mocking the the responses we can easily test the non happy path, check that driver doesn't throw unhandled exceprions, but a one with good message to the client.

    At least in the java tests the http responses are mocked and injected, to simulate these failures.

    HttpResponse response = new HttpResponse();
    response.setStatus(HTTP_OK);
    response.setContent(
    utf8String("{\"value\": {\"sessionId\": \"23456789\", \"capabilities\": {}}}"));
    RecordingHttpClient client = new RecordingHttpClient(response);

    Or more high level a decoded json object with a invalid response:

    Map<String, ?> payload =
    ImmutableMap.of("value", caps.asMap(), "sessionId", "cheese is opaque");
    InitialHandshakeResponse initialResponse = new InitialHandshakeResponse(0, 200, payload);

    So there is no mock for the driver it self, but we are able to write such tests.

    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