Skip to content

Conversation

@aseembits93
Copy link
Contributor

@aseembits93 aseembits93 commented Jul 21, 2025

PR Type

Enhancement, Tests


Description

  • Add replay and concolic tests support

  • Separate test tables for existing/replay/concolic

  • Reformat speedup labels from "✅x%" to "x%✅"

  • Extend unit tests for new categorization and formatting


Diagram Walkthrough

flowchart LR
  A["process_review"] -- "fetch tests & runtimes" --> B["existing_tests_source_for"]
  B -- "return existing, replay, concolic" --> C["cfapi.suggest_changes/create_pr payload"]
  C -- "include separated test tables" --> D["PR comment"]
Loading

File Walkthrough

Relevant files
Enhancement
cfapi.py
Add replay and concolic tests params                                         

codeflash/api/cfapi.py

  • Added replay_tests and concolic_tests parameters
  • Updated suggest_changes and create_pr payloads
+8/-0     
function_optimizer.py
Propagate replay/concolic tests data                                         

codeflash/optimization/function_optimizer.py

  • Unpacked triple return from existing_tests_source_for
  • Propagated replay_tests and concolic_tests into data dict
+3/-1     
create_pr.py
Refactor test source categorization                                           

codeflash/result/create_pr.py

  • Refactored existing_tests_source_for to return three tables
  • Categorized rows into existing, replay, concolic
  • Separated tabulate outputs and conditional emptiness
  • Updated check_create_pr to accept new args
+73/-12 
Tests
test_existing_tests_source_for.py
Update tests for new test tables                                                 

tests/test_existing_tests_source_for.py

  • Updated tests to unpack triple return values
  • Fixed speedup formatting assertions to x%✅
  • Added tests for replay/concolic categorization
  • Introduced mock classes and temp_project_dir helper
+266/-32

@aseembits93 aseembits93 changed the title todo modify tests, modify js side todo modify tests Jul 21, 2025
@aseembits93 aseembits93 changed the title todo modify tests PR comment will contain deltailed information of existing, replay and concolic tests in separate tables Jul 21, 2025
@github-actions github-actions bot changed the title PR comment will contain deltailed information of existing, replay and concolic tests in separate tables todo modify tests Jul 21, 2025
@github-actions
Copy link

github-actions bot commented Jul 21, 2025

PR Reviewer Guide 🔍

(Review updated until commit ab43ff0)

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

Backward Compatibility

Adding new parameters replay_tests and concolic_tests changes the function signature; ensure all callers, documentation, and client integrations are updated and backward compatibility is maintained.

    replay_tests: str,
    concolic_tests: str,
) -> Response:
Missing Table Titles

Separated tables for existing, replay, and concolic tests lack descriptive titles or headers to distinguish them in the PR comment.

                )
output_existing += tabulate(  # type: ignore[no-untyped-call]
    headers=headers, tabular_data=rows_existing, tablefmt="pipe", colglobalalign=None, preserve_whitespace=True
)
output_existing += "\n"
if len(rows_existing) == 0:
    output_existing = ""
output_concolic += tabulate(  # type: ignore[no-untyped-call]
    headers=headers, tabular_data=rows_concolic, tablefmt="pipe", colglobalalign=None, preserve_whitespace=True
)
output_concolic += "\n"
if len(rows_concolic) == 0:
    output_concolic = ""
output_replay += tabulate(  # type: ignore[no-untyped-call]
    headers=headers, tabular_data=rows_replay, tablefmt="pipe", colglobalalign=None, preserve_whitespace=True
)
output_replay += "\n"
if len(rows_replay) == 0:
    output_replay = ""
return output_existing, output_replay, output_concolic
Unsorted Rows

Replay and concolic test rows are not explicitly sorted, which may lead to inconsistent ordering across runs.

    if "__replay_test_" in str(print_filename):
        rows_replay.append(
            [
                f"`{print_filename}::{qualified_name}`",
                f"{print_original_runtime}",
                f"{print_optimized_runtime}",
                f"{perf_gain}%⚠️",
            ]
        )
    elif "codeflash_concolic" in str(print_filename):
        rows_concolic.append(
            [
                f"`{print_filename}::{qualified_name}`",
                f"{print_original_runtime}",
                f"{print_optimized_runtime}",
                f"{perf_gain}%⚠️",
            ]
        )
    else:
        rows_existing.append(
            [
                f"`{print_filename}::{qualified_name}`",
                f"{print_original_runtime}",
                f"{print_optimized_runtime}",
                f"{perf_gain}%⚠️",
            ]
        )
elif "__replay_test_" in str(print_filename):

@github-actions
Copy link

github-actions bot commented Jul 21, 2025

PR Code Suggestions ✨

Latest suggestions up to ab43ff0
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix inverted emojis

Invert the emoji logic so that performance improvements (when greater is true) use
the ✅ emoji and regressions (else branch) use ⚠️, aligning with the semantics
expected by the tests.

codeflash/result/create_pr.py [105-158]

 if greater:
     ...
-    f"{perf_gain}%⚠️",
+    f"{perf_gain}%✅",
 ...
 else:
     ...
-    f"{perf_gain}%✅",
+    f"{perf_gain}%⚠️",
Suggestion importance[1-10]: 9

__

Why: The emojis for speedup and regression are swapped in the new logic, causing improvements to show ⚠️ and regressions ✅, which contradicts the tests’ expectations.

High

Previous suggestions

Suggestions up to commit 587c80c
CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix return type and empty return

The function now returns three separate strings but is still annotated to return
list[str] and returns only one value on empty input. Update the signature to reflect
a tuple of three strings and return three empty strings in the early-exit branch.

codeflash/result/create_pr.py [31-40]

 def existing_tests_source_for(
     function_qualified_name_with_modules_from_root: str,
     function_to_tests: dict[str, set[FunctionCalledInTest]],
     test_cfg: TestConfig,
     original_runtimes_all: dict[InvocationId, list[int]],
     optimized_runtimes_all: dict[InvocationId, list[int]],
-) -> list[str]:
+) -> tuple[str, str, str]:
     test_files = function_to_tests.get(function_qualified_name_with_modules_from_root)
     if not test_files:
-        return ""
+        return "", "", ""
Suggestion importance[1-10]: 8

__

Why: The function’s signature and early‐exit return only a single string but the implementation now returns three strings, so updating the signature to tuple[str, str, str] and returning three empty strings fixes a critical type and runtime mismatch.

Medium
General
Use camelCase JSON keys

The JSON payload uses snake_case keys while other fields use camelCase. Rename these
keys to replayTests and concolicTests for consistency with the API contract.

codeflash/api/cfapi.py [147-148]

-"replay_tests":replay_tests,
-"concolic_tests":concolic_tests
+"replayTests": replay_tests,
+"concolicTests": concolic_tests
Suggestion importance[1-10]: 5

__

Why: Renaming replay_tests and concolic_tests to replayTests and concolicTests aligns them with the camelCase convention used by other payload fields, improving API consistency.

Low

@aseembits93 aseembits93 changed the title todo modify tests PR comment will contain deltailed information of existing, replay and concolic tests in separate tables Jul 21, 2025
@aseembits93 aseembits93 changed the title PR comment will contain deltailed information of existing, replay and concolic tests in separate tables PR comment will contain detailed information of existing, replay and concolic tests in separate tables Aug 8, 2025
@aseembits93 aseembits93 marked this pull request as ready for review August 11, 2025 19:06
@github-actions github-actions bot changed the title PR comment will contain detailed information of existing, replay and concolic tests in separate tables PR comment will contain detailed information of existing, replay and concolic tests in separate tables Aug 11, 2025
@github-actions
Copy link

Persistent review updated to latest commit ab43ff0

@aseembits93 aseembits93 enabled auto-merge August 11, 2025 23:47
@aseembits93 aseembits93 requested a review from KRRT7 August 11, 2025 23:48
@misrasaurabh1 misrasaurabh1 disabled auto-merge August 11, 2025 23:51
@misrasaurabh1 misrasaurabh1 merged commit ad7dbe4 into main Aug 11, 2025
18 of 21 checks passed
@aseembits93 aseembits93 deleted the existing-test-pr-comment 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.

2 participants