Skip to content

Conversation

@diemol
Copy link
Member

@diemol diemol commented Aug 17, 2025

User description

🔗 Related Issues

This is related to #12273. I mistakenly thought it was not possible to enable logging in SafariDriver.

💥 What does this PR do?

This option enables logging in SafariDriver, which only goes to a file in a hardcoded directory (~/Library/Logs/com.apple.WebDriver/).

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement


Description

  • Add Diagnose property to enable SafariDriver logging

  • Append --diagnose flag to command line arguments

  • Remove unused imports for cleaner code


Diagram Walkthrough

flowchart LR A["SafariDriverService"] --> B["Diagnose Property"] B --> C["--diagnose Flag"] C --> D["~/Library/Logs/com.apple.WebDriver/"] 
Loading

File Walkthrough

Relevant files
Enhancement
SafariDriverService.cs
Add diagnostic logging support and cleanup imports             

dotnet/src/webdriver/Safari/SafariDriverService.cs

  • Add Diagnose boolean property with documentation
  • Modify CommandLineArguments to append --diagnose flag when enabled
  • Remove unused System.Net and System.Threading.Tasks imports
+13/-3   

@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Aug 17, 2025
@qodo-merge-pro
Copy link
Contributor

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

API Consistency

Consider whether the new nullable bool Diagnose should be non-nullable with a default (e.g., false) for a clearer public API and to match similar flags elsewhere in the codebase.

/// <summary> /// Value to enable diagnose logging. /// When set to true, the SafariDriver will be started with the --diagnose flag. /// Logs will be written to ~/Library/Logs/com.apple.WebDriver/ /// </summary> public bool? Diagnose { get; set; } /// <summary>
Argument Formatting

Ensure no unintended leading/trailing spaces or duplicate flags occur when appending " --diagnose"; verify base.CommandLineArguments handling and potential repeated appends across calls.

protected override string CommandLineArguments { get { StringBuilder argsBuilder = new StringBuilder(base.CommandLineArguments); if (this.Diagnose is true) { argsBuilder.Append(" --diagnose"); } return argsBuilder.ToString(); }
@diemol diemol added this to the Selenium 4.36 milestone Aug 17, 2025
@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Aug 17, 2025

PR Code Suggestions ✨

diemol and others added 2 commits August 18, 2025 07:10
Co-authored-by: Nikolay Borisenko <22616990+nvborisenko@users.noreply.github.com>
Co-authored-by: Nikolay Borisenko <22616990+nvborisenko@users.noreply.github.com>
@diemol diemol merged commit 1283dd2 into trunk Aug 18, 2025
10 checks passed
@diemol diemol deleted the dotnet_enable_logging_safaridriver branch August 18, 2025 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4 participants