Skip to content

Conversation

@diemol
Copy link
Member

@diemol diemol commented Aug 5, 2025

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

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

qodo-merge-pro bot commented Aug 5, 2025

PR Code Suggestions ✨

No code suggestions found for the PR.

@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
/// <summary>
/// Adds readable timestamps to log
/// </summary>
public bool ReadableTimestamp { get; set; }
Copy link
Member

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants