[dotnet] Modernize EnvironmentManager, standardize assembly teardown

[RenderMichael]

User description

Description

Modernizes some test code and standardizes assembly teardown, in preparation for testing infrastructure improvements.

Motivation and Context

Contributes to #15536

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

• Introduced centralized AssemblyTeardown for test setup and teardown.

  • Added AssemblyTeardown classes for Chrome, Edge, Firefox, IE, Safari, and Remote tests.
  • Unified test environment initialization and cleanup logic.

• Refactored EnvironmentManager for improved readability and immutability.

  • Converted mutable fields to readonly properties where applicable.
  • Simplified singleton implementation for EnvironmentManager.

• Modernized test utilities and attributes for better maintainability.

  • Replaced manual property definitions with auto-properties.
  • Simplified type checks and casting in NeedsFreshDriverAttribute and TestUtilities.

• Reactivated and updated previously commented-out Firefox tests.

  • Enabled and adjusted tests in FirefoxProfileManagerTest and FirefoxProfileTests.

---

Changes walkthrough 📝

	Relevant files
Enhancement	12 files AssemblyTeardown.cs Added centralized test setup and teardown for Chrome tests +41/-0    ChromeSpecificTests.cs Removed redundant teardown logic from Chrome tests              +0/-8      NeedsFreshDriverAttribute.cs Simplified property definitions and type checks                    +9/-19    DriverTestFixture.cs Refactored driver management logic and removed unused methods +6/-17    EnvironmentManager.cs Refactored `EnvironmentManager` for immutability and readability +18/-68  StubDriver.cs Simplified property definitions in `StubDriver`                    +7/-22    TestUtilities.cs Improved type casting and property handling in utilities  +5/-6      AssemblyTeardown.cs Added centralized test setup and teardown for Edge tests  +13/-15  AssemblyTeardown.cs Added centralized test setup and teardown for Firefox tests +13/-15  AssemblyTeardown.cs Added centralized test setup and teardown for IE tests      +13/-15  AssemblyTeardown.cs Added centralized test setup and teardown for Remote tests +18/-19  AssemblyTeardown.cs Added centralized test setup and teardown for Safari tests +41/-0
Tests	2 files FirefoxProfileManagerTest.cs Reactivated and updated Firefox profile manager tests        +5/-4      FirefoxProfileTests.cs Reactivated and updated Firefox profile tests                        +4/-3

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[qodo-code-review]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 15536 - Partially compliant Compliant requirements: • Avoid static context in EnvironmentManager (partially addressed by making properties readonly and improving singleton implementation) Non-compliant requirements: • Consolidate test projects (no changes to merge test projects) • Fix namespace issues (namespace structure remains unchanged) Requires further human verification: • Verify if the changes actually improve parallelization capabilities • Check if the modernized code structure allows for better test execution performance

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Incomplete Refactoring The EnvironmentManager class still has some fields that were converted to properties but their initialization is not visible in the diff. Ensure all properties are properly initialized. public static EnvironmentManager Instance => instance ??= new EnvironmentManager(); public Browser Browser { get; } public string CurrentDirectory { get { string assemblyLocation = Path.GetDirectoryName(typeof(EnvironmentManager).Assembly.Location); string testDirectory = TestContext.CurrentContext.TestDirectory; if (assemblyLocation != testDirectory) { return assemblyLocation; } return testDirectory; } } public TestWebServer WebServer { get; } public RemoteSeleniumServer RemoteServer { get; } public string RemoteCapabilities { get; } public UrlBuilder UrlBuilder { get; } Property Visibility The driver property is now public instead of protected, which might expose implementation details unnecessarily and allow external modification. public IWebDriver driver { get; set; }

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Close driver before creating new The method creates a fresh driver but doesn't properly clean up the previous one. This can lead to resource leaks as browser instances may remain open. dotnet/test/common/CustomTestAttributes/NeedsFreshDriverAttribute.cs [43-52] public override void AfterTest(ITest test) { if (test.Fixture is DriverTestFixture fixtureInstance && this.IsCreatedAfterTest) { + EnvironmentManager.Instance.CloseCurrentDriver(); EnvironmentManager.Instance.CreateFreshDriver(); fixtureInstance.driver = EnvironmentManager.Instance.GetCurrentDriver(); } base.AfterTest(test); } Apply this suggestion Suggestion importance[1-10]: 9 __ Why: Not closing the previous driver before creating a new one will cause resource leaks as browser instances remain open. This can significantly impact system resources during test runs.	High
	Avoid blocking in finalizer The finalizer is blocking on asynchronous operations which can lead to deadlocks or application hangs. Finalizers should be quick and non-blocking. Consider implementing IDisposable pattern instead. dotnet/test/common/Environment/EnvironmentManager.cs [193-198] ~EnvironmentManager() { - RemoteServer?.StopAsync().Wait(); - WebServer?.StopAsync().Wait(); + // Finalizers should not block on async operations CloseCurrentDriver(); } Apply this suggestion Suggestion importance[1-10]: 8 __ Why: Blocking on async operations in finalizers is a serious issue that can lead to deadlocks or application hangs. Finalizers run on a special thread and should complete quickly without blocking operations.	Medium
	Use safe casting Direct casting with (IJavaScriptExecutor) will throw an InvalidCastException if the driver doesn't implement IJavaScriptExecutor. Use the 'as' operator or pattern matching to safely cast. dotnet/test/common/TestUtilities.cs [26-29] private static IJavaScriptExecutor GetExecutor(IWebDriver driver) { - return (IJavaScriptExecutor)driver; + return driver as IJavaScriptExecutor ?? throw new ArgumentException("Driver does not support JavaScript execution"); } Apply this suggestion Suggestion importance[1-10]: 7 __ Why: Direct casting with parentheses will throw an InvalidCastException if the driver doesn't implement IJavaScriptExecutor. Using the 'as' operator with null check provides safer type conversion with proper error handling.	Medium
Update

[RenderMichael]

@nvborisenko What do you think? This modernization should have no behavior changes, and it will allow us to iterate on our testing without adding a million lines which change nothing.

[nvborisenko]

In general I like it. But I don't like implicit "magic" namespace for public async Task RunBeforeAnyTestAsync(). Let us still be explicit.

[RenderMichael]

The no-namespace magic is NUnit’s, no choice but to use it https://docs.nunit.org/articles/nunit/writing-tests/attributes/setupfixture.html#notes

[RenderMichael]

@nvborisenko Let me know if you’re OK with this approach in light of the NUnit feature. I think I can figure it out one way or another.