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

Revert "Enhanced coverage processing (#2)" #259

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

EmbeddedDevops1
Copy link
Collaborator

@EmbeddedDevops1 EmbeddedDevops1 commented Jan 7, 2025

User description

Reverts #254


PR Type

Enhancement, Bug fix, Tests


Description

  • Reverted changes from a previous enhancement to coverage processing.

  • Introduced a new CoverageReportFilter class for filtering reports by file patterns.

  • Refactored CoverageData and CoverageReport structures, removing is_target_file and simplifying logic.

  • Updated tests to align with the refactored coverage processing logic.


Changes walkthrough 📝

Relevant files
Enhancement
UnitTestValidator.py
Added file pattern support in coverage processing               

cover_agent/UnitTestValidator.py

  • Added file_pattern parameter to post_process_coverage_report.
  • Adjusted coverage processing to support filtering by file patterns.
  • +1/-0     
    processor.py
    Refactored coverage processing and added filtering             

    cover_agent/coverage/processor.py

  • Removed is_target_file from CoverageData.
  • Refactored CoverageReport to simplify filtering logic.
  • Added CoverageReportFilter class for filtering reports by file
    patterns.
  • Updated JaCoCo processor to handle XML and CSV formats more robustly.
  • +137/-118
    Tests
    test_processor.py
    Updated tests for refactored coverage processing                 

    tests/coverage/test_processor.py

  • Added tests for CoverageReportFilter class.
  • Updated tests to reflect changes in CoverageData and CoverageReport.
  • Removed tests related to is_target_file.
  • +33/-131
    test_UnitTestValidator.py
    Adjusted UnitTestValidator tests for refactored coverage 

    tests/test_UnitTestValidator.py

  • Updated tests to align with refactored CoverageProcessor.
  • Adjusted mock data to reflect removal of is_target_file.
  • +10/-9   
    Configuration changes
    version.txt
    Reverted version number                                                                   

    cover_agent/version.txt

    • Reverted version number from 0.2.14 to 0.2.13.
    +1/-1     

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

    @EmbeddedDevops1 EmbeddedDevops1 merged commit 7405a29 into main Jan 7, 2025
    1 check passed
    @EmbeddedDevops1 EmbeddedDevops1 deleted the revert-254-feature/refactor-cp branch January 7, 2025 04:57
    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 7, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Code Duplication

    The coverage calculation logic is duplicated across different processor classes. Consider extracting common coverage calculation into a shared utility method.

    self._is_coverage_valid(time_of_test_command=time_of_test_command)
    coverage = self.parse_coverage_report()
    report = CoverageReport(0.0, coverage)
    if coverage:
        total_covered = sum(cov.covered for cov in coverage.values())
        total_missed = sum(cov.missed for cov in coverage.values())
        total_lines = total_covered + total_missed
        report.total_coverage = (float(total_covered) / float(total_lines)) if total_lines > 0 else 0.0
    return report
    Error Handling

    The JaCoCo processor does not handle missing package or class names gracefully. Consider adding proper error handling and logging for these cases.

    def _extract_package_and_class_java(self):
        package_pattern = re.compile(r"^\s*package\s+([\w\.]+)\s*;.*$")
        class_pattern = re.compile(r"^\s*public\s+class\s+(\w+).*")
    
        package_name = ""
        class_name = ""
        try:
            with open(self.src_file_path, "r") as file:
                for line in file:
                    if not package_name:  # Only match package if not already found
                        package_match = package_pattern.match(line)
                        if package_match:
                            package_name = package_match.group(1)
    
                    if not class_name:  # Only match class if not already found
                        class_match = class_pattern.match(line)
                        if class_match:
                            class_name = class_match.group(1)
    
                    if package_name and class_name:  # Exit loop if both are found
                        break
        except (FileNotFoundError, IOError) as e:
            self.logger.error(f"Error reading file {self.src_file_path}: {e}")
            raise
    
        return package_name, class_name

    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 7, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add error handling for XML parsing to prevent crashes on malformed input

    Add error handling for potential XML parsing errors in _parse_jacoco_xml method. XML
    parsing can fail due to malformed input or missing attributes.

    cover_agent/coverage/processor.py [270-276]

     def _parse_jacoco_xml(
         self, class_name: str
     ) -> tuple[int, int]:
         """Parses a JaCoCo XML code coverage report to extract covered and missed line numbers for a specific file."""
    -    tree = ET.parse(self.file_path)
    -    root = tree.getroot()
    -    sourcefile = root.find(f".//sourcefile[@name='{class_name}.java']")
    +    try:
    +        tree = ET.parse(self.file_path)
    +        root = tree.getroot()
    +        sourcefile = root.find(f".//sourcefile[@name='{class_name}.java']")
    +    except ET.ParseError as e:
    +        self.logger.error(f"Failed to parse JaCoCo XML report: {e}")
    +        raise
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion adds important error handling for XML parsing which could prevent application crashes in production. XML parsing errors are common and should be handled gracefully.

    7
    Add input validation to prevent filtering with invalid patterns

    Add input validation for file_pattern parameter in
    CoverageReportFilter.filter_report() to handle None, empty strings and invalid
    patterns.

    cover_agent/coverage/processor.py [375-377]

     def filter_report(self, report: CoverageReport, file_pattern: str) -> CoverageReport:
         """
         Filters the coverage report based on the specified file pattern.
    +    
    +    Args:
    +        report (CoverageReport): The coverage report to filter.
    +        file_pattern (str): The file pattern to filter by. Must not be empty.
    +        
    +    Raises:
    +        ValueError: If file_pattern is None or empty.
    +    """
    +    if not file_pattern:
    +        raise ValueError("file_pattern must not be None or empty")
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The suggestion adds necessary input validation to prevent potential issues with invalid file patterns. This would improve the robustness of the filtering functionality.

    6
    Add error handling for division by zero in coverage calculations

    Add error handling for potential division by zero in process_coverage_report when
    calculating coverage percentage.

    cover_agent/coverage/processor.py [110-111]

    -total_lines = total_covered + total_missed
    -report.total_coverage = (float(total_covered) / float(total_lines)) if total_lines > 0 else 0.0
    +try:
    +    total_lines = total_covered + total_missed
    +    report.total_coverage = (float(total_covered) / float(total_lines)) if total_lines > 0 else 0.0
    +except ZeroDivisionError:
    +    self.logger.warning("No lines found for coverage calculation")
    +    report.total_coverage = 0.0
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: While the code already has a check for total_lines > 0, adding explicit error handling provides extra safety. However, the impact is low since the existing check already prevents division by zero.

    4

    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.

    1 participant