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

show warning instead of assert #290

Merged
merged 2 commits into from
Feb 21, 2025
Merged

Conversation

qododavid
Copy link
Collaborator

@qododavid qododavid commented Feb 20, 2025

User description

certain test frameworks will cache the test coverage report, which hits this assert


PR Type

Bug fix, Tests


Description

  • Replaced an assertion with a warning in verify_report_update.

  • Updated logging behavior in CustomLogger.

  • Added a test to validate warning logging in verify_report_update.


Changes walkthrough 📝

Relevant files
Bug fix
CoverageProcessor.py
Replace assertion with warning in `verify_report_update` 

cover_agent/CoverageProcessor.py

  • Replaced an assertion with a warning in verify_report_update.
  • Ensures the logger warns instead of halting execution.
  • +3/-3     
    Enhancement
    CustomLogger.py
    Modify logger propagation behavior                                             

    cover_agent/CustomLogger.py

  • Commented out the logger.propagate line.
  • Adjusted logger behavior for potential propagation.
  • +1/-1     
    Tests
    test_CoverageProcessor.py
    Add test for logging in `verify_report_update`                     

    tests/test_CoverageProcessor.py

  • Added a test to validate warning logging in verify_report_update.
  • Used caplog to capture and assert warning messages.
  • +8/-5     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Logger Configuration

    Commenting out logger.propagate=False may cause duplicate log messages if parent loggers exist. Consider documenting the reason for this change or validating the logging behavior.

    # logger.propagate = False
    Error Handling

    The warning message for outdated coverage report could include guidance on how to resolve the issue, since this was previously treated as a fatal error.

    if not file_mod_time_ms > time_of_test_command:
        self.logger.warning(f"The coverage report file was not updated after the test command. file_mod_time_ms: {file_mod_time_ms}, time_of_test_command: {time_of_test_command}. {file_mod_time_ms > time_of_test_command}")

    Copy link

    qodo-merge-pro-for-open-source bot commented Feb 20, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix potential duplicate logging issue
    Suggestion Impact:The commit implemented the suggestion by uncommenting logger.propagate=False to prevent duplicate logging in parent loggers

    code diff:

    -            # logger.propagate = False
    +            logger.propagate = False

    Commenting out the logger propagation control could lead to duplicate log
    messages in parent loggers. Either remove the comment or document why
    propagation is now needed.

    cover_agent/CustomLogger.py [60-61]

     # Prevent log messages from being propagated to the root logger
    -# logger.propagate = False
    +logger.propagate = False  # Disabled to prevent duplicate logging in parent loggers

    [Suggestion has been applied]

    Suggestion importance[1-10]: 8

    __

    Why: Commenting out logger.propagate=False could cause duplicate log messages to appear in parent loggers, which would degrade log readability and potentially impact debugging. The fix prevents this issue.

    Medium
    General
    Add actionable warning message content

    The warning message should include guidance on potential causes and remediation
    steps for when the coverage report is not updated.

    cover_agent/CoverageProcessor.py [76-77]

     if not file_mod_time_ms > time_of_test_command:
    -    self.logger.warning(f"The coverage report file was not updated after the test command. file_mod_time_ms: {file_mod_time_ms}, time_of_test_command: {time_of_test_command}. {file_mod_time_ms > time_of_test_command}")
    +    self.logger.warning(f"The coverage report file was not updated after the test command. This may indicate test execution failure or coverage tool misconfiguration. Please verify test execution and coverage settings. file_mod_time_ms: {file_mod_time_ms}, time_of_test_command: {time_of_test_command}")
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    __

    Why: The improved warning message provides helpful context and troubleshooting guidance, making it more actionable for users when coverage report issues occur.

    Low
    Learned
    best practice
    Add validation for timestamp values before comparison to handle potential edge cases with invalid or negative timestamps

    When comparing timestamps, validate that both values are valid positive numbers
    before comparison to avoid potential edge cases. Add validation for the input
    timestamp and file modification time.

    cover_agent/CoverageProcessor.py [76-77]

    -if not file_mod_time_ms > time_of_test_command:
    -    self.logger.warning(f"The coverage report file was not updated after the test command. file_mod_time_ms: {file_mod_time_ms}, time_of_test_command: {time_of_test_command}. {file_mod_time_ms > time_of_test_command}")
    +if file_mod_time_ms <= 0 or time_of_test_command <= 0:
    +    self.logger.error(f"Invalid timestamp values: file_mod_time_ms={file_mod_time_ms}, time_of_test_command={time_of_test_command}")
    +elif not file_mod_time_ms > time_of_test_command:
    +    self.logger.warning(f"The coverage report file was not updated after the test command. file_mod_time_ms: {file_mod_time_ms}, time_of_test_command: {time_of_test_command}")
    • Apply this suggestion
    Suggestion importance[1-10]: 6
    Low
    • Update

    @qododavid qododavid force-pushed the dw/verify-report-error branch from abbbea6 to a2f6956 Compare February 20, 2025 23:38
    @EmbeddedDevops1 EmbeddedDevops1 merged commit 1c611e8 into main Feb 21, 2025
    7 checks passed
    @EmbeddedDevops1 EmbeddedDevops1 deleted the dw/verify-report-error branch February 21, 2025 00:06
    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.

    2 participants