[dotnet] Optionally overwrite internal log file if it already exists

[nvborisenko]

User description

Description

Added overload constructor for FileLogHandler(string filePath, bool overwrite).

Motivation and Context

Fixes https://github.com/SeleniumHQ/selenium/issues/13860

Types of changes

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

Checklist

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

---

Type

Enhancement, Tests

---

Description

• Added an overloaded constructor in FileLogHandler to support optional overwriting of log files.
• Modified file handling to append to an existing file or overwrite based on the new overwrite parameter.
• Enhanced unit tests to cover new functionality, including tests for appending to existing files, overwriting files, and handling non-existent files.

---

Changes walkthrough

	Relevant files
Enhancement	FileLogHandler.cs Support Optional Overwriting in FileLogHandler                      dotnet/src/webdriver/Internal/Logging/FileLogHandler.cs Added an overloaded constructor to allow specifying whether to overwrite the log file. Modified file opening logic to either append to or create/overwrite the file based on the overwrite parameter. +14/-2
Tests	FileLogHandlerTest.cs Extend Tests for FileLogHandler Overwrite and Append Behaviors dotnet/test/common/Internal/Logging/FileLogHandlerTest.cs Added tests to verify appending behavior when the file does not exist. Added tests to verify appending behavior when the file exists. Added tests to verify file overwriting behavior. Added tests to verify file creation and writing when the file does not exist. +91/-0

---

✨ PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[qodo-code-review[bot]]

PR Description updated to latest commit (https://github.com/SeleniumHQ/selenium/commit/7274501b74cf0d6aac241f9dd02d5ce19f640ecc)

• [ ] Copy walkthrough table to "Files Changed" Tab

[qodo-code-review[bot]]

PR Review

⏱️ Estimated effort to review [1-5]	2, because the changes are straightforward and localized to specific methods in the FileLogHandler class. The added functionality is well-contained, and the tests are comprehensive and directly related to the changes.
🧪 Relevant tests	Yes
🔍 Possible issues	Possible Bug: The code does not handle potential exceptions that might be thrown by the File.Open method when the file cannot be accessed due to permission issues or I/O errors. This could lead to unhandled exceptions in runtime.
🔒 Security concerns	No

---

✨ Review tool usage guide:

---

Overview: The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

• When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

• With a configuration file, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

[qodo-code-review[bot]]

PR Code Suggestions

Category	Suggestions
Best practice	Add a parameter validation check to the overloaded constructor. Consider adding a check for filePath validity in the overloaded constructor to ensure consistency in error handling across constructors. dotnet/src/webdriver/Internal/Logging/FileLogHandler.cs [35] public FileLogHandler(string filePath, bool overwrite) { + if (string.IsNullOrEmpty(filePath)) throw new ArgumentException("File log path cannot be null or empty.", nameof(filePath));
Enhancement	Modify file sharing mode based on the overwrite flag to prevent access conflicts. To avoid potential file access conflicts, consider specifying FileShare.None when the overwrite flag is true, ensuring exclusive access when a file is being overwritten. dotnet/src/webdriver/Internal/Logging/FileLogHandler.cs [47] -_fileStream = File.Open(filePath, fileMode, FileAccess.Write, FileShare.Read); +_fileStream = File.Open(filePath, fileMode, FileAccess.Write, overwrite ? FileShare.None : FileShare.Read);
	Rename the test method to better describe its functionality. To ensure the test 'ShouldAppendFileIfDoesNotExist' accurately reflects its purpose, consider renaming it to 'ShouldCreateAndAppendFileIfDoesNotExist' for clarity. dotnet/test/common/Internal/Logging/FileLogHandlerTest.cs [40] -public void ShouldAppendFileIfDoesNotExist() +public void ShouldCreateAndAppendFileIfDoesNotExist()
	Add file size checks to validate the overwrite functionality in the test. To improve the robustness of the test 'ShouldOverwriteFileIfExists', add assertions to check the file size or content before and after the overwrite to ensure the file is indeed overwritten and not just appended to or recreated. dotnet/test/common/Internal/Logging/FileLogHandlerTest.cs [96-101] +var initialSize = new FileInfo(tempFile).Length; using (var fileLogHandler = new FileLogHandler(tempFile, overwrite: true)) { fileLogHandler.Handle(new LogEvent(typeof(FileLogHandlerTest), DateTimeOffset.Now, LogEventLevel.Info, "test message")); } +var finalSize = new FileInfo(tempFile).Length; +Assert.That(finalSize, Is.LessThanOrEqualTo(initialSize)); Assert.That(Regex.Matches(File.ReadAllText(tempFile), "test message").Count, Is.EqualTo(1));
Maintainability	Refactor test setup and teardown into separate methods to reduce code duplication. Refactor the repeated test setup and teardown code into a separate method or use a setup and teardown fixture to improve code maintainability and reduce duplication. dotnet/test/common/Internal/Logging/FileLogHandlerTest.cs [67-86] -var tempFile = Path.GetTempFileName(); -try +[SetUp] +public void Setup() +{ + tempFile = Path.GetTempFileName(); +} + +[TearDown] +public void Teardown() +{ + File.Delete(tempFile); +} + +[Test] +public void ShouldAppendFileIfExists() { using (var fileLogHandler = new FileLogHandler(tempFile)) { fileLogHandler.Handle(new LogEvent(typeof(FileLogHandlerTest), DateTimeOffset.Now, LogEventLevel.Info, "test message")); } using (var fileLogHandler2 = new FileLogHandler(tempFile)) { fileLogHandler2.Handle(new LogEvent(typeof(FileLogHandlerTest), DateTimeOffset.Now, LogEventLevel.Info, "test message")); } Assert.That(Regex.Matches(File.ReadAllText(tempFile), "test message").Count, Is.EqualTo(2)); } -finally -{ - File.Delete(tempFile); -}

---

✨ Improve tool usage guide:

---

Overview: The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

• When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

• With a configuration file, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

See the improve usage page for a comprehensive guide on using this tool.

[nvborisenko]

Open questions:

• argument name overwrite or rewrite
• by default true or false (seems true, like a fresh log file per execution)

@diemol please suggest the best naming for point 1

@YevgeniyShunevych cc

[YevgeniyShunevych]

As for me, the current implementation is good. overwrite and false by default (the same behavior as it was before this PR). Just curious whether in other WebDriver language implementations similar log functionality by default overwrites log file or appends to it.

[diemol]

I agree with @YevgeniyShunevych

[diemol]

Can we merge this?

[nvborisenko]

We still need to agree whether existing file should be overwritten by default.

[diemol]

I thought the conversation above meant the default was false, did I miss something?

[nvborisenko]

OK, then let's proceed if it is aligned with others bindings.

[diemol]

Oh, wait. Good point. I was somehow not reading everything in detail. They all overwrite by default, except JS because it does not support sending logs to a file for now.

So, yeah, good point @nvborisenko, it should be true the default then.