Skip to content

Conversation

@KRRT7
Copy link
Contributor

@KRRT7 KRRT7 commented Sep 23, 2025

PR Type

Bug fix, Enhancement


Description

  • Replace import with constant usage

  • Remove unused pytest duration args

  • Set default benchmarking runtime constant


Diagram Walkthrough

flowchart LR
  TR["test_runner.py"]
  CC["config_consts.TOTAL_LOOPING_TIME_EFFECTIVE"]
  PT["Pytest args cleanup"]
  TR -- "import constant" --> CC
  TR -- "remove --codeflash_seconds" --> PT
  TR -- "default runtime for benchmarking" --> CC
Loading

File Walkthrough

Relevant files
Bug fix
test_runner.py
Replace function import and clean pytest args                       

codeflash/verification/test_runner.py

  • Import TOTAL_LOOPING_TIME_EFFECTIVE instead of function.
  • Remove pytest_target_runtime_seconds from two functions.
  • Drop --codeflash_seconds from pytest args.
  • Use TOTAL_LOOPING_TIME_EFFECTIVE as default for benchmarking.
+2/-6     

misrasaurabh1
misrasaurabh1 previously approved these changes Sep 23, 2025
@github-actions
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

Possible Issue

Using TOTAL_LOOPING_TIME_EFFECTIVE as a default only for benchmarking while removing runtime seconds from other pytest invocations may change behavior. Confirm that the pytest plugin truly no longer requires or uses --codeflash_seconds in behavioral/line-profile paths to avoid silent regressions.

test_files = list(set(test_files))  # remove multiple calls in the same test function
common_pytest_args = [
    "--capture=tee-sys",
    f"--timeout={pytest_timeout}",
    "-q",
    "--codeflash_loops_scope=session",
    "--codeflash_min_loops=1",
    "--codeflash_max_loops=1",
]
API Consistency

Function signatures for behavioral and line-profile tests dropped the pytest_target_runtime_seconds parameter, but benchmarking retains it with a new default. Verify call sites and docs to ensure consistent API and avoid unexpected argument errors.

def run_benchmarking_tests(
    test_paths: TestFiles,
    pytest_cmd: str,
    test_env: dict[str, str],
    cwd: Path,
    test_framework: str,
    *,
    pytest_target_runtime_seconds: float = TOTAL_LOOPING_TIME_EFFECTIVE,
    verbose: bool = False,
    pytest_timeout: int | None = None,
    pytest_min_loops: int = 5,
    pytest_max_loops: int = 100_000,

@KRRT7 KRRT7 enabled auto-merge September 23, 2025 03:12
@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Align function parameter consistency

Other test runners had their pytest_target_runtime_seconds parameter removed; here
it remains and defaults to a constant, which can cause API inconsistency. Align the
signature or consistently use the parameter across functions to prevent callers from
passing now-ignored args or hitting TypeErrors.

codeflash/verification/test_runner.py [236-240]

 def run_benchmarking_tests(
-    ...
+    test_paths: TestFiles,
+    pytest_cmd: str,
+    test_env: dict[str, str],
+    cwd: Path,
+    test_framework: str,
     *,
-    pytest_target_runtime_seconds: float = TOTAL_LOOPING_TIME_EFFECTIVE,
     verbose: bool = False,
-    ...
+    pytest_timeout: int | None = None,
+    pytest_min_loops: int = 5,
+    pytest_max_loops: int = 100_000,
 ):
Suggestion importance[1-10]: 7

__

Why: run_benchmarking_tests retains pytest_target_runtime_seconds (line 236) while others removed it, creating API inconsistency; aligning signatures improves maintainability and reduces caller confusion.

Medium
Verify constant semantics match prior behavior

Ensure the imported constant is the intended runtime-effective value and not a
function requiring invocation. If the original behavior relied on dynamic
computation at call time, consider preserving that by importing a callable or
documenting the change in semantics to avoid silent behavior drift.

codeflash/verification/test_runner.py [11]

-from codeflash.code_utils.config_consts import TOTAL_LOOPING_TIME_EFFECTIVE
+from codeflash.code_utils.config_consts import TOTAL_LOOPING_TIME_EFFECTIVE  # Constant replaces previous callable; confirm semantics match expected runtime behavior
Suggestion importance[1-10]: 6

__

Why: The import of TOTAL_LOOPING_TIME_EFFECTIVE at line 11 replaces a previous callable; confirming semantics is reasonable but mostly a cautionary check, not a concrete fix.

Low
Possible issue
Restore required plugin runtime flag

The previous --codeflash_seconds argument was removed entirely; if the pytest plugin
still expects it implicitly, runs may behave differently or fail. Confirm the plugin
no longer requires this flag; otherwise, set it from TOTAL_LOOPING_TIME_EFFECTIVE to
avoid runtime errors.

codeflash/verification/test_runner.py [60-68]

 # in run_behavioral_tests common_pytest_args:
 ...
 "--codeflash_max_loops=1",
+f"--codeflash_seconds={TOTAL_LOOPING_TIME_EFFECTIVE}",
 ...
Suggestion importance[1-10]: 5

__

Why: The PR removed --codeflash_seconds from pytest args at lines 61-68; suggesting re-adding it conditionally is plausible but speculative without plugin context.

Low

@KRRT7 KRRT7 merged commit 443cb4d into main Sep 23, 2025
18 of 19 checks passed
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