[dotnet] Modernize code patterns in test suites

[RenderMichael]

User description

• Apply visibility modifiers to types
• Avoid commented tests, ignore instead

🔗 Related Issues

💥 What does this PR do?

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

• Cleanup (formatting, renaming)

---

PR Type

Enhancement

---

Description

• Apply visibility modifiers to test class declarations

• Replace commented-out tests with [Ignore] attributes

• Modernize C# patterns: primary constructors, property expressions

• Improve code quality with pattern matching and readonly fields

---

Diagram Walkthrough

flowchart LR
  A["Test Classes"] -->|Add internal modifier| B["Visibility Applied"]
  C["Commented Tests"] -->|Replace with Ignore| D["Proper Test Attributes"]
  E["Old Constructors"] -->|Convert to primary| F["Modern C# Syntax"]
  G["Properties"] -->|Use expressions| H["Concise Code"]
  I["Type Checks"] -->|Use pattern matching| J["Cleaner Logic"]

Loading

File Walkthrough

	Relevant files
Code modernization	55 files BrowserTest.cs Add internal visibility modifier to test class                      +1/-1      BrowsingContextEventsTest.cs Add internal visibility modifier to test class                      +1/-1      BrowsingContextTest.cs Add internal visibility modifier to test class                      +1/-1      EmulationTest.cs Add internal visibility modifier to test class                      +1/-1      CombinedInputActionsTest.cs Add internal visibility modifier to test class                      +1/-1      SetFilesTest.cs Add internal modifier and readonly to field                            +2/-2      LogTest.cs Add internal visibility modifier to test class                      +1/-1      NetworkEventsTest.cs Add internal visibility modifier to test class                      +1/-1      NetworkTest.cs Add internal visibility modifier to test class                      +1/-1      CallFunctionLocalValueTest.cs Add internal visibility modifier to test class                      +1/-1      CallFunctionParameterTest.cs Add internal visibility modifier to test class                      +1/-1      EvaluateParametersTest.cs Add internal visibility modifier to test class                      +1/-1      LocalValueConversionTests.cs Add internal visibility modifier to test class                      +1/-1      ScriptCommandsTest.cs Add internal visibility modifier to test class                      +1/-1      ScriptEventsTest.cs Add internal visibility modifier to test class                      +1/-1      StorageTest.cs Add internal visibility modifier to test class                      +1/-1      WebExtensionTest.cs Add internal modifier and readonly to constant                      +2/-2      CookieImplementationTest.cs Add readonly to field and use pattern matching                      +3/-4      DevChannelChromeDriver.cs Convert property getter to expression body                              +1/-4      DevChannelEdgeDriver.cs Convert property getter to expression body                              +1/-4      EdgeInternetExplorerModeDriver.cs Convert property getter to expression body                              +1/-4      NightlyChannelFirefoxDriver.cs Convert property getter to expression body                              +1/-4      StableChannelChromeDriver.cs Convert property getter to expression body                              +1/-4      StableChannelEdgeDriver.cs Convert property getter to expression body                              +1/-4      StableChannelFirefoxDriver.cs Convert property getter to expression body                              +1/-4      IgnoreBrowserAttribute.cs Modernize with primary constructor and property expressions +9/-32    IgnorePlatformAttribute.cs Modernize with primary constructor and property expressions +12/-38  IgnoreTargetAttribute.cs Modernize with primary constructor and property expressions +2/-7      DevToolsSecurityTest.cs Replace commented test with Ignore attribute                          +1/-1      DevToolsTargetTest.cs Change field to const and add visibility                                  +1/-1      DevToolsTestFixture.cs Convert property to expression with MemberNotNullWhen        +3/-4      ElementSelectingTest.cs Convert properties to expression bodies                                    +10/-70  DriverFactory.cs Add readonly modifiers to fields                                                  +4/-4      DriverStartingEventArgs.cs Convert to primary constructor with readonly properties    +3/-12    InlinePage.cs Add readonly modifiers to collection fields                            +3/-3      RemoteSeleniumServer.cs Convert to primary constructor with nullable reference      +8/-14    TestEnvironment.cs Add internal visibility modifier to class                                +1/-1      TestWebServer.cs Convert to primary constructor with field initialization  +7/-16    UrlBuilder.cs Convert fields to readonly properties and modernize            +19/-31  ExecutingJavascriptTest.cs Use pattern matching for type checking                                      +1/-2      GetLogsTest.cs Replace commented tests with Ignore attributes                      +4/-3      I18Test.cs Add readonly modifiers to string fields                                    +3/-3      BasicKeyboardInterfaceTest.cs Use pattern matching for type checking                                      +4/-8      BasicMouseInterfaceTest.cs Use pattern matching for type checking                                      +1/-2      BasicWheelInterfaceTest.cs Use pattern matching for type checking                                      +1/-2      CombinedInputActionsTest.cs Use pattern matching for type checking                                      +2/-4      DragAndDropTest.cs Use pattern matching and remove utility method                      +2/-3      LogTest.cs Add internal visibility modifier to test handler                  +1/-1      ProxySettingTest.cs Modernize with pattern matching and readonly fields            +43/-52  TakesScreenshotTest.cs Use pattern matching for type checking                                      +9/-18    TextPagesTest.cs Add readonly modifier to field                                                      +1/-1      TimeoutDriverOptionsTest.cs Add private visibility modifier to test class                        +1/-1      WebElementWrapper.cs Convert to primary constructor with expression bodies        +20/-27  FirefoxDriverTest.cs Replace commented tests with Ignore attributes                      +20/-10  IeSpecificTests.cs Replace commented tests with Ignore attributes                      +15/-14
Enhancement	1 files DriverTestFixture.cs Refactor WaitFor methods to use WebDriverWait                        +16/-39
Code cleanup	1 files TestUtilities.cs Remove IsNativeEventsEnabled utility method                            +0/-15
Formatting	1 files SelectBrowserTests.cs Fix comment formatting for commented test                                +1/-1

---

[selenium-ci]

Thank you, @RenderMichael 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]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Command execution risk Description: The code starts an external process for a web server using dynamic arguments and captures output without sandboxing or validation, which could enable command injection or unintended execution if projectRoot or arguments are influenced externally. TestWebServer.cs [68-126] Referred Code processArguments = $"run //java/test/org/openqa/selenium/environment:appserver {processArguments}"; // Override project root path to be exact selenium repo path, not 'bazel-bin' projectRoot = Path.Combine(AppContext.BaseDirectory, "../../../../../.."); } webserverProcess = new Process(); webserverProcess.StartInfo.FileName = processFileName; webserverProcess.StartInfo.Arguments = processArguments; webserverProcess.StartInfo.WorkingDirectory = projectRoot; webserverProcess.StartInfo.UseShellExecute = !(hideCommandPrompt || captureWebServerOutput); webserverProcess.StartInfo.CreateNoWindow = hideCommandPrompt; captureWebServerOutput = true; if (captureWebServerOutput) { webserverProcess.StartInfo.RedirectStandardOutput = true; webserverProcess.StartInfo.RedirectStandardError = true; } ... (clipped 38 lines)
	Unvalidated process launch Description: Launching java with a JAR path derived from projectRoot without validating existence beyond a simple check may allow path manipulation if projectRoot is user-controlled, leading to execution of unintended binaries. RemoteSeleniumServer.cs [50-87] Referred Code webserverProcess = new Process(); webserverProcess.StartInfo.FileName = "java.exe"; webserverProcess.StartInfo.Arguments = " -jar " + serverJarName + " standalone --port 6000 --selenium-manager true --enable-managed-downloads true"; webserverProcess.StartInfo.WorkingDirectory = projectRoot; webserverProcess.Start(); DateTime timeout = DateTime.Now.Add(TimeSpan.FromSeconds(30)); bool isRunning = false; // Poll until the webserver is correctly serving pages. using var httpClient = new HttpClient(); while (!isRunning && DateTime.Now < timeout) { try { using var response = await httpClient.GetAsync("http://localhost:6000/wd/hub/status"); if (response.StatusCode == HttpStatusCode.OK) { isRunning = true; } ... (clipped 17 lines)
Ticket Compliance
🟡	🎫 #1234 🔴 Investigate and ensure Firefox triggers JavaScript in link href on click() as it did in 2.47.1, which regressed in 2.48.x. Provide a fix or tests demonstrating correct alert/JS execution behavior when clicking JS href links in Firefox 42 scenario.
🟡	🎫 #5678 🔴 Address "Error: ConnectFailure (Connection refused)" when instantiating multiple ChromeDriver instances on Ubuntu 16.04 with Chrome 65/Chromedriver 2.35, Selenium 3.9.0. Provide stability improvements or guidance/tests ensuring multiple ChromeDriver instances can be created without connection failures after the first instance.
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing Audit Logs: The new code starts external processes for a test web server but adds no audit logging of start/stop actions or outcomes, which may be acceptable for test code but cannot be confirmed from the diff alone. Referred Code processArguments = $"run //java/test/org/openqa/selenium/environment:appserver {processArguments}"; // Override project root path to be exact selenium repo path, not 'bazel-bin' projectRoot = Path.Combine(AppContext.BaseDirectory, "../../../../../.."); } webserverProcess = new Process(); webserverProcess.StartInfo.FileName = processFileName; webserverProcess.StartInfo.Arguments = processArguments; webserverProcess.StartInfo.WorkingDirectory = projectRoot; webserverProcess.StartInfo.UseShellExecute = !(hideCommandPrompt || captureWebServerOutput); webserverProcess.StartInfo.CreateNoWindow = hideCommandPrompt; captureWebServerOutput = true; if (captureWebServerOutput) { webserverProcess.StartInfo.RedirectStandardOutput = true; webserverProcess.StartInfo.RedirectStandardError = true; } ... (clipped 38 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Limited Error Handling: Process start and stop flows lack explicit exception handling and retries in the new code paths (e.g., when the server JAR is missing or HTTP stop request fails), which may be sufficient for tests but is unclear from the diff. Referred Code public class RemoteSeleniumServer(string projectRoot, bool autoStartServer) { private Process? webserverProcess; private string serverJarName = @"java/src/org/openqa/selenium/grid/selenium_server_deploy.jar"; public async Task StartAsync() { if (autoStartServer && (webserverProcess == null || webserverProcess.HasExited)) { serverJarName = serverJarName.Replace('/', Path.DirectorySeparatorChar); if (!File.Exists(Path.Combine(projectRoot, serverJarName))) { throw new FileNotFoundException( string.Format( "Selenium server jar at {0} didn't exist - please build it using something like {1}", serverJarName, "go //java/src/org/openqa/grid/selenium:selenium")); } webserverProcess = new Process(); webserverProcess.StartInfo.FileName = "java.exe"; ... (clipped 36 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Verbose Error Detail: The timeout error message includes working directory, full process arguments, stdout, and stderr which can expose internal details; acceptable in tests but warrants confirmation. Referred Code output = webserverProcess.StandardOutput.ReadToEnd(); } string errorMessage = string.Format("Could not start the test web server in {0} seconds.\nWorking directory: {1}\nProcess Args: {2}\nstdout: {3}\nstderr: {4}", timeout.TotalSeconds, projectRoot, processArguments, output, error); Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Input Validation: The HTTP listener in the test PAC server processes requests based on URL content without explicit sanitization or validation; this is likely safe in test scope but cannot be fully assessed from the diff. Referred Code private void ProcessContext(HttpListenerContext context) { if (context.Request.Url.AbsoluteUri.Contains("proxy.pac", StringComparison.InvariantCultureIgnoreCase)) { byte[] pacFileBuffer = Encoding.ASCII.GetBytes(this.pacFileContent); context.Response.ContentType = "application/x-javascript-config"; Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Learned best practice	Ensure proper thread and listener cleanup Ensure the listener thread is stopped and joined, and dispose HttpListener/streams in finally blocks to avoid leaked resources if exceptions occur. dotnet/test/common/ProxySettingTest.cs [183-280] -private class ProxyAutoConfigServer : IDisposable +private Thread StartListeningThread() { - private readonly string pacFileContent; - private bool disposedValue = false; // To detect redundant calls - private bool keepRunning = true; - private HttpListener listener; - private readonly Thread listenerThread; + var thread = new Thread(Listen) { IsBackground = true }; + thread.Start(); + return thread; +} - public ProxyAutoConfigServer(string pacFileContent) - : this(pacFileContent, "localhost") +protected virtual void Dispose(bool disposing) +{ + if (disposedValue) return; + + keepRunning = false; + + try { + listener?.Stop(); + listener?.Close(); + } + catch (ObjectDisposedException) + { + // no op } - public ProxyAutoConfigServer(string pacFileContent, string hostName) + if (disposing) { - this.pacFileContent = pacFileContent; - HostName = hostName; - Port = DetectEmptyPort(); - this.listenerThread = StartListeningThread(); + // join listener thread to ensure cleanup completes + try + { + if (listenerThread.IsAlive) + { + listenerThread.Join(TimeSpan.FromSeconds(2)); + } + } + catch { /* best effort */ } } - public string HostName { get; } + disposedValue = true; +} - public int Port { get; } - - private static int DetectEmptyPort() - { - TcpListener l = new TcpListener(IPAddress.Loopback, 0); - l.Start(); - var port = ((IPEndPoint)l.LocalEndpoint).Port; - l.Stop(); - return port; - } - - private Thread StartListeningThread() - { - var thread = new Thread(Listen); - thread.Start(); - return thread; - } - - public void Dispose() - { - Dispose(true); - GC.SuppressFinalize(this); - } - - protected virtual void Dispose(bool disposing) - { - if (!disposedValue) - { - if (disposing) - { - // TODO: dispose managed state (managed objects) - } - - // TODO: free unmanaged resources (unmanaged objects) and override finalizer - // TODO: set large fields to null - keepRunning = false; - - if (listener != null) - { - try - { - listener.Stop(); - listener.Close(); - } - catch (ObjectDisposedException) - { - // no op - } - } - - disposedValue = true; - } - } - - ~ProxyAutoConfigServer() - { - Dispose(false); - } - - private void ProcessContext(HttpListenerContext context) +private void ProcessContext(HttpListenerContext context) +{ + try { if (context.Request.Url.AbsoluteUri.Contains("proxy.pac", StringComparison.InvariantCultureIgnoreCase)) { byte[] pacFileBuffer = Encoding.ASCII.GetBytes(this.pacFileContent); context.Response.ContentType = "application/x-javascript-config"; context.Response.ContentLength64 = pacFileBuffer.Length; - var output = context.Response.OutputStream; + using var output = context.Response.OutputStream; output.Write(pacFileBuffer, 0, pacFileBuffer.Length); output.Flush(); - output.Close(); } else { context.Response.StatusCode = 404; context.Response.StatusDescription = "Not Found"; context.Response.OutputStream.Close(); } } - - private void Listen() + finally { - listener = new HttpListener(); - listener.Prefixes.Add("http://" + this.HostName + ":" + Port.ToString() + "/"); - listener.Start(); - while (this.keepRunning) - { - HttpListenerContext context = null; - try - { - context = listener.GetContext(); - } - catch (HttpListenerException) - { - // no op - } - - if (context != null) - { - ProcessContext(context); - } - } + try { context.Response.OutputStream?.Close(); } catch { } } } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Pattern 1: Wrap creation of external resources in try/finally and perform explicit cleanup to avoid leaks.	Low
High-level	Replace commented-out tests with [Ignore] Replace remaining commented-out [Test] attributes with [Test, Ignore("reason")] to fully align with the PR's stated goal of avoiding commented tests. Examples: dotnet/test/common/BiDi/Input/CombinedInputActionsTest.cs [28-29] //[Test] public async Task Paint() dotnet/test/support/UI/SelectBrowserTests.cs [164] Solution Walkthrough: Before: // file: dotnet/test/common/BiDi/Input/CombinedInputActionsTest.cs internal class CombinedInputActionsTest : BiDiTestFixture { //[Test] public async Task Paint() { driver.Url = "https://kleki.com/"; // ... } } // file: dotnet/test/support/UI/SelectBrowserTests.cs public class SelectBrowserTests : DriverTestFixture { // ... //[Test] // [ExpectedException(typeof(NoSuchElementException))] public void ShouldThrowAnExceptionIfThereAreNoSelectedOptions() // ... } After: // file: dotnet/test/common/BiDi/Input/CombinedInputActionsTest.cs internal class CombinedInputActionsTest : BiDiTestFixture { [Test, Ignore("Test is incomplete or flaky")] public async Task Paint() { driver.Url = "https://kleki.com/"; // ... } } // file: dotnet/test/support/UI/SelectBrowserTests.cs public class SelectBrowserTests : DriverTestFixture { // ... [Test, Ignore("The .NET bindings do not have a FirstSelectedOption property")] // [ExpectedException(typeof(NoSuchElementException))] public void ShouldThrowAnExceptionIfThereAreNoSelectedOptions() // ... } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly points out that the PR's stated goal of replacing commented-out tests is incomplete, as it missed several instances, which affects code consistency.	Low
General	Avoid overwriting test ignore reason Add a break statement after a test is marked as ignored to prevent the SkipReason from being overwritten by subsequent attributes. dotnet/test/common/CustomTestAttributes/IgnoreBrowserAttribute.cs [42-71] public void ApplyToTest(Test test) { if (test.RunState != RunState.NotRunnable) { IgnoreBrowserAttribute[] ignoreAttributes; if (test.IsSuite) { ignoreAttributes = test.TypeInfo.GetCustomAttributes<IgnoreBrowserAttribute>(true); } else { ignoreAttributes = test.Method.GetCustomAttributes<IgnoreBrowserAttribute>(true); } foreach (IgnoreBrowserAttribute browserToIgnoreAttr in ignoreAttributes) { if (browserToIgnoreAttr != null && IgnoreTestForBrowser(browserToIgnoreAttr.Value)) { string ignoreReason = "Ignoring browser " + EnvironmentManager.Instance.Browser.ToString() + "."; if (!string.IsNullOrEmpty(browserToIgnoreAttr.Reason)) { ignoreReason = ignoreReason + " " + browserToIgnoreAttr.Reason; } test.RunState = RunState.Ignored; test.Properties.Set(PropertyNames.SkipReason, ignoreReason); + break; } } } } Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that multiple matching ignore attributes would overwrite the skip reason, and proposes adding a break to fix it, which improves robustness.	Low
Update