[dotnet] implementation of log path property in class FirefoxDriverService

[iampopovich]

User description

fix build

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

This pull request introduces logging functionality to the FirefoxDriverService class and adds corresponding tests to ensure the new feature works correctly. The most important changes include adding a new property for specifying the log path, updating command-line arguments to include the log path, and adding tests to verify logging behavior.

Enhancements to FirefoxDriverService:

• Added a new private field logPath and a public property LogPath to specify the path for service logging. [1] [2]
• Updated the CommandLineArguments property to include the log path in the command-line arguments if it is set.

Tests for logging functionality:

• Introduced a new test class FirefoxDriverServiceTests with tests to verify that the LogPath property can be set, and that the log file is created and contains content when the service starts.
• Added tests in FirefoxDriverTest to ensure that logging to a file works as expected and that the LogPath is null by default.

Motivation and Context

these changes are a part of issue #12273

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

Enhancement, Tests

---

Description

• Added LogPath property to FirefoxDriverService for logging configuration.

• Updated command-line arguments to include log path if set.

• Introduced tests to validate LogPath functionality in FirefoxDriverServiceTests.

• Added integration tests in FirefoxDriverTest to ensure logging works as expected.

---

Changes walkthrough 📝

	Relevant files
Enhancement	FirefoxDriverService.cs Introduced `LogPath` property and updated command-line arguments dotnet/src/webdriver/Firefox/FirefoxDriverService.cs Added LogPath property for specifying log file path. Updated command-line arguments to include log path if set. Introduced private field logPath to store the log path. +15/-0
Tests	FirefoxDriverServiceTests.cs Added unit tests for `LogPath` in FirefoxDriverService      dotnet/test/firefox/FirefoxDriverServiceTests.cs Added tests to verify LogPath property functionality. Ensured log file creation and content validation during service start. Included setup and teardown for temporary file management. +63/-0    FirefoxDriverTest.cs Added integration tests for FirefoxDriver logging                dotnet/test/firefox/FirefoxDriverTest.cs Added integration test for logging to a file. Verified default LogPath is null. Ensured log file creation and content validation during driver usage. +37/-0

---

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 12273 - Partially compliant Compliant requirements: Firefox should be able to log to file Non-compliant requirements: All drivers should be able to log to console Chrome/Edge/Firefox should be able to set log level Chrome/Edge should be able to set readableTimeStamp Firefox should be able to set truncated logs and profile root Safari should be able to enable logging

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 PR contains tests

🔒 Security concerns File System Access: The implementation allows writing log files to any location specified by LogPath. This could potentially be used to write files to sensitive system locations if proper path validation is not implemented.

⚡ Recommended focus areas for review Path Validation The LogPath property setter doesn't validate if the provided path is valid or if the directory exists. This could lead to runtime errors. { get { return this.logPath; } set { this.logPath = value; } } Resource Cleanup The service.Start() test might leave Firefox processes running if an assertion fails before reaching the finally block. service.Start(); try { Assert.That(File.Exists(tempFileName), Is.True, "Log file should be created when service starts"); string logContent = File.ReadAllText(tempFileName); Assert.That(logContent, Is.Not.Empty, "Log file should contain content"); }

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Ensure proper resource cleanup by using try-finally blocks around driver instances Add cleanup in a try-finally block around the Firefox driver instance to ensure proper disposal even if assertions fail. dotnet/test/firefox/FirefoxDriverTest.cs [374-378] -IWebDriver firefox = new FirefoxDriver(service, options); -firefox.Url = xhtmlTestPage; -Assert.That(firefox.Title, Is.EqualTo("XHTML Test Page")); -firefox.Quit(); +IWebDriver firefox = null; +try { + firefox = new FirefoxDriver(service, options); + firefox.Url = xhtmlTestPage; + Assert.That(firefox.Title, Is.EqualTo("XHTML Test Page")); +} +finally { + firefox?.Quit(); +} Apply this suggestion Suggestion importance[1-10]: 9 Why: Critical improvement for resource management - ensures driver cleanup even if tests fail, preventing resource leaks and potential system issues.	9
	Validate log file path to prevent runtime failures due to invalid directories Validate the log path before setting it to ensure it points to a valid directory location and the process has write permissions. dotnet/src/webdriver/Firefox/FirefoxDriverService.cs [160-164] public string LogPath { get { return this.logPath; } - set { this.logPath = value; } + set { + if (!string.IsNullOrEmpty(value) && !Directory.GetParent(value)?.Exists == true) + throw new ArgumentException("Log path directory must exist", nameof(value)); + this.logPath = value; + } } Apply this suggestion Suggestion importance[1-10]: 8 Why: Important defensive programming practice that prevents runtime errors by validating path validity before usage, improving robustness of the logging feature.	8
General	Add test coverage for invalid log path scenarios to ensure proper error handling Add a test case to verify behavior when an invalid log path is provided. The service should handle invalid paths gracefully with appropriate error handling. dotnet/test/firefox/FirefoxDriverServiceTests.cs [30-36] [Test] public void CanSetLogPath() { var service = FirefoxDriverService.CreateDefaultService(); service.LogPath = tempFileName; Assert.That(service.LogPath, Is.EqualTo(tempFileName), "LogPath should be set correctly"); } +[Test] +public void ShouldHandleInvalidLogPath() +{ + var service = FirefoxDriverService.CreateDefaultService(); + service.LogPath = "\\\\invalid:path"; + Assert.Throws<WebDriverException>(() => service.Start(), "Service should throw when log path is invalid"); +} + Apply this suggestion Suggestion importance[1-10]: 8 Why: Adding error handling test cases is crucial for robustness and reliability. The suggestion helps prevent runtime issues by validating how the service handles invalid paths.	8

[iampopovich]

@nvborisenko Nikolay , hi 👋
i see there is no BUILD file in dotnet/test/firefox directory
Tests in this directory are not run on a regular basis ?
Should we move the tests to the common directory?
Or is there a way to run the tests in the firefox directory without creating a BUILD file?

[nvborisenko]

Hi! You are doing right. The location of tests is correct, and yes, they are not ran on regular basis for now.

You can execute tests in your IDE you like.