Skip to content

[dotnet] [test] Start test webserver globally before any tests in Support#17275

Merged
nvborisenko merged 2 commits intoSeleniumHQ:trunkfrom
nvborisenko:dotnet-ci-test-fix
Mar 30, 2026
Merged

[dotnet] [test] Start test webserver globally before any tests in Support#17275
nvborisenko merged 2 commits intoSeleniumHQ:trunkfrom
nvborisenko:dotnet-ci-test-fix

Conversation

@nvborisenko
Copy link
Copy Markdown
Member

Hopefully fixes CI tests.

💥 What does this PR do?

This pull request refactors the setup and teardown logic for starting and stopping the web server and remote server in the test suite. The main improvement is the introduction of a shared AssemblyFixture class to handle this functionality at the assembly level, which removes redundant setup/teardown code from individual test classes.

Test infrastructure improvements:

  • Added a new AssemblyFixture class in dotnet/test/support/AssemblyFixture.cs that uses NUnit's [SetUpFixture] to start and stop the web server (and remote server if needed) once for all tests, ensuring consistent and efficient test environment setup and cleanup.
  • Removed duplicated [OneTimeSetUp] and [OneTimeTearDown] methods from PopupWindowFinderTests and SelectBrowserTests, as this logic is now centralized in AssemblyFixture. [1] [2]

🔄 Types of changes

  • Cleanup (formatting, renaming)

@selenium-ci selenium-ci added C-dotnet .NET Bindings B-support Issue or PR related to support classes labels Mar 30, 2026
@qodo-code-review
Copy link
Copy Markdown
Contributor

Review Summary by Qodo

Centralize test web server setup in assembly-level fixture

🧪 Tests

Grey Divider

Walkthroughs

Description
• Centralize web server setup/teardown in assembly-level fixture
• Remove duplicate server initialization from individual test classes
• Ensure consistent test environment across all Support project tests

Grey Divider

File Changes

1. dotnet/test/support/AssemblyFixture.cs 🧪 Tests +55/-0

New assembly-level fixture for global test setup

• Created new AssemblyFixture class with [SetUpFixture] attribute for assembly-level test setup
• Implements RunBeforeAnyTestAsync() to start web server and remote server (if needed) before all
 tests
• Implements RunAfterAnyTestsAsync() to stop servers and close driver after all tests complete
• Sets logging level to Trace for test diagnostics

dotnet/test/support/AssemblyFixture.cs


2. dotnet/test/support/UI/PopupWindowFinderTests.cs Cleanup +0/-14

Remove duplicate server setup from PopupWindowFinderTests

• Removed duplicate [OneTimeSetUp] and [OneTimeTearDown] methods
• Removed TODO comment about moving server setup to standalone class
• Removed unused using System.Threading.Tasks import
• Server setup now handled by centralized AssemblyFixture

dotnet/test/support/UI/PopupWindowFinderTests.cs


3. dotnet/test/support/UI/SelectBrowserTests.cs Cleanup +0/-15

Remove duplicate server setup from SelectBrowserTests

• Removed duplicate [OneTimeSetUp] and [OneTimeTearDown] methods
• Removed unused using System.Threading.Tasks import
• Removed unused using OpenQA.Selenium.Environment import
• Server setup now handled by centralized AssemblyFixture

dotnet/test/support/UI/SelectBrowserTests.cs


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 Mar 30, 2026

Code Review by Qodo

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

Grey Divider


Action required

1. Webserver leak on setup failure 🐞 Bug ⛯ Reliability
Description
AssemblyFixture.RunBeforeAnyTestAsync starts the web server and then starts the remote server; if
remote server startup throws, the web server is never stopped on that failure path. This can leave a
stray appserver process running (holding resources/ports) until finalization or process exit,
creating CI flakiness and interference with subsequent runs.
Code

dotnet/test/support/AssemblyFixture.cs[R34-42]

+    public async Task RunBeforeAnyTestAsync()
+    {
+        Internal.Logging.Log.SetLevel(Internal.Logging.LogEventLevel.Trace);
+
+        await EnvironmentManager.Instance.WebServer.StartAsync();
+        if (EnvironmentManager.Instance.Browser == Browser.Remote)
+        {
+            await EnvironmentManager.Instance.RemoteServer.StartAsync();
+        }
Evidence
The setup method starts the web server and then awaits remote server startup without any
try/catch/finally, so exceptions after the first await skip cleanup. TestWebServer.StartAsync
launches a child process and can throw a TimeoutException without killing/disposing that process.
While EnvironmentManager has a finalizer that attempts to stop servers, finalizers are not
deterministic and may not run before the test host terminates.

dotnet/test/support/AssemblyFixture.cs[33-43]
dotnet/test/common/Environment/TestWebServer.cs[39-128]
dotnet/test/common/Environment/RemoteSeleniumServer.cs[57-125]
dotnet/test/common/Environment/EnvironmentManager.cs[197-202]

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

### Issue description
`dotnet/test/support/AssemblyFixture.cs` starts the web server and then the remote server. If the second step throws (or if `WebServer.StartAsync()` itself throws after starting its process), there is no deterministic cleanup on that failure path.

### Issue Context
`TestWebServer.StartAsync()` starts a child process and throws on timeout without killing/disposing that process. The fixture should ensure resources started during setup are stopped even when later setup steps fail.

### Fix Focus Areas
- dotnet/test/support/AssemblyFixture.cs[33-54]

### Proposed fix
- Wrap startup in `try`/`catch` (or `try`/`finally`) and track what successfully started.
- If `RemoteServer.StartAsync()` fails after the web server started, call `await EnvironmentManager.Instance.WebServer.StopAsync()` before rethrowing.
- Optionally, in teardown, stop remote server in a `finally` so it still attempts to stop even if `WebServer.StopAsync()` fails, and consider stopping remote before web for symmetry:
 - `try { if (remote) await RemoteServer.StopAsync(); } finally { await WebServer.StopAsync(); }`
- Keep the teardown robust to partial startup (only stop what was started).

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


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@nvborisenko nvborisenko merged commit 1cf03a9 into SeleniumHQ:trunk Mar 30, 2026
18 of 19 checks passed
@nvborisenko nvborisenko deleted the dotnet-ci-test-fix branch March 30, 2026 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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