Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[dotnet] Overwrite internal log file if it already exists #13900

Merged
merged 4 commits into from May 15, 2024

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented May 2, 2024

User description

Description

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

Motivation and Context

Fixes #13860

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.

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

    Copy link

    PR Description updated to latest commit (7274501)

    Copy link

    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=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

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

    Copy link

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    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=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

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

    @nvborisenko
    Copy link
    Member Author

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

    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
    Copy link
    Member

    diemol commented May 9, 2024

    I agree with @YevgeniyShunevych

    @diemol diemol added this to the 4.21 milestone May 9, 2024
    @diemol
    Copy link
    Member

    diemol commented May 10, 2024

    Can we merge this?

    @nvborisenko
    Copy link
    Member Author

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

    @diemol
    Copy link
    Member

    diemol commented May 10, 2024

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

    @nvborisenko
    Copy link
    Member Author

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

    @diemol
    Copy link
    Member

    diemol commented May 10, 2024

    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.

    @nvborisenko nvborisenko changed the title [dotnet] Optionally overwrite internal log file if it already exists [dotnet] Overwrite internal log file if it already exists May 14, 2024
    Copy link
    Member

    @diemol diemol left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Thank you, @nvborisenko!

    @diemol diemol merged commit 15319c0 into SeleniumHQ:trunk May 15, 2024
    11 checks passed
    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.

    [🚀 Feature]: Ability to overwrite files when using FileLogHandler
    3 participants