Better Existing tests performance reporting by misrasaurabh1 · Pull Request #321 · codeflash-ai/codeflash

misrasaurabh1 · 2025-06-12T05:05:23Z

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 · 2025-06-12T05:07:04Z

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 · 2025-06-12T05:08:12Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	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
General	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

LGTM

misrasaurabh1 · 2025-06-18T19:51:39Z


                if matching_original_times and matching_optimized_times:
                    original_time = min(matching_original_times)
                    optimized_time = min(matching_optimized_times)


check for optimized_time or original_time = 0

aseembits93 · 2025-06-18T22:37:46Z

@misrasaurabh1 ready to merge

aseembits93 · 2025-06-18T22:38:16Z

This is how the PR would look like - #347

first version that does not work correctly

fec0627

misrasaurabh1 requested a review from aseembits93 June 12, 2025 05:05

github-actions Bot added the Review effort 3/5 label Jun 12, 2025

aseembits93 added 9 commits June 12, 2025 01:12

Merge branch 'main' into existing-tests-reporting

de359a1

Merge branch 'main' into existing-tests-reporting

61094f7

wip need to do in a single loop

3f1cbef

Merge remote-tracking branch 'origin/main' into existing-tests-reporting

0332d2b

works

a09d11c

improve runtimecomments

30259c0

text highlight doesnt work

0d566bf

works

30410ff

minor fixes

4a68aa0

aseembits93 self-assigned this Jun 17, 2025

aseembits93 marked this pull request as ready for review June 17, 2025 22:36

aseembits93 added 6 commits June 17, 2025 16:01

minor fixes

e202289

fix tests, correct perf calc

7b4bdd0

tests wip

2ad1029

tests work now, ready to merge

d2289e5

mypy fix

df6efe9

mypy fix

9ad40e2

aseembits93 requested a review from KRRT7 June 18, 2025 03:51

aseembits93 previously approved these changes Jun 18, 2025

View reviewed changes