Skip to content

[dotnet] [test] Add NUnit analyzer#17522

Merged
nvborisenko merged 9 commits into
SeleniumHQ:trunkfrom
nvborisenko:dotnet-nunit-analyzer
May 19, 2026
Merged

[dotnet] [test] Add NUnit analyzer#17522
nvborisenko merged 9 commits into
SeleniumHQ:trunkfrom
nvborisenko:dotnet-nunit-analyzer

Conversation

@nvborisenko
Copy link
Copy Markdown
Member

  • And fixed errors/warnings

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s):
    • What was generated:
    • I reviewed all AI output and can explain the change

🔄 Types of changes

  • Cleanup (formatting, renaming)

@selenium-ci selenium-ci added C-dotnet .NET Bindings B-build Includes scripting, bazel and CI integrations B-support Issue or PR related to support classes labels May 19, 2026
@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Add NUnit analyzer and fix test code issues

🐞 Bug fix 🧪 Tests

Grey Divider

Walkthroughs

Description
• Add NUnit.Analyzers dependency to test projects
• Fix resource disposal using Dispose instead of Quit
• Make test fixture classes abstract to prevent instantiation
• Fix test assertions to use correct property access patterns
• Refactor test code to extract variables for clarity
Diagram
flowchart LR
  A["NUnit.Analyzers<br/>Dependency"] --> B["Test Projects<br/>Updated"]
  C["Resource Disposal<br/>Quit → Dispose"] --> D["Test Cleanup<br/>Improved"]
  E["Abstract Fixtures<br/>BiDiTestFixture<br/>DevToolsTestFixture"] --> F["Inheritance<br/>Enforced"]
  G["Assertion Fixes<br/>Property Access<br/>Variable Extraction"] --> H["Test Code<br/>Corrected"]
Loading

Grey Divider

File Changes

1. dotnet/paket.dependencies Dependencies +1/-0

Add NUnit.Analyzers package dependency

dotnet/paket.dependencies


2. dotnet/paket.nuget.bzl ⚙️ Configuration changes +1/-0

Add NUnit.Analyzers to Bazel configuration

dotnet/paket.nuget.bzl


3. dotnet/test/support/Events/EventFiringWebDriverTests.cs Error handling +6/-0

Add TearDown method for resource cleanup

dotnet/test/support/Events/EventFiringWebDriverTests.cs


View more (18)
4. dotnet/test/webdriver/BiDi/BiDiFixture.cs 🐞 Bug fix +1/-1

Make BiDiTestFixture abstract class

dotnet/test/webdriver/BiDi/BiDiFixture.cs


5. dotnet/test/webdriver/BiDi/BrowsingContext/BrowsingContextTests.cs 🐞 Bug fix +2/-2

Fix assertion to use correct Context property

dotnet/test/webdriver/BiDi/BrowsingContext/BrowsingContextTests.cs


6. dotnet/test/webdriver/BiDi/Session/SessionTests.cs 🧪 Tests +3/-1

Extract module variable for assertion clarity

dotnet/test/webdriver/BiDi/Session/SessionTests.cs


7. dotnet/test/webdriver/DevTools/DevToolsDomainsTests.cs 🧪 Tests +9/-4

Extract domain variables for assertion clarity

dotnet/test/webdriver/DevTools/DevToolsDomainsTests.cs


8. dotnet/test/webdriver/DevTools/DevToolsTargetTests.cs 🐞 Bug fix +1/-1

Fix assertion to check for non-zero value

dotnet/test/webdriver/DevTools/DevToolsTargetTests.cs


9. dotnet/test/webdriver/DevTools/DevToolsTestFixture.cs 🐞 Bug fix +1/-1

Make DevToolsTestFixture abstract class

dotnet/test/webdriver/DevTools/DevToolsTestFixture.cs


10. dotnet/test/webdriver/DownloadsTests.cs 🐞 Bug fix +1/-1

Replace Quit with Dispose for cleanup

dotnet/test/webdriver/DownloadsTests.cs


11. dotnet/test/webdriver/DriverTestFixture.cs Error handling +7/-0

Add OneTimeTearDown and improve cleanup logic

dotnet/test/webdriver/DriverTestFixture.cs


12. dotnet/test/webdriver/PageLoadingTests.cs 🐞 Bug fix +1/-1

Replace Quit with Dispose for cleanup

dotnet/test/webdriver/PageLoadingTests.cs


13. dotnet/test/webdriver/ProxySettingTests.cs 🐞 Bug fix +1/-1

Replace Quit with Dispose for cleanup

dotnet/test/webdriver/ProxySettingTests.cs


14. dotnet/test/webdriver/TimeoutDriverOptionsTests.cs 🐞 Bug fix +1/-1

Replace Quit with Dispose for cleanup

dotnet/test/webdriver/TimeoutDriverOptionsTests.cs


15. dotnet/test/webdriver/UnexpectedAlertBehaviorTests.cs 🐞 Bug fix +1/-1

Replace Quit with Dispose for cleanup

dotnet/test/webdriver/UnexpectedAlertBehaviorTests.cs


16. dotnet/test/remote/BUILD.bazel ⚙️ Configuration changes +1/-0

Add NUnit.Analyzers to remote test dependencies

dotnet/test/remote/BUILD.bazel


17. dotnet/test/remote/Selenium.WebDriver.Remote.Tests.csproj Dependencies +1/-0

Add NUnit.Analyzers package reference

dotnet/test/remote/Selenium.WebDriver.Remote.Tests.csproj


18. dotnet/test/support/BUILD.bazel ⚙️ Configuration changes +1/-0

Add NUnit.Analyzers to support test dependencies

dotnet/test/support/BUILD.bazel


19. dotnet/test/support/Selenium.Support.Tests.csproj Dependencies +1/-0

Add NUnit.Analyzers package reference

dotnet/test/support/Selenium.Support.Tests.csproj


20. dotnet/test/webdriver/BUILD.bazel ⚙️ Configuration changes +1/-0

Add NUnit.Analyzers to webdriver test dependencies

dotnet/test/webdriver/BUILD.bazel


21. dotnet/test/webdriver/Selenium.WebDriver.Tests.csproj Dependencies +1/-0

Add NUnit.Analyzers package reference

dotnet/test/webdriver/Selenium.WebDriver.Tests.csproj


Grey Divider

Qodo Logo

@selenium-ci
Copy link
Copy Markdown
Member

Thank you, @nvborisenko for this code suggestion.

The support packages contain example code that many users find helpful, but they do not necessarily represent
the best practices for using Selenium, and the Selenium team is not currently merging changes to them.

After reviewing the change, unless it is a critical fix or a feature that is needed for Selenium
to work, we will likely close the PR.

We actively encourage people to add the wrapper and helper code that makes sense for them to their own frameworks.
If you have any questions, please contact us

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 19, 2026

Persistent review updated to latest commit 28ea17f

Comment thread dotnet/test/webdriver/DriverTestFixture.cs
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 19, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Action required

1. Shared driver disposed unsafely ✓ Resolved 🐞 Bug ☼ Reliability
Description
DriverTestFixture disposes the shared EnvironmentManager-held driver without clearing
EnvironmentManager's internal driver reference, so later GetCurrentDriver() calls can return a
disposed driver. ResetOnError() also disposes before calling CreateFreshDriver(), which then calls
CloseCurrentDriver()/Quit on an already-disposed instance and can throw, masking the real test
failure.
Code

dotnet/test/webdriver/DriverTestFixture.cs[R134-147]

+    [OneTimeTearDown]
+    public void TearDown()
+    {
+        driver?.Dispose();
+    }
+
    [TearDown]
    public void ResetOnError()
    {
        if (TestContext.CurrentContext.Result.Outcome == Error)
        {
+            driver?.Dispose();
            driver = EnvironmentManager.Instance.CreateFreshDriver();
        }
Evidence
The fixture now disposes driver directly, but EnvironmentManager caches the current driver and
only CloseCurrentDriver() both quits and nulls that cache; disposing without clearing the cache
allows GetCurrentDriver() to return a disposed instance and also sets up CloseCurrentDriver() to
call Quit() on a disposed driver later.

dotnet/test/webdriver/DriverTestFixture.cs[128-148]
dotnet/test/webdriver/Infrastructure/Environment/EnvironmentManager.cs[174-200]
dotnet/test/webdriver/AssemblyFixture.cs[44-52]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`DriverTestFixture` currently calls `driver.Dispose()` in `[OneTimeTearDown]` and in `ResetOnError()`. Because the driver is owned/tracked by `EnvironmentManager` (singleton), disposing the driver directly can leave `EnvironmentManager` holding a disposed instance and cause later calls to reuse or quit a disposed driver.

### Issue Context
- `EnvironmentManager.GetCurrentDriver()` returns the cached `driver` field if non-null.
- `EnvironmentManager.CloseCurrentDriver()` is the method that both quits the driver and clears the cached reference (`driver = null`).

### Fix Focus Areas
- dotnet/test/webdriver/DriverTestFixture.cs[128-148]

### Suggested fix
1. In `DriverTestFixture.TearDown()` (the `[OneTimeTearDown]`), replace `driver?.Dispose();` with `EnvironmentManager.Instance.CloseCurrentDriver();` (and optionally set `driver = null;` for clarity).
2. In `ResetOnError()`, remove the explicit `driver?.Dispose();` and just call `driver = EnvironmentManager.Instance.CreateFreshDriver();` (since `CreateFreshDriver()` already closes the current driver via `CloseCurrentDriver()`).
3. Ensure there is no remaining path where `EnvironmentManager.driver` can remain non-null while the underlying driver is disposed.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 19, 2026

Persistent review updated to latest commit 6c46f10

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review Bot commented May 19, 2026

Persistent review updated to latest commit 4fead34

@nvborisenko nvborisenko merged commit f5601cd into SeleniumHQ:trunk May 19, 2026
21 checks passed
@nvborisenko nvborisenko deleted the dotnet-nunit-analyzer branch May 19, 2026 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations B-support Issue or PR related to support classes C-dotnet .NET Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants