Skip to content

[dotnet] Add readable timestamps to log in Chromium browsers#16133

Merged
diemol merged 2 commits intotrunkfrom
dotnet_chromium_readable_timestamp
Aug 5, 2025
Merged

[dotnet] Add readable timestamps to log in Chromium browsers#16133
diemol merged 2 commits intotrunkfrom
dotnet_chromium_readable_timestamp

Conversation

@diemol
Copy link
Copy Markdown
Member

@diemol diemol commented Aug 5, 2025 •
edited by qodo-code-review bot
Loading

User description

🔗 Related Issues

Helps with #12273

💥 What does this PR do?

🔧 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

  • Add ReadableTimestamp property to ChromiumDriverService

  • Enable readable timestamp logging via --readable-timestamp command line argument

Diagram Walkthrough

flowchart LR
  A["ChromiumDriverService"] --> B["ReadableTimestamp Property"]
  B --> C["--readable-timestamp CLI Argument"]
  C --> D["Enhanced Log Output"]
Loading

File Walkthrough
Relevant files
Enhancement
ChromiumDriverService.cs
Add readable timestamp logging support                                     

dotnet/src/webdriver/Chromium/ChromiumDriverService.cs

  • Added ReadableTimestamp boolean property with XML documentation
  • Modified CommandLineArguments to append --readable-timestamp flag when
    enabled
 +10/-0   

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

qodo-code-review bot commented Aug 5, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ❌

1234 - Not compliant

Non-compliant requirements:

• Fix JavaScript execution in link href attributes when using click() method
• Ensure compatibility with Firefox 42.0
• Restore functionality that worked in version 2.47.1 but broke in 2.48.0/2.48.2

5678 - Not compliant

Non-compliant requirements:

• Fix ChromeDriver ConnectFailure error on Ubuntu 16.04.4
• Resolve connection refused errors for subsequent ChromeDriver instances
• Ensure compatibility with Chrome 65.0.3325.181 and ChromeDriver 2.35

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

Missing Documentation

The XML documentation for ReadableTimestamp property is incomplete and lacks detail about what readable timestamps actually provide or when this feature should be used.

 
/// <summary>
/// Adds readable timestamps to log
/// </summary>
public bool ReadableTimestamp { get; set; }

@qodo-code-review
Copy link
Copy Markdown
Contributor

qodo-code-review bot commented Aug 5, 2025 •
edited
Loading

PR Code Suggestions ✨

No code suggestions found for the PR.

@diemol diemol mentioned this pull request Aug 5, 2025
Closed
6 tasks
@diemol diemol added this to the Selenium 4.35 milestone Aug 5, 2025
@diemol diemol merged commit 2ab802b into trunk Aug 5, 2025
10 checks passed
@diemol diemol deleted the dotnet_chromium_readable_timestamp branch August 5, 2025 19:30
nvborisenko
nvborisenko reviewed Aug 6, 2025
View reviewed changes
dotnet/src/webdriver/Chromium/ChromiumDriverService.cs
/// <summary>
/// Adds readable timestamps to log
/// </summary>
public bool ReadableTimestamp { get; set; }
Copy link
Copy Markdown
Member

@nvborisenko nvborisenko Aug 6, 2025

Choose a reason for hiding this comment

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

Kind of optional argument, would be nice to convert it to nullable bool?. Yes, it doesn't follow existing pattern, but improving old API is a breaking change.

Contributes to: #14640

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

Assignees

No one assigned

Labels

C-dotnet .NET Bindings Review effort 2/5

Projects

None yet

Milestone

Selenium 4.35

Development

Successfully merging this pull request may close these issues.

3 participants

@diemol @nvborisenko @selenium-ci