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

Fix regression on Jacoco coverage & Corbertura #264

Merged
merged 4 commits into from
Jan 12, 2025

Conversation

coderustic
Copy link
Contributor

@coderustic coderustic commented Jan 11, 2025

User description

  • During refactoring of coverage processing, there were
    few regressions that were unintentional, this PR is to
    fix them by brining the overwritten changes back.

  • On Jacoco add support for Kotlin and change to use
    mi & nr.

  • On Corbertura, merge coverage by file name if the report
    contains multiple references by file name


PR Type

Bug fix, Tests


Description

  • Fixed regression in Jacoco and Cobertura coverage processing.

  • Added support for Kotlin in Jacoco coverage reports.

  • Enhanced merging of Cobertura coverage by file name.

  • Updated tests to validate new coverage processing logic.


Changes walkthrough 📝

Relevant files
Bug fix
processor.py
Fix and enhance coverage processing logic                               

cover_agent/coverage/processor.py

  • Fixed regression in Cobertura coverage merging logic.
  • Added support for Kotlin in Jacoco coverage processing.
  • Enhanced Jacoco XML parsing to handle Kotlin and Java files.
  • Introduced _merge_coverage_data method for Cobertura.
  • +65/-14 
    Tests
    test_processor.py
    Update and add tests for coverage processing                         

    tests/coverage/test_processor.py

  • Updated tests for Cobertura coverage merging logic.
  • Added tests for Kotlin support in Jacoco processing.
  • Enhanced test cases for missed and covered lines parsing.
  • Verified handling of empty and malformed coverage reports.
  • +155/-7 

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

    Copy link
    Contributor

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Error Handling

    The _extract_package_and_class_kotlin method should handle cases where package or class name cannot be found in the source file. Currently it may return empty strings without any warning.

    def _extract_package_and_class_kotlin(self):
        package_pattern = re.compile(r"^\s*package\s+([\w.]+)\s*(?:;)?\s*(?://.*)?$")
        class_pattern = re.compile(r"^\s*(?:public|internal|abstract|data|sealed|enum|open|final|private|protected)*\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
    Code Duplication

    The package and class name extraction logic is very similar between Kotlin and Java. Consider refactoring to reduce code duplication and improve maintainability.

    def _extract_package_and_class_kotlin(self):
        package_pattern = re.compile(r"^\s*package\s+([\w.]+)\s*(?:;)?\s*(?://.*)?$")
        class_pattern = re.compile(r"^\s*(?:public|internal|abstract|data|sealed|enum|open|final|private|protected)*\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
    
    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+).*")

    Copy link
    Contributor

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Fix incorrect method name reference that would cause runtime error

    The extract_package_and_class_java() method call in the else branch has a typo
    (missing underscore prefix), which will cause an AttributeError.

    cover_agent/coverage/processor.py [246-247]

     else:
         self.logger.warn(f"Unsupported Bytecode Language: {source_file_extension}. Using default Java logic.")
    -    package_name, class_name = self.extract_package_and_class_java()
    +    package_name, class_name = self._extract_package_and_class_java()
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: This suggestion fixes a critical bug where a typo in the method name would cause an AttributeError, completely breaking the fallback logic for unsupported file types.

    10
    Add validation for required package and class name extraction to prevent silent failures

    Add error handling for the case when both package name and class name are empty
    strings after extraction. Currently, the code proceeds with empty values which could
    cause issues in coverage reporting.

    cover_agent/coverage/processor.py [240-244]

     package_name, class_name = "",""
     if source_file_extension == 'java':
         package_name, class_name = self._extract_package_and_class_java()
     elif source_file_extension == 'kt':
         package_name, class_name = self._extract_package_and_class_kotlin()
    +if not package_name or not class_name:
    +    raise ValueError(f"Failed to extract package and class names from {self.src_file_path}")
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a critical issue where empty package and class names could silently propagate through the system, potentially causing data integrity issues in coverage reporting.

    8
    Prevent duplicate line numbers when merging coverage data

    The _merge_coverage_data method directly adds lists of line numbers which could lead
    to duplicate entries. Use sets to ensure unique line numbers.

    cover_agent/coverage/processor.py [173-174]

     def _merge_coverage_data(self, existing_coverage: CoverageData, new_coverage: CoverageData) -> CoverageData:
    -    covered_lines = existing_coverage.covered_lines + new_coverage.covered_lines
    -    missed_lines = existing_coverage.missed_lines + new_coverage.missed_lines
    +    covered_lines = list(set(existing_coverage.covered_lines + new_coverage.covered_lines))
    +    missed_lines = list(set(existing_coverage.missed_lines + new_coverage.missed_lines))
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves data accuracy by preventing duplicate line numbers in coverage data, which could otherwise lead to inflated coverage metrics.

    7

    @coderustic
    Copy link
    Contributor Author

    Here is the integration tests -> https://github.com/coderustic/cover-agent/actions/runs/12728121454

    @EmbeddedDevops1 EmbeddedDevops1 merged commit f46980f into qodo-ai:main Jan 12, 2025
    7 checks passed
    EmbeddedDevops1 added a commit that referenced this pull request Jan 13, 2025
    EmbeddedDevops1 added a commit that referenced this pull request Jan 14, 2025
    * Revert "Fix regression on Jacoco coverage & Corbertura (#264)"
    
    This reverts commit f46980f.
    
    * Revert "Removing bad reference to self.last_coverage_percentages causing necessary failure logs (#262)"
    
    This reverts commit 1b1a9ad.
    
    * Revert "Refactored coverage processor in to class hierarchy (#230)"
    
    This reverts commit 3496069.
    
    * Incrementing version.
    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