Skip to content

Conversation

@RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Nov 26, 2025

User description

Dependency is unnecessary; now the tests can align with the product code in JSON serialization.

🔗 Related Issues

💥 What does this PR do?

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

  • Replace Newtonsoft.Json with System.Text.Json for test serialization

  • Remove Newtonsoft.Json dependency from test projects

  • Implement JsonSerializerContext for source-generated JSON serialization

  • Update JSON attributes to use System.Text.Json equivalents


Diagram Walkthrough

flowchart LR
  A["Newtonsoft.Json<br/>Dependency"] -->|Remove| B["System.Text.Json<br/>Built-in"]
  C["JsonProperty<br/>Attributes"] -->|Replace| D["System.Text.Json<br/>Attributes"]
  E["JsonConvert<br/>Methods"] -->|Replace| F["JsonSerializer<br/>Methods"]
  G["SeleniumTestSerializerContext<br/>New"] -->|Enable| H["Source-Generated<br/>Serialization"]
Loading

File Walkthrough

Relevant files
Dependencies
11 files
DriverConfig.cs
Replace Newtonsoft.Json with System.Text.Json attributes 
+2/-9     
EnvironmentManager.cs
Update JSON deserialization to use JsonSerializer               
+2/-2     
TestEnvironment.cs
Remove Newtonsoft.Json attributes from class                         
+0/-10   
TestWebServerConfig.cs
Replace Newtonsoft.Json with System.Text.Json attributes 
+2/-5     
UrlBuilder.cs
Replace JsonConvert with JsonObject serialization               
+3/-3     
WebsiteConfig.cs
Remove Newtonsoft.Json attributes from class                         
+0/-8     
paket.nuget.bzl
Remove Newtonsoft.Json from NuGet dependencies                     
+0/-1     
paket.dependencies
Remove Newtonsoft.Json package dependency                               
+0/-1     
BUILD.bazel
Remove Newtonsoft.Json from build dependencies                     
+0/-2     
Selenium.WebDriver.Common.Tests.csproj
Remove Newtonsoft.Json package reference                                 
+0/-1     
project.hbs
Remove Newtonsoft.Json from template dependencies               
+1/-5     
Enhancement
1 files
SeleniumTestSerializerContext.cs
Add new source-generated JSON serializer context                 
+27/-0   

@selenium-ci selenium-ci added C-dotnet .NET Bindings B-build Includes scripting, bazel and CI integrations B-devtools Includes everything BiDi or Chrome DevTools related labels Nov 26, 2025
@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Nov 26, 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
🟡
🎫 #1234
🔴 Investigate and fix regression where clicking a link with JavaScript in its href no longer
triggers in Selenium 2.48.x on Firefox 42 (worked in 2.47.1).
Provide a reproducible test to verify the JavaScript href click triggers as expected.
🟡
🎫 #5678
🔴 Diagnose and resolve "Error: ConnectFailure (Connection refused)" when instantiating
multiple ChromeDriver instances on Ubuntu 16.04.4 with Chrome 65/ChromeDriver 2.35 and
Selenium 3.9.0.
Provide steps or code changes that prevent the connection failures for subsequent
ChromeDriver instantiations.
Validate fix across multiple instantiations to ensure stability.
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 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: Comprehensive Audit Trails

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

Status:
Action Logging: New code constructs and sends HTTP JSON payloads without any added logging of critical
actions, but as this is test infrastructure it may be acceptable.

Referred Code
public string CreateInlinePage(InlinePage page)
{
    Uri createPageUri = new Uri(new Uri(WhereIs(string.Empty)), "createPage");

    var payloadDictionary = new JsonObject
    {
        ["content"] = page.ToString()
    };

    string commandPayload = payloadDictionary.ToJsonString();

    using var httpClient = new HttpClient();

    var postHttpContent = new StringContent(commandPayload, Encoding.UTF8, "application/json");

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:
Deserialization Errors: New JSON deserialization with System.Text.Json lacks explicit error handling for
invalid/missing config content, though test context may handle this elsewhere.

Referred Code
string content = File.ReadAllText(dataFilePath);
TestEnvironment env = JsonSerializer.Deserialize<TestEnvironment>(content, SeleniumTestSerializerContext.Default.TestEnvironment);

string activeDriverConfig = System.Environment.GetEnvironmentVariable("ACTIVE_DRIVER_CONFIG") ?? TestContext.Parameters.Get("ActiveDriverConfig", env.ActiveDriverConfig);

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 new JSON payload creation uses page content without visible validation or sanitization
before sending, which may be acceptable in test code but merits review.

Referred Code
public string CreateInlinePage(InlinePage page)
{
    Uri createPageUri = new Uri(new Uri(WhereIs(string.Empty)), "createPage");

    var payloadDictionary = new JsonObject
    {
        ["content"] = page.ToString()
    };

    string commandPayload = payloadDictionary.ToJsonString();

    using var httpClient = new HttpClient();

    var postHttpContent = new StringContent(commandPayload, Encoding.UTF8, "application/json");

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

qodo-merge-pro bot commented Nov 26, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add missing types to JSON serialization context

Add [JsonSerializable] attributes for DriverConfig, WebsiteConfig,
TestWebServerConfig, and related dictionary types to the
SeleniumTestSerializerContext. This is required for the System.Text.Json source
generator to correctly handle the entire object graph during deserialization.

dotnet/test/common/Environment/SeleniumTestSerializerContext.cs [25-27]

 [JsonSerializable(typeof(TestEnvironment))]
+[JsonSerializable(typeof(DriverConfig))]
+[JsonSerializable(typeof(WebsiteConfig))]
+[JsonSerializable(typeof(TestWebServerConfig))]
+[JsonSerializable(typeof(Dictionary<string, DriverConfig>))]
+[JsonSerializable(typeof(Dictionary<string, WebsiteConfig>))]
 [JsonSourceGenerationOptions(PropertyNameCaseInsensitive = true)]
 internal sealed partial class SeleniumTestSerializerContext : JsonSerializerContext;
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical bug where the System.Text.Json source generator is missing [JsonSerializable] attributes for nested types within TestEnvironment, which would cause a runtime exception during deserialization.

High
Learned
best practice
Dispose HTTP content explicitly

Wrap the content read in a using block and ensure the HttpClient and HttpContent
are disposed deterministically to prevent resource leaks if exceptions occur.

dotnet/test/common/Environment/UrlBuilder.cs [135-141]

 using var httpClient = new HttpClient();
+using var postHttpContent = new StringContent(commandPayload, Encoding.UTF8, "application/json");
+using var response = httpClient.PostAsync(createPageUri, postHttpContent).GetAwaiter().GetResult();
+using var content = response.Content;
+var responseString = content.ReadAsStringAsync().GetAwaiter().GetResult();
 
-var postHttpContent = new StringContent(commandPayload, Encoding.UTF8, "application/json");
-
-using var response = httpClient.PostAsync(createPageUri, postHttpContent).GetAwaiter().GetResult();
-
-var responseString = response.Content.ReadAsStringAsync().GetAwaiter().GetResult();
-
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Ensure resource cleanup by disposing HTTP resources via using/try-finally, including HttpClient responses, to avoid leaks on exceptions.

Low
  • Update

@nvborisenko nvborisenko self-requested a review November 26, 2025 19:02
Copy link
Member

@nvborisenko nvborisenko left a comment

Choose a reason for hiding this comment

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

Thank you, one less tech debt.

@RenderMichael RenderMichael merged commit 1fbd40c into SeleniumHQ:trunk Nov 26, 2025
10 checks passed
@RenderMichael RenderMichael deleted the remove-newtonsoft branch November 26, 2025 19:09
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-devtools Includes everything BiDi or Chrome DevTools related C-dotnet .NET Bindings Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants