Skip to content

Conversation

@KRRT7
Copy link
Contributor

@KRRT7 KRRT7 commented Mar 24, 2025

User description

first pass

fix codeflash_capture

fix formatter

more tempfile changes

Update test_get_code.py


PR Type

  • Enhancement
  • Tests

Description

  • Update temporary file handling with TemporaryDirectory.

  • Enhance get_run_tmp_file with improved type hints and resolution.

  • Refactor tests to use as_posix() conversions for cross-platform compatibility.

  • Replace NamedTemporaryFile with robust TemporaryDirectory usage.


Changes walkthrough 📝

Relevant files
Enhancement
4 files
code_utils.py
Add global _tmpdir and update get_run_tmp_file signature 
+6/-3     
coverage_utils.py
Refine candidate path building with as_posix conversion   
+3/-3     
instrument_existing_tests.py
Use get_run_tmp_file with as_posix for temporary paths     
+1/-1     
instrument_codeflash_capture.py
Update decorator calls with as_posix and tests_root conversion
+4/-4     
Tests
12 files
test_code_context_extractor.py
Replace NamedTemporaryFile with TemporaryDirectory for file creation
+25/-32 
test_code_utils.py
Adjust site package path resolution and import order         
+2/-2     
test_codeflash_capture.py
Update get_run_tmp_file usage and environment setup in tests
+19/-19 
test_formatter.py
Migrate tempfile usage to TemporaryDirectory and update file writes
+30/-29 
test_function_discovery.py
Replace temporary file handling with TemporaryDirectory in discovery
tests
+18/-18 
test_get_code.py
Update temporary file creation using TemporaryDirectory and file APIs
+55/-37 
test_get_helper_code.py
Refactor temporary file usage to use TemporaryDirectory and resolve
paths
+6/-4     
test_instrument_all_and_run.py
Update inject_profiling tests with as_posix conversion for tmpdir
paths
+7/-7     
test_instrument_codeflash_capture.py
Ensure consistent get_run_tmp_file usage with as_posix in decorators
+9/-9     
test_instrument_tests.py
Major refactoring: use TemporaryDirectory and update get_run_tmp_file
calls
+237/-230
test_shell_utils.py
Add posix-only skip condition and update file access in tests
+1/-0     
test_test_runner.py
Replace NamedTemporaryFile with TemporaryDirectory and adjust file
writes
+10/-8   

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

    github-actions bot commented Mar 24, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit accc636)

    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

    Temporary File Handling

    The new usage of TemporaryDirectory instead of NamedTemporaryFile is more robust but please verify that all temporary files are reliably cleaned up and work as expected across all targeted platforms.

    with tempfile.TemporaryDirectory() as tempdir:
        temp_file_path = Path(tempdir) / "temp_test.py"
        with temp_file_path.open("w") as f:
            f.write(code)
        func = FunctionToOptimize(function_name="sorter", parents=[], file_path=temp_file_path)
        original_cwd = Path.cwd()
        run_cwd = Path(__file__).parent.parent.resolve()
        os.chdir(run_cwd)
        success, new_test = inject_profiling_into_existing_test(
            temp_file_path,
            [CodePosition(9, 17), CodePosition(13, 17), CodePosition(17, 17)],
            func,
            temp_file_path.parent,
            "unittest",
        )
        os.chdir(original_cwd)
    assert success
    assert new_test == expected.format(
        module_path="temp_test", tmp_dir_path=get_run_tmp_file("test_return_values").as_posix()
    )
    Path Conversion

    The updated get_run_tmp_file function now converts paths using as_posix(). Confirm that this conversion remains safe and does not cause issues on non‐POSIX systems.

    def get_run_tmp_file(file_path: Path | str) -> Path:
        if not hasattr(get_run_tmp_file, "tmpdir"):
            get_run_tmp_file.tmpdir = _tmpdir
        if isinstance(file_path, str):
            file_path = Path(file_path)
        return (Path(get_run_tmp_file.tmpdir.name) / file_path).resolve()
    Debug Output

    The changes to the debug print statements now use as_posix() for path resolution. Ensure that the output remains clear and useful for troubleshooting across platforms.

    print(f"TEST_INFO_START|{{get_test_info_from_stack('{test_dir.resolve().as_posix()}')}}|TEST_INFO_END")

    @github-actions
    Copy link

    github-actions bot commented Mar 24, 2025

    PR Code Suggestions ✨

    Latest suggestions up to accc636
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add temp dir cleanup

    Ensure the global temporary directory is cleaned up at exit to prevent resource
    leaks.

    codeflash/code_utils/code_utils.py [12]

    +import atexit
     _tmpdir = TemporaryDirectory(prefix="codeflash_")
    +atexit.register(_tmpdir.cleanup)
    Suggestion importance[1-10]: 8

    __

    Why: Registering the temporary directory’s cleanup helps prevent resource leaks, and the improved code correctly integrates atexit without overcomplicating the original design.

    Medium
    General
    Eliminate redundant commented code

    Remove or refactor the large block of commented-out test code to improve clarity and
    maintainability.

    tests/test_instrument_tests.py [2280-2287]

    -# assert new_test_behavior.replace('"', "'") == expected_behavior.format(
    -#     module_path="code_to_optimize.tests.unittest.test_perfinjector_bubble_sort_unittest_parametrized_loop_results_temp",
    -#     tmp_dir_path=get_run_tmp_file(Path("test_return_values")),
    -# ).replace('"', "'")
    -# assert new_test_perf.replace('"', "'") == expected_perf.format(
    -#     module_path="code_to_optimize.tests.unittest.test_perfinjector_bubble_sort_unittest_parametrized_loop_results_temp",
    -#     tmp_dir_path=get_run_tmp_file(Path("test_return_values")),
    -# ).replace('"', "'")
    -# ... (and similar commented-out assertions)
    +(Remove the commented-out assertions entirely if no longer needed)
    Suggestion importance[1-10]: 4

    __

    Why: Removing the block of commented-out assertions improves clarity and maintainability, although it is a minor stylistic improvement.

    Low

    Previous suggestions

    Suggestions up to commit cf55e96
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Use text mode for temp files

    Change the file open mode from binary ("wb") to text ("w") when writing string data.

    tests/test_formatter.py [133-137]

     with tempfile.TemporaryDirectory() as tmp_dir:
         tmp_path = Path(tmp_dir) / "test.py"
    -    with tmp_path.open("wb") as tmp:
    +    with tmp_path.open("w") as tmp:
             tmp.write(original_code)
             tmp.flush()
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies that writing string data in binary mode may lead to encoding issues, and switching to text mode ensures proper handling of the string data.

    Medium

    codeflash-ai bot added a commit that referenced this pull request Mar 31, 2025
    …s-fixes`)
    
    Here is an optimized version of the program.
    
    
    
    ### Changes made.
    1. Cached `current_path.root` in variable `root` to avoid calling `current_path.parent` multiple times.
    2. Reduced the computation of `candidate_path` by using f-strings for faster string concatenation.
    
    These changes help in reducing the overhead and making the loop slightly faster.
    @codeflash-ai
    Copy link
    Contributor

    codeflash-ai bot commented Mar 31, 2025

    ⚡️ Codeflash found optimizations for this PR

    📄 137% (1.37x) speedup for generate_candidates in codeflash/code_utils/coverage_utils.py

    ⏱️ Runtime : 821 microseconds 346 microseconds (best of 416 runs)

    I created a new dependent PR with the suggested changes. Please review:

    If you approve, it will be merged into this PR (branch windows-fixes).

    @KRRT7 KRRT7 closed this Apr 8, 2025
    @KRRT7 KRRT7 deleted the windows-fixes branch April 8, 2025 00:50
    @KRRT7 KRRT7 restored the windows-fixes branch April 8, 2025 00:51
    @KRRT7 KRRT7 reopened this Apr 8, 2025
    @github-actions
    Copy link

    github-actions bot commented Apr 8, 2025

    Persistent review updated to latest commit accc636

    @KRRT7 KRRT7 closed this Jun 21, 2025
    @KRRT7 KRRT7 deleted the windows-fixes branch June 21, 2025 04:42
    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