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

Refactor revert #267

Merged
merged 4 commits into from
Jan 14, 2025
Merged

Refactor revert #267

merged 4 commits into from
Jan 14, 2025

Conversation

EmbeddedDevops1
Copy link
Collaborator

@EmbeddedDevops1 EmbeddedDevops1 commented Jan 13, 2025

PR Type

Enhancement, Tests, Bug fix


Description

  • Refactored CoverageProcessor into a standalone class with enhanced functionality.

  • Updated UnitTestValidator and UnitTestGenerator to integrate the new CoverageProcessor.

  • Removed deprecated coverage.processor module and its associated tests.

  • Added comprehensive unit tests for CoverageProcessor and updated existing tests for compatibility.


Changes walkthrough 📝

Relevant files
Enhancement
4 files
CoverAgent.py
Updated coverage validation logic to use `CoverageProcessor`.
+7/-7     
CoverageProcessor.py
Introduced standalone CoverageProcessor class for coverage handling.
+413/-0 
UnitTestGenerator.py
Integrated `CoverageProcessor` into `UnitTestGenerator`. 
+1/-0     
UnitTestValidator.py
Refactored `UnitTestValidator` to use `CoverageProcessor`.
+87/-32 
Cleanup
2 files
processor.py
Removed deprecated `coverage.processor` module.                   
+0/-504 
test_processor.py
Removed tests for deprecated `coverage.processor` module.
+0/-527 
Tests
5 files
test_CoverAgent.py
Updated tests to reflect `CoverageProcessor` integration.
+9/-8     
test_CoverageProcessor.py
Added unit tests for `CoverageProcessor` class.                   
+532/-0 
test_UnitTestGenerator.py
Updated tests for `UnitTestGenerator` with `CoverageProcessor`.
+1/-0     
test_UnitTestValidator.py
Updated tests for `UnitTestValidator` with `CoverageProcessor`.
+19/-18 
test_all.sh
Adjusted integration tests to align with refactored coverage handling.
+0/-1     
Configuration changes
1 files
version.txt
Rolled back version to 0.2.15.                                                     
+1/-1     

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

Copy link
Contributor

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 parse_coverage_report method contains duplicated logic for handling different coverage types. Consider extracting common patterns into helper methods to improve maintainability.

"""
Parses a code coverage report to extract covered and missed line numbers for a specific file,
and calculates the coverage percentage, based on the specified coverage report type.

Returns:
    Tuple[list, list, float]: A tuple containing lists of covered and missed line numbers, and the coverage percentage.
"""
if self.use_report_coverage_feature_flag:
    if self.coverage_type == "cobertura":
        return self.parse_coverage_report_cobertura()
    elif self.coverage_type == "lcov":
        return self.parse_coverage_report_lcov()
    elif self.coverage_type == "jacoco":
        return self.parse_coverage_report_jacoco()
    else:
        raise ValueError(f"Unsupported coverage report type: {self.coverage_type}")
else:
    if self.coverage_type == "cobertura":
        # Default behavior is to parse out a single file from the report
        return self.parse_coverage_report_cobertura(filename=os.path.basename(self.src_file_path))
    elif self.coverage_type == "lcov":
        return self.parse_coverage_report_lcov()
    elif self.coverage_type == "jacoco":
        return self.parse_coverage_report_jacoco()
    elif self.coverage_type == "diff_cover_json":
        return self.parse_json_diff_coverage_report()
    else:
        raise ValueError(f"Unsupported coverage report type: {self.coverage_type}")
Error Handling

The parse_coverage_data_for_class method lacks error handling for malformed XML data or missing attributes, which could lead to runtime errors.

def parse_coverage_data_for_class(self, cls) -> Tuple[list, list, float]:
    """
    Parses coverage data for a single class.

    Args:
        cls (Element): XML element representing the class.

    Returns:
        Tuple[list, list, float]: A tuple containing lists of covered and missed line numbers, 
                                and the coverage percentage.
    """
    lines_covered, lines_missed = [], []

    for line in cls.findall(".//line"):
        line_number = int(line.get("number"))
        hits = int(line.get("hits"))
        if hits > 0:
            lines_covered.append(line_number)
        else:
            lines_missed.append(line_number)

    total_lines = len(lines_covered) + len(lines_missed)
    coverage_percentage = (len(lines_covered) / total_lines) if total_lines > 0 else 0

    return lines_covered, lines_missed, coverage_percentage
Performance

The parse_coverage_report_cobertura method performs multiple tree traversals and set operations that could be optimized for large coverage reports.

def parse_coverage_report_cobertura(self, filename: str = None) -> Union[Tuple[list, list, float], dict]:
    """
    Parses a Cobertura XML code coverage report to extract covered and missed line numbers
    for a specific file or for all files (if filename is None). Aggregates coverage data from
    multiple <class> entries that share the same filename.

    Args:
        filename (str, optional): Filename to process. If None, process all files.

    Returns:
        If filename is provided, returns (covered_lines, missed_lines, coverage_percent).
        If filename is None, returns a dict: { filename: (covered_lines, missed_lines, coverage_percent) }.
    """
    tree = ET.parse(self.file_path)
    root = tree.getroot()

    if filename:
        # Collect coverage for all <class> elements matching the given filename
        all_covered, all_missed = [], []
        for cls in root.findall(".//class"):
            name_attr = cls.get("filename")
            if name_attr and name_attr.endswith(filename):
                c_covered, c_missed, _ = self.parse_coverage_data_for_class(cls)
                all_covered.extend(c_covered)
                all_missed.extend(c_missed)

        # Deduplicate and compute coverage
        covered_set = set(all_covered)
        missed_set = set(all_missed) - covered_set
        total_lines = len(covered_set) + len(missed_set)
        coverage_percentage = (len(covered_set) / total_lines) if total_lines else 0

        return list(covered_set), list(missed_set), coverage_percentage

    else:
        # Collect coverage for every <class>, grouping by filename
        coverage_data = {}
        file_map = {}  # filename -> ([covered], [missed])

        for cls in root.findall(".//class"):
            cls_filename = cls.get("filename")
            if cls_filename:
                c_covered, c_missed, _ = self.parse_coverage_data_for_class(cls)
                if cls_filename not in file_map:
                    file_map[cls_filename] = ([], [])
                file_map[cls_filename][0].extend(c_covered)
                file_map[cls_filename][1].extend(c_missed)

        # Convert raw lists to sets, compute coverage, store results
        for f_name, (c_covered, c_missed) in file_map.items():
            covered_set = set(c_covered)
            missed_set = set(c_missed) - covered_set
            total_lines = len(covered_set) + len(missed_set)
            coverage_percentage = (len(covered_set) / total_lines) if total_lines else 0
            coverage_data[f_name] = (list(covered_set), list(missed_set), coverage_percentage)

        return coverage_data

@EmbeddedDevops1
Copy link
Collaborator Author

⌛ Running integration tests: https://github.com/qodo-ai/qodo-cover/actions/runs/12757828357

Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Add XML parsing error handling to prevent application crashes on malformed coverage reports

Add error handling for XML parsing in parse_coverage_report_cobertura to handle
malformed XML files and prevent potential crashes.

cover_agent/CoverageProcessor.py [110-112]

 def parse_coverage_report_cobertura(self, filename: str = None) -> Union[Tuple[list, list, float], dict]:
-    tree = ET.parse(self.file_path)
-    root = tree.getroot()
+    try:
+        tree = ET.parse(self.file_path)
+        root = tree.getroot()
+    except ET.ParseError as e:
+        self.logger.error(f"Error parsing XML file {self.file_path}: {e}")
+        return ([], [], 0.0) if filename else {}
  • Apply this suggestion
Suggestion importance[1-10]: 8

Why: The suggestion adds crucial error handling for XML parsing, which could prevent application crashes in production when dealing with malformed coverage reports. This is a significant reliability improvement.

8
Add JSON parsing error handling to prevent crashes when processing malformed or missing diff coverage reports

Add input validation for the file_path parameter in parse_json_diff_coverage_report
to handle potential JSON decoding errors.

cover_agent/CoverageProcessor.py [373-374]

 def parse_json_diff_coverage_report(self) -> Tuple[List[int], List[int], float]:
-    with open(self.diff_coverage_report_path, "r") as file:
-        report_data = json.load(file)
+    try:
+        with open(self.diff_coverage_report_path, "r") as file:
+            report_data = json.load(file)
+    except (json.JSONDecodeError, FileNotFoundError) as e:
+        self.logger.error(f"Error reading JSON file {self.diff_coverage_report_path}: {e}")
+        return [], [], 0.0
  • Apply this suggestion
Suggestion importance[1-10]: 8

Why: Adding error handling for JSON parsing is essential for application stability, as it prevents crashes when dealing with malformed or missing diff coverage reports. This is a critical defensive programming practice.

8
Improve error handling for edge cases in coverage calculation to prevent runtime errors

Add error handling for potential ZeroDivisionError earlier in the code flow, before
attempting the division operation. This will provide clearer error messages and
prevent potential crashes.

cover_agent/UnitTestValidator.py [719-722]

-try:
+if total_lines == 0:
+    self.logger.error("No lines to calculate coverage from. Check if the source files are empty or if coverage data is being collected correctly.")
+    percentage_covered = 0
+else:
     percentage_covered = total_lines_covered / total_lines
-except ZeroDivisionError:
-    self.logger.error(f"ZeroDivisionError: Attempting to perform total_lines_covered / total_lines: {total_lines_covered} / {total_lines}.")
-    percentage_covered = 0
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why: The suggestion improves error handling by checking for zero lines before division, providing more descriptive error messages and preventing runtime errors. This makes the code more robust and maintainable.

7
Initialize all required variables at the start of the method to prevent potential reference errors

Initialize coverage_percentages dictionary before the conditional block to prevent
potential KeyError when accessing coverage_percentages in the else branches.

cover_agent/UnitTestValidator.py [694-696]

 def post_process_coverage_report(self, time_of_test_command):
     coverage_percentages = {}
+    percentage_covered = 0
     if self.use_report_coverage_feature_flag:
  • Apply this suggestion
Suggestion importance[1-10]: 6

Why: The suggestion helps prevent potential KeyError exceptions by initializing all variables at the start of the method. This is a good defensive programming practice that improves code reliability.

6
Add explicit zero check when calculating coverage percentage to prevent potential division by zero errors

Add validation for division by zero in parse_coverage_report_cobertura when
calculating coverage percentage.

cover_agent/CoverageProcessor.py [139-140]

 total_lines = len(covered_set) + len(missed_set)
-coverage_percentage = (len(covered_set) / total_lines) if total_lines else 0
+coverage_percentage = (len(covered_set) / total_lines) if total_lines > 0 else 0.0
  • Apply this suggestion
Suggestion importance[1-10]: 2

Why: The suggestion is technically valid but offers minimal improvement, as the existing code already handles the zero case with the 'if total_lines else 0' condition. The change to 'if total_lines > 0' is mostly stylistic.

2

@EmbeddedDevops1 EmbeddedDevops1 merged commit e6c790c into main Jan 14, 2025
7 checks passed
@EmbeddedDevops1 EmbeddedDevops1 deleted the refactor-revert branch January 14, 2025 17:30
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.

UnitTestValidator.get_coverage method behavior has changed bring back cobertura and jacoco fixes
2 participants