WARNING: THIS SITE IS A MIRROR OF GITHUB.COM / IT CANNOT LOGIN OR REGISTER ACCOUNTS / THE CONTENTS ARE PROVIDED AS-IS / THIS SITE ASSUMES NO RESPONSIBILITY FOR ANY DISPLAYED CONTENT OR LINKS / IF YOU FOUND SOMETHING MAY NOT GOOD FOR EVERYONE, CONTACT ADMIN AT ilovescratch@foxmail.com
Skip to content

Conversation

@RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Dec 21, 2025

User description

Contributes to #16115

💥 What does this PR do?

Ensure that we do not use .NET Core only APIs in the test suites.

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

  • Replace .NET Core-only APIs with .NET Framework compatible alternatives

  • Add utility extension classes for cross-framework compatibility

  • Update test code to use reflection and conditional compilation

  • Fix test assertions for negative zero and datetime precision


Diagram Walkthrough

flowchart LR
  A["Test Files"] -->|Replace .NET Core APIs| B["Utility Extensions"]
  A -->|Use reflection| C["Compatibility Layer"]
  B -->|DoubleExtensions| D[".NET Framework Support"]
  B -->|PlatformUtilities| D
  B -->|ProcessExtensions| D
  B -->|TaskExtensions| D
  C -->|Conditional Compilation| D
Loading

File Walkthrough

Relevant files
Compatibility fix
5 files
CallFunctionLocalValueTest.cs
Replace double.NegativeZero with literal                                 
+1/-1     
CallFunctionParameterTest.cs
Replace UnsafeAccessor with reflection API                             
+4/-2     
CallFunctionRemoteValueTest.cs
Use extension method for negative check                                   
+2/-1     
TestWebServer.cs
Replace OperatingSystem with utility wrapper                         
+2/-1     
UrlBuilder.cs
Replace range operator with Substring method                         
+3/-2     
Bug fix
1 files
LocalValueConversionTests.cs
Fix DateTimeOffset constructor and assertion                         
+2/-2     
New utility
5 files
ArrayExtensions.cs
Add array reverse extension for compatibility                       
+31/-0   
DoubleExtensions.cs
Add IsNegative extension for double type                                 
+34/-0   
PlatformUtilities.cs
Add cross-framework platform detection utility                     
+32/-0   
ProcessExtensions.cs
Add process kill extension method                                               
+37/-0   
TaskExtensions.cs
Add WaitAsync extension for pre-.NET 8                                     
+41/-0   

@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Dec 21, 2025
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 21, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡
🎫 #16115
🟢 Ensure test suites do not rely on .NET (Core/8+) only APIs so they can run correctly on
.NET Framework implementations behind netstandard2.0.
🔴 Execute internal unit tests against .NET Framework (e.g., target .NET Framework such as
4.6.1 or 4.8 in addition to current .NET 8 targeting).
Detect/handle behavioral differences between .NET 8 and .NET Framework that may impact
tests (e.g., networking/CDP/BiDi differences), ideally by exposing issues via tests.
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

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: 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: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status:
Misleading method name: The Kill(this Process process, bool entireProcessTree) overload implies it can kill an
entire process tree but currently always kills only the parent process, making the API
behavior misleading.

Referred Code
public static void Kill(this Process process, bool entireProcessTree)
{
    if (!entireProcessTree)
    {
        process.Kill();
        return;
    }

    // TODO kill all child processes
    process.Kill();
}

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:
Unhandled edge case: The entireProcessTree true-path is acknowledged as incomplete via a // TODO and does not
actually handle child processes, leaving the key edge case unimplemented.

Referred Code
public static void Kill(this Process process, bool entireProcessTree)
{
    if (!entireProcessTree)
    {
        process.Kill();
        return;
    }

    // TODO kill all child processes
    process.Kill();
}

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
Copy link
Contributor

qodo-code-review bot commented Dec 21, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Implement asynchronous wait to prevent deadlocks
Suggestion Impact:Updated WaitAsync to be async, removed the blocking task.Wait call, and implemented timeout handling via Task.Delay and Task.WhenAny, then awaited the original task.

code diff:

-    public static Task<T> WaitAsync<T>(this Task<T> task, TimeSpan timeout)
+    public static async Task<T> WaitAsync<T>(this Task<T> task, TimeSpan timeout)
     {
-        // Good enough implementation
-        var success = task.Wait(timeout);
-        if (!success)
+        var timeoutTask = Task.Delay(timeout);
+        var completedTask = await Task.WhenAny(task, timeoutTask);
+        if (completedTask == timeoutTask)
         {
             throw new TimeoutException();
         }
-
-        return Task.FromResult(task.Result);
+        return await task;
     }

Replace the blocking task.Wait(timeout) with a non-blocking, asynchronous
implementation using Task.WhenAny and Task.Delay to prevent potential deadlocks.

dotnet/test/common/Utilities/TaskExtensions.cs [28-38]

-public static Task<T> WaitAsync<T>(this Task<T> task, TimeSpan timeout)
+public static async Task<T> WaitAsync<T>(this Task<T> task, TimeSpan timeout)
 {
-    // Good enough implementation
-    var success = task.Wait(timeout);
-    if (!success)
+    var timeoutTask = Task.Delay(timeout);
+    var completedTask = await Task.WhenAny(task, timeoutTask).ConfigureAwait(false);
+    if (completedTask == timeoutTask)
     {
         throw new TimeoutException();
     }
 
-    return Task.FromResult(task.Result);
+    return await task.ConfigureAwait(false);
 }

[Suggestion processed]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that the current WaitAsync implementation is blocking and proposes a proper asynchronous alternative using Task.WhenAny, which prevents potential deadlocks and improves performance.

Medium
General
Implement process tree kill

Implement process tree killing by using the built-in
process.Kill(entireProcessTree: true) overload on .NET 6+ and leaving the TODO
for older runtimes.

dotnet/test/common/Utilities/ProcessExtensions.cs [26-36]

 public static void Kill(this Process process, bool entireProcessTree)
 {
     if (!entireProcessTree)
     {
         process.Kill();
         return;
     }
 
-    // TODO kill all child processes
+#if NET6_0_OR_GREATER
+    // Use built-in tree kill support
+    process.Kill(entireProcessTree: true);
+#else
+    // TODO: Enumerate and kill child processes on earlier runtimes
     process.Kill();
+#endif
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly addresses the TODO by providing a modern and clean solution using the built-in process.Kill(true) for supported .NET versions, while keeping the TODO for older runtimes.

Medium
  • Update

Comment on lines 28 to 38
public static Task<T> WaitAsync<T>(this Task<T> task, TimeSpan timeout)
{
// Good enough implementation
var success = task.Wait(timeout);
if (!success)
{
throw new TimeoutException();
}

return Task.FromResult(task.Result);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Implement asynchronous wait to prevent deadlocks

Suggested change
public static Task<T> WaitAsync<T>(this Task<T> task, TimeSpan timeout)
{
// Good enough implementation
var success = task.Wait(timeout);
if (!success)
{
throw new TimeoutException();
}
return Task.FromResult(task.Result);
}
public static async Task<T> WaitAsync<T>(this Task<T> task, TimeSpan timeout)
{
var timeoutTask = Task.Delay(timeout);
var completedTask = await Task.WhenAny(task, timeoutTask).ConfigureAwait(false);
if (completedTask == timeoutTask)
{
throw new TimeoutException();
}
return await task.ConfigureAwait(false);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants