Skip to content

Conversation

@misrasaurabh1
Copy link
Contributor

@misrasaurabh1 misrasaurabh1 commented Jun 12, 2025

PR Type

Enhancement, Tests

$\textcolor{red}{\rightarrow}$ $\textcolor{green}{\rightarrow}$ $\rightarrow$


Description

  • Include timing info in existing tests report

  • Group existing tests by test file

  • Pass test results in function optimizer

  • Add tests for existing_tests_source_for


Changes walkthrough 📝

Relevant files
Enhancement
function_optimizer.py
Update optimize_function to pass test results                       

codeflash/optimization/function_optimizer.py

  • Moved existing_tests_source_for call into optimization branch
  • Passed original and optimized TestResults parameters
  • Removed legacy early call to fetch existing tests
  • +7/-6     
    create_pr.py
    Extend existing_tests_source_for with timing                         

    codeflash/result/create_pr.py

  • Added original_test_results and optimized_test_results params
  • Grouped tests per file and sorted output
  • Included timing comparisons for each test function
  • Preserved backward compatibility without timing info
  • +59/-2   
    Tests
    test_existing_tests_source_for.py
    Add comprehensive tests for existing_tests_source_for       

    tests/test_existing_tests_source_for.py

  • Added tests for timing and grouping logic
  • Covered backward compatibility and missing results
  • Verified sorting and format_time calls
  • Used pytest fixtures for sample data
  • +645/-0 

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @github-actions
    Copy link

    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

    Missing Import

    The function signature references Optional and Path without showing their imports, which may lead to NameError at runtime.

        original_test_results: Optional[TestResults] = None,
        optimized_test_results: Optional[TestResults] = None,
    ) -> str:
        test_files = function_to_tests.get(function_qualified_name_with_modules_from_root)
    Partial Results Handling

    The logic for including tests when optimized results are missing only checks for any runtime match and can include partial data, but tests expect exclusion when some invocation data is absent.

    for invocation_id, runtimes in original_runtime_by_test.items():
        if invocation_id.test_function_name == test_function_name:
            original_runtimes.extend(runtimes)
    
    for invocation_id, runtimes in optimized_runtime_by_test.items():
        if invocation_id.test_function_name == test_function_name:
            optimized_runtimes.extend(runtimes)
    
    if original_runtimes and optimized_runtimes:
    Unused Variable

    The newly assigned existing_tests variable in optimize_function is not used later, so existing test insights may not be incorporated into the PR output.

    existing_tests = existing_tests_source_for(
        self.function_to_optimize.qualified_name_with_modules_from_root(self.project_root),
        function_to_all_tests,
        tests_root=self.test_cfg.tests_root,
        original_test_results=original_code_baseline.benchmarking_test_results,
        optimized_test_results=best_optimization.winning_benchmarking_test_results,
    )

    @github-actions
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Pass existing tests into PR creation

    The existing_tests value is never passed into check_create_pr, so the new test
    report will be computed but not used. Include it in the call to ensure existing
    tests data appear in the PR.

    codeflash/optimization/function_optimizer.py [366-377]

     existing_tests = existing_tests_source_for(
         self.function_to_optimize.qualified_name_with_modules_from_root(self.project_root),
         function_to_all_tests,
         tests_root=self.test_cfg.tests_root,
         original_test_results=original_code_baseline.benchmarking_test_results,
         optimized_test_results=best_optimization.winning_benchmarking_test_results,
     )
    +if concolic_test_str:
    +    generated_tests_str += "\n\n" + concolic_test_str
    +
     check_create_pr(
    +    # ... other args ...
    +    existing_tests=existing_tests,
    +)
    Suggestion importance[1-10]: 8

    __

    Why: The computed existing_tests is never used in the check_create_pr call, which would omit existing test data from the PR; passing it fixes a functionality gap.

    Medium
    General
    Move format_time import up

    Importing format_time inside the loop causes repeated imports and reduces
    readability. Move the import to the top of the function to avoid runtime overhead
    and clarify dependencies.

    codeflash/result/create_pr.py [68-80]

    +from codeflash.code_utils.time_utils import format_time
    +# ...
     for invocation_id, runtimes in optimized_runtime_by_test.items():
         if invocation_id.test_function_name == test_function_name:
             optimized_runtimes.extend(runtimes)
     
         if original_runtimes and optimized_runtimes:
    -        # Use minimum timing like the generated tests function does
             original_time = min(original_runtimes)
             optimized_time = min(optimized_runtimes)
    -
    -        from codeflash.code_utils.time_utils import format_time
     
             original_str = format_time(original_time)
             optimized_str = format_time(optimized_time)
    Suggestion importance[1-10]: 4

    __

    Why: Moving the format_time import out of the loop reduces repeated imports and improves readability, but has only minor performance impact.

    Low
    Simplify grouping with defaultdict

    The grouping logic can be simplified with defaultdict(list), which makes the code
    more concise and eliminates the explicit key-check.

    codeflash/result/create_pr.py [38-45]

    -test_files_grouped = {}
    +from collections import defaultdict
    +test_files_grouped = defaultdict(list)
     for test_file in test_files:
         file_path = Path(test_file.tests_in_file.test_file)
         relative_path = str(file_path.relative_to(tests_root))
    -
    -    if relative_path not in test_files_grouped:
    -        test_files_grouped[relative_path] = []
         test_files_grouped[relative_path].append(test_file)
    Suggestion importance[1-10]: 4

    __

    Why: Using defaultdict(list) makes the grouping logic more concise and readable, but it's a stylistic change without functional impact.

    Low

    @aseembits93 aseembits93 self-assigned this Jun 17, 2025
    @aseembits93 aseembits93 marked this pull request as ready for review June 17, 2025 22:36
    @aseembits93 aseembits93 requested a review from KRRT7 June 18, 2025 03:51
    aseembits93
    aseembits93 previously approved these changes Jun 18, 2025
    Copy link
    Contributor

    @aseembits93 aseembits93 left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    LGTM


    if matching_original_times and matching_optimized_times:
    original_time = min(matching_original_times)
    optimized_time = min(matching_optimized_times)
    Copy link
    Contributor Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    check for optimized_time or original_time = 0

    @aseembits93
    Copy link
    Contributor

    @misrasaurabh1 ready to merge

    @aseembits93
    Copy link
    Contributor

    This is how the PR would look like - #347

    @misrasaurabh1 misrasaurabh1 requested a review from KRRT7 June 18, 2025 22:48
    @misrasaurabh1 misrasaurabh1 merged commit 8c5a46b into main Jun 18, 2025
    16 checks passed
    @aseembits93 aseembits93 deleted the existing-tests-reporting branch August 17, 2025 19:26
    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.

    4 participants