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?

Allows Firefox to set truncated logs and set profile root

🔧 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 LogTruncate property to control GeckoDriver log truncation

  • Add ProfileRoot property to specify profile directory

  • Include directory validation for profile root path

  • Update command line argument generation logic


Diagram Walkthrough

flowchart LR
  A["FirefoxDriverService"] --> B["LogTruncate Property"]
  A --> C["ProfileRoot Property"]
  B --> D["--log-no-truncate Argument"]
  C --> E["--profile-root Argument"]
  C --> F["Directory Validation"]
Loading

File Walkthrough

Relevant files
Enhancement
FirefoxDriverService.cs
Add log truncation and profile root configuration               

dotnet/src/webdriver/Firefox/FirefoxDriverService.cs

  • Added LogTruncate boolean property for controlling log truncation
  • Added ProfileRoot string property for specifying profile directory
  • Added directory existence validation for ProfileRoot
  • Updated CommandLineArguments to include new command line flags
+25/-0   

@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 ✅

12273 - PR Code Verified

Compliant requirements:

• Allow Firefox to set truncated logs
• Allow setting profile root directory
• Provide control over GeckoDriver log truncation behavior
• Enable specification of profile directory location

Requires further human verification:

• Verify that the command line arguments are correctly passed to GeckoDriver
• Test that log truncation behavior works as expected
• Validate profile root directory functionality in runtime scenarios

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 Security concerns

Path injection:
The ProfileRoot property accepts user input for directory paths without sanitization. While Directory.Exists() provides some validation, malicious paths could potentially be constructed to access unintended directories or cause path traversal issues if not properly handled by the underlying GeckoDriver.

⚡ Recommended focus areas for review

Logic Error

The LogTruncate property logic appears inverted - when LogTruncate is true, it adds the --log-no-truncate flag, which suggests disabling truncation. This naming and behavior mismatch could confuse users.

if (this.LogTruncate)
{
    argsBuilder.Append(" --log-no-truncate");
}
Exception Handling

Directory validation throws ArgumentException but uses nameof(ProfileRoot) which may not match the actual property name casing, potentially causing confusion in error messages.

if (!Directory.Exists(this.ProfileRoot))
{
    throw new ArgumentException($"Profile root directory does not exist: {this.ProfileRoot}", nameof(ProfileRoot));
}

@diemol diemol changed the title Dotnet ff log no truncate [dotnet] Truncate log and set profile root in Firefox (GeckoDriver) Aug 5, 2025
@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Aug 5, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Fix confusing property logic
Suggestion Impact:The suggestion was implemented by renaming the property from LogTruncate to LogNoTruncate, making the property name align with its actual behavior of disabling truncation when set to true

code diff:

-    public bool LogTruncate { get; set; }
+    public bool LogNoTruncate { get; set; }
 
     /// <summary>
     /// Directory in which GeckoDriver creates profiles.
@@ -199,7 +199,7 @@
                 argsBuilder.Append(" --jsdebugger");
             }
 
-            if (this.LogTruncate)
+            if (this.LogNoTruncate)

The property name LogTruncate is misleading since setting it to true actually
disables truncation. Consider renaming to LogNoTruncate or inverting the logic
to make the intent clearer.

dotnet/src/webdriver/Firefox/FirefoxDriverService.cs [202-205]

-if (this.LogTruncate)
+if (!this.LogTruncate)
 {
     argsBuilder.Append(" --log-no-truncate");
 }

[Suggestion processed]

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that the property name LogTruncate is confusing, as setting it to true disables truncation. This change would improve code clarity and maintainability.

Low
Fix parameter name reference

The nameof(ProfileRoot) should reference the actual property name ProfileRoot
instead of the parameter name. Use nameof(this.ProfileRoot) for consistency.

dotnet/src/webdriver/Firefox/FirefoxDriverService.cs [211]

-throw new ArgumentException($"Profile root directory does not exist: {this.ProfileRoot}", nameof(ProfileRoot));
+throw new ArgumentException($"Profile root directory does not exist: {this.ProfileRoot}", nameof(this.ProfileRoot));
  • Apply / Chat
Suggestion importance[1-10]: 1

__

Why: The suggestion to change nameof(ProfileRoot) to nameof(this.ProfileRoot) is a minor stylistic preference, as both expressions compile to the same string "ProfileRoot" and have no functional difference.

Low
  • Update

@diemol diemol added this to the Selenium 4.35 milestone Aug 5, 2025
@diemol diemol merged commit f00e838 into trunk Aug 5, 2025
10 checks passed
@diemol diemol deleted the dotnet_ff_log_no_truncate branch August 5, 2025 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants