Skip to content

[dotnet] Enabling Chrome and Edge driver services to set log level#16098

Merged
diemol merged 5 commits intotrunkfrom
dotnet_log_level_chrome_edge_driver
Jul 29, 2025
Merged

[dotnet] Enabling Chrome and Edge driver services to set log level#16098
diemol merged 5 commits intotrunkfrom
dotnet_log_level_chrome_edge_driver

Conversation

@diemol
Copy link
Copy Markdown
Member

@diemol diemol commented Jul 28, 2025 •
edited by qodo-code-review bot
Loading

User description

🔗 Related Issues

Helps with #12273

💥 What does this PR do?

Enabling Chrome and Edge driver services to set log level

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)
  • New feature (non-breaking change which adds functionality and tests!)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR Type

Enhancement

Description

  • Added ChromiumDriverLogLevel enum with logging levels (All, Debug, Info, Warning, Severe, Off, Default)

  • Added LogLevel property to ChromiumDriverService for configuring driver log output

  • Implemented command line argument generation for --log-level parameter

Diagram Walkthrough

flowchart LR
  A["ChromiumDriverLogLevel enum"] --> B["ChromiumDriverService.LogLevel property"]
  B --> C["Command line argument --log-level"]
Loading

File Walkthrough
Relevant files
Enhancement
ChromiumDriverLogLevel.cs
New ChromiumDriverLogLevel enum definition                             

dotnet/src/webdriver/Chromium/ChromiumDriverLogLevel.cs

  • Created new enum with 7 logging levels (All, Debug, Info, Warning,
    Severe, Off, Default)
  • Added comprehensive XML documentation for each enum value
 +61/-0   
ChromiumDriverService.cs
LogLevel property and command line integration                     

dotnet/src/webdriver/Chromium/ChromiumDriverService.cs

  • Added LogLevel property with ChromiumDriverLogLevel.Default as default
    value
  • Modified CommandLineArguments to include --log-level parameter when
    not default
  • Added XML documentation for the new LogLevel property
 +11/-0   

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review bot commented Jul 28, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Formatting Issue

There is an extra space after "Warning" in the enum value definition that should be removed for consistency with other enum values.

 
Warning ,
Case Sensitivity

The log level is converted to uppercase using ToUpperInvariant(), but it should be verified that the Chromium driver actually expects uppercase values for the --log-level parameter.

 
    argsBuilder.Append(string.Format(CultureInfo.InvariantCulture, " --log-level={0}", this.LogLevel.ToString().ToUpperInvariant()));
}

@diemol diemol mentioned this pull request Jul 28, 2025
Closed
6 tasks
@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review bot commented Jul 28, 2025 •
edited
Loading

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
✅ Add enum validation before string conversion
Suggestion Impact:The suggestion was directly implemented - the commit added the exact enum validation check using Enum.IsDefined() and wrapped the string conversion in a conditional block as suggested

code diff:

+                if (Enum.IsDefined(typeof(ChromiumDriverLogLevel), this.LogLevel))
+                {
+                    argsBuilder.Append($" --log-level={this.LogLevel.ToString().ToUpperInvariant()}");
+                }

Add validation to ensure the enum value is defined before converting to string.
This prevents potential issues if an invalid enum value is somehow assigned to
LogLevel.

dotnet/src/webdriver/Chromium/ChromiumDriverService.cs [162-165]

 if (this.LogLevel != ChromiumDriverLogLevel.Default)
 {
-    argsBuilder.Append(string.Format(CultureInfo.InvariantCulture, " --log-level={0}", this.LogLevel.ToString().ToUpperInvariant()));
+    if (Enum.IsDefined(typeof(ChromiumDriverLogLevel), this.LogLevel))
+    {
+        argsBuilder.Append($" --log-level={this.LogLevel.ToString().ToUpperInvariant()}");
+    }
 }

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: This is a good defensive programming suggestion that improves robustness by validating the enum value, preventing potential errors if an undefined value is assigned to the public LogLevel property.

Medium
✅ Use string interpolation for cleaner code
Suggestion Impact:The suggestion was implemented - string.Format was replaced with string interpolation on line 14, though the commit also added additional validation logic with Enum.IsDefined check

code diff:

+                    argsBuilder.Append($" --log-level={this.LogLevel.ToString().ToUpperInvariant()}");

Consider using string interpolation instead of string.Format for better
readability and performance. The current approach is unnecessarily verbose for a
simple string concatenation.

dotnet/src/webdriver/Chromium/ChromiumDriverService.cs [164]

-argsBuilder.Append(string.Format(CultureInfo.InvariantCulture, " --log-level={0}", this.LogLevel.ToString().ToUpperInvariant()));
+argsBuilder.Append($" --log-level={this.LogLevel.ToString().ToUpperInvariant()}");

[Suggestion processed]

Suggestion importance[1-10]: 4

__

Why: The suggestion to use string interpolation instead of string.Format is a valid style improvement that enhances code readability without changing functionality.

Low
Fix trailing space formatting issue

Remove the trailing space before the comma in the Warning enum value. This
inconsistent formatting could cause issues with code analysis tools and violates
standard C# formatting conventions.

dotnet/src/webdriver/Chromium/ChromiumDriverLogLevel.cs [45]

-Warning ,
+Warning,
  • Apply / Chat
Suggestion importance[1-10]: 3

__

Why: The suggestion correctly points out a minor formatting inconsistency (a space before a comma), which improves code style and consistency.

Low
  • Update

diemol and others added 3 commits July 28, 2025 14:19
@diemol diemol merged commit ef6c472 into trunk Jul 29, 2025
10 checks passed
@diemol diemol deleted the dotnet_log_level_chrome_edge_driver branch July 29, 2025 11:32
@diemol diemol mentioned this pull request Aug 6, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nvborisenko nvborisenko nvborisenko left review comments

@qodo-code-review qodo-code-review[bot] qodo-code-review[bot] left review comments

Assignees

No one assigned

Labels

Review effort 2/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@diemol @nvborisenko