Skip to content

Conversation

@misrasaurabh1
Copy link
Contributor

@misrasaurabh1 misrasaurabh1 commented Jun 28, 2025

PR Type

Bug fix, Enhancement


Description

  • Add static method to find instrumented test files

  • Invoke cleanup before optimization run

  • Refactor cleanup_temporary_paths logic

  • Improve regex for test file matching


Changes diagram

flowchart LR
  run["Optimizer.run()"] -- "invoke cleanup" --> find["find_leftover_instrumented_test_files"]
  find -- "matching files" --> clean1["cleanup_paths(files)"]
  run -- "finally" --> temp_cleanup["cleanup_temporary_paths()"]
  temp_cleanup -- "cleanup dirs" --> clean2["cleanup_paths(dirs)"]
Loading

Changes walkthrough 📝

Relevant files
Enhancement
optimizer.py
Refactor and extend cleanup logic                                               

codeflash/optimization/optimizer.py

  • Imported cleanup_paths from code_utils
  • Added find_leftover_instrumented_test_files static method
  • Called cleanup early in run()
  • Refactored cleanup_temporary_paths implementation
  • +22/-3   

    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: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Pattern Accuracy

    The regex in find_leftover_instrumented_test_files may not match all intended filename variants or could match unintended ones. Validate the patterns against expected test file names with unit tests.

        r"(?:test.*__perf_test_\d?\.py|test_.*__unit_test_\d?\.py|test_.*__perfinstrumented\.py|test_.*__perfonlyinstrumented\.py)$"
    )
    Scanning Performance

    Using test_root.rglob("*") traverses all files, including non-Python ones. Consider restricting the glob to "*.py" to improve performance when searching for test files.

    return [
        file_path for file_path in test_root.rglob("*") if file_path.is_file() and pattern.match(file_path.name)
    ]
    Type Compatibility

    cleanup_paths is called with a list of Path objects. Ensure it handles Path instances or convert paths to strings to avoid potential type errors.

    cleanup_paths(Optimizer.find_leftover_instrumented_test_files(self.test_cfg.tests_root))

    Copy link
    Contributor

    @KRRT7 KRRT7 left a comment

    Choose a reason for hiding this comment

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

    LGTM, though codeflash will probably optimize the regex away to module level

    @github-actions
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Guard against nonexistent test root

    Ensure that test_root exists and is a directory before iterating to prevent runtime
    errors. Add a guard to return an empty list if test_root is missing or not a
    directory.

    codeflash/optimization/optimizer.py [346-348]

    +if not test_root.is_dir():
    +    return []
     return [
    -    file_path for file_path in test_root.rglob("*") if file_path.is_file() and pattern.match(file_path.name)
    +    file_path for file_path in test_root.rglob("*")
    +    if file_path.is_file() and pattern.match(file_path.name)
     ]
    Suggestion importance[1-10]: 6

    __

    Why: Adding a directory existence check for test_root prevents potential runtime errors when the path is missing, improving the robustness of find_leftover_instrumented_test_files.

    Low
    General
    Cleanup leftover instrumented tests

    Include the leftover instrumented test files in the final cleanup to ensure all
    generated artifacts are removed. Append the list returned by
    find_leftover_instrumented_test_files.

    codeflash/optimization/optimizer.py [354]

    -cleanup_paths([self.test_cfg.concolic_test_root_dir, self.replay_tests_dir])
    +cleanup_paths(
    +    [self.test_cfg.concolic_test_root_dir, self.replay_tests_dir]
    +    + self.find_leftover_instrumented_test_files(self.test_cfg.tests_root)
    +)
    Suggestion importance[1-10]: 6

    __

    Why: Including leftover instrumented test files in cleanup_temporary_paths ensures all generated artifacts are removed, preventing stale test files post-optimization.

    Low
    Cache regex pattern globally

    Avoid recompiling the same regex on every call by moving the compilation to module
    level. Define a constant TEST_FILE_PATTERN at the top and reference it in the
    method.

    codeflash/optimization/optimizer.py [342-344]

    -pattern = re.compile(
    +# at module scope
    +TEST_FILE_PATTERN = re.compile(
         r"(?:test.*__perf_test_\\d?\\.py|test_.*__unit_test_\\d?\\.py|test_.*__perfinstrumented\\.py|test_.*__perfonlyinstrumented\\.py)$"
     )
     
    +@staticmethod
    +def find_leftover_instrumented_test_files(test_root: Path) -> list[Path]:
    +    if not test_root.is_dir():
    +        return []
    +    return [
    +        file_path for file_path in test_root.rglob("*")
    +        if file_path.is_file() and TEST_FILE_PATTERN.match(file_path.name)
    +    ]
    +
    Suggestion importance[1-10]: 5

    __

    Why: Moving the regex compilation to module scope reduces redundant work on every call, improving performance with a straightforward refactor.

    Low
    Use instance method call

    Use the instance method rather than hardcoding the class name to support potential
    subclassing. Replace Optimizer with self.

    codeflash/optimization/optimizer.py [255]

    -cleanup_paths(Optimizer.find_leftover_instrumented_test_files(self.test_cfg.tests_root))
    +cleanup_paths(self.find_leftover_instrumented_test_files(self.test_cfg.tests_root))
    Suggestion importance[1-10]: 4

    __

    Why: Calling self.find_leftover_instrumented_test_files allows subclasses to override the behavior, enhancing extensibility with minimal impact.

    Low

    @misrasaurabh1 misrasaurabh1 merged commit 132b92d into main Jun 28, 2025
    17 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