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

Enhanced coverage processing (#2) #254

Merged
merged 3 commits into from
Jan 7, 2025

Conversation

coderustic
Copy link
Contributor

@coderustic coderustic commented Jan 5, 2025

User description


PR Type

Enhancement, Tests


Description

  • Enhanced coverage processing to handle file path conflicts.

  • Added is_target_file attribute for accurate coverage matching.

  • Refactored coverage processors to improve modularity and accuracy.

  • Updated and expanded test cases for coverage processors.


Changes walkthrough 📝

Relevant files
Enhancement
UnitTestValidator.py
Simplify coverage report processing in UnitTestValidator 

cover_agent/UnitTestValidator.py

  • Removed unused file_pattern parameter in coverage processing.
  • Adjusted logging for coverage percentage reporting.
  • +0/-1     
    processor.py
    Refactor coverage processors and add target file filtering

    cover_agent/coverage/processor.py

  • Introduced is_target_file attribute in CoverageData.
  • Added filter_to_target_coverage method in CoverageReport.
  • Refactored coverage processors for better handling of file paths.
  • Improved support for Cobertura, LCOV, and JaCoCo formats.
  • +118/-137
    Tests
    test_processor.py
    Expand and update tests for coverage processors                   

    tests/coverage/test_processor.py

  • Updated tests to validate is_target_file functionality.
  • Added tests for multiple file coverage scenarios.
  • Enhanced test coverage for Cobertura, LCOV, and JaCoCo processors.
  • +131/-33
    test_UnitTestValidator.py
    Update UnitTestValidator tests for new coverage logic       

    tests/test_UnitTestValidator.py

  • Updated tests to reflect changes in CoverageReport structure.
  • Adjusted mock data to include is_target_file attribute.
  • +9/-10   
    jacocoTestReport.xml
    Add sample JaCoCo XML report for tests                                     

    sample-reports/jacocoTestReport.xml

    • Added a sample JaCoCo XML report for testing purposes.
    +108/-0 
    lcov.info
    Add sample LCOV report for tests                                                 

    sample-reports/lcov.info

    • Added a sample LCOV report for testing purposes.
    +452/-0 

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

    @coderustic
    Copy link
    Contributor Author

    @qododavid @EmbeddedDevops1 Appreciate your review.

    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 5, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    251 - Fully compliant

    Compliant requirements:

    • Fix bug where coverage report matching uses partial file name instead of full path
    • Ensure correct coverage report is matched when multiple files have same name endings
    • Prevent tests from being discarded due to incorrect coverage matching
    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Complexity

    The refactored coverage processors contain complex logic for handling file paths and coverage data. Validate that edge cases are properly handled, especially for nested packages and files with similar names.

    def parse_coverage_report(self) -> Dict[str, CoverageData]:
        tree = ET.parse(self.file_path)
        root = tree.getroot()
        coverage = {}
        for package in root.findall(".//package"):
            # Package name could be '.' if the class is in the default package
            # Eg: <package name="." line-rate="0.8143" branch-rate="0" complexity="0">
            # In such cases, lets use default as the package name.
            package_name = package.get("name", ".")
            if package_name == ".":
                package_name = "default"
            for cls in package.findall(".//class"):
                # In languages where Class is not a first class citizen,
                # the class name is set to the file name as you can see
                # in the below example from the Cobertura XML report.
                # Usually this could be your util files. So we are good
                # to consier name as the key for the CoverageData.
                # Eg: <class name="utils.py" filename="utils.py" complexity="0" line-rate="0.8794" branch-rate="0">
                class_name = cls.get("name", "")
                fully_qualified_name = f"{package_name}.{class_name}".strip('.')
                coverage[fully_qualified_name] = self._parse_class_coverage(cls)
        return coverage
    Error Handling

    The coverage processors could benefit from more robust error handling and validation, particularly around file path resolution and coverage data parsing.

    def process_coverage_report(self, time_of_test_command: int) -> CoverageReport:
        """
        Processes the coverage report and returns the coverage data.
        """
        self._is_coverage_valid(time_of_test_command=time_of_test_command)
        coverage_data = self.parse_coverage_report()
        total_covered = sum(cov.covered for cov in coverage_data.values())
        total_missed = sum(cov.missed for cov in coverage_data.values())
        total_lines = total_covered + total_missed
        total_coverage = (total_covered / total_lines) if total_lines > 0 else 0.0
        return CoverageReport(total_coverage, coverage_data)

    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 5, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add validation to prevent division by zero operations

    Add error handling for division by zero in the calculate function. Currently the
    function returns without proper error handling when the operation is division and
    the second operand is zero.

    app/calculator.cpp [5-8]

     double calculate(double a, double b, const std::string& operation) {
         if (operation == "+") {
             return a + b;
    -}
    +    } else if (operation == "/") {
    +        if (b == 0) {
    +            throw std::invalid_argument("Division by zero");
    +        }
    +        return a / b;
    +    }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion addresses a critical safety issue by adding proper error handling for division by zero, which could cause program crashes. This is a significant improvement for code robustness and reliability.

    9
    Handle empty target coverage case to prevent division by zero error

    Add error handling for potential division by zero in filter_to_target_coverage()
    when target_coverage is empty. Currently, if no target files are found, the division
    by zero could crash the application.

    cover_agent/coverage/processor.py [64-66]

    +if not target_coverage:
    +    return CoverageReport(0.0, {})
     total_lines = sum(len(cov.covered_lines) + len(cov.missed_lines) for cov in target_coverage.values())
     total_coverage = (sum(len(cov.covered_lines) for cov in target_coverage.values()) / total_lines) if total_lines > 0 else 0.0
     return CoverageReport(total_coverage, target_coverage)
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a critical edge case that could cause runtime errors. Adding explicit handling for empty target coverage prevents potential application crashes.

    8
    Add error handling for XML parsing to prevent crashes from malformed data

    Add error handling for potential XML parsing errors in _parse_xml() method of
    JacocoProcessor. Currently, if the XML is malformed or missing required attributes,
    it could raise unhandled exceptions.

    cover_agent/coverage/processor.py [279-280]

    -missed += int(counter.attrib.get('missed', 0))
    -covered += int(counter.attrib.get('covered', 0))
    +try:
    +    missed += int(counter.attrib.get('missed', 0))
    +    covered += int(counter.attrib.get('covered', 0))
    +except (ValueError, TypeError) as e:
    +    self.logger.error(f"Error parsing coverage values: {e}")
    +    missed, covered = 0, 0
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds crucial error handling for XML parsing, preventing potential crashes from malformed data and improving the application's reliability.

    8
    Validate source file path before comparison to prevent potential AttributeError

    Add input validation for src_file_path in JacocoProcessor's _parse_xml() method.
    Currently, if src_file_path is None or empty, the target file check could raise an
    AttributeError.

    cover_agent/coverage/processor.py [286-287]

    -if f"{src_path}/{class_name}" == self.src_file_path:
    +if self.src_file_path and f"{src_path}/{class_name}" == self.src_file_path:
         is_target = True
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion prevents potential AttributeError by validating the src_file_path before comparison, improving the code's robustness and error handling.

    7

    * While earlier PR[qodo-ai#230] managed to breakdown processing
      code into a class hierarechy, there wasnt any changes
      made to the code. This PR brings in enhancements to
      coverage processing where coverage data is stored by
      entity (Class or File).
    
    * Coverage data is stored using a FQDN so that conflicts
      are taken care. This closes[qodo-ai#251]
    
    * Earlier PR broke the behaviour of the agent that only
      target file coverage is considered if the global coverage
      flag is not set by the user, this PR fixes it to bring
      back the original behaviour.
    @coderustic coderustic force-pushed the feature/refactor-cp branch from 340f16c to 86dbebe Compare January 7, 2025 04:23
    @EmbeddedDevops1 EmbeddedDevops1 merged commit fa343e2 into qodo-ai:main Jan 7, 2025
    7 checks passed
    EmbeddedDevops1 added a commit that referenced this pull request Jan 7, 2025
    EmbeddedDevops1 added a commit that referenced this pull request Jan 7, 2025
    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.

    Misidentification of relevant coverage report causes tests to be thrown away
    2 participants