Skip to content

Conversation

@KRRT7
Copy link
Contributor

@KRRT7 KRRT7 commented Jun 4, 2025

PR Type

Tests, Enhancement


Description

  • Encapsulate compatibility utilities in Compat class

  • Re-export compat paths via class instance

  • Add comprehensive tests for filter_functions behavior

  • Cover ignoring tests dirs, paths, submodules, packages


Changes walkthrough 📝

Relevant files
Enhancement
compat.py
Encapsulate compat utilities in class                                       

codeflash/code_utils/compat.py

  • Introduced Compat class for environment variables
  • Converted globals to @property methods
  • Created _compat instance and re-exported values
  • +35/-9   
    Tests
    test_function_discovery.py
    Add comprehensive filter_functions tests                                 

    tests/test_function_discovery.py

  • Added test_filter_functions covering filter logic
  • Tested ignoring test dirs, paths, submodules, site packages
  • Validated blocklist and checkpoint filtering scenarios
  • Imported and used codeflash_temp_dir
  • +223/-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
    Copy link

    github-actions bot commented Jun 4, 2025

    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

    Directory Write Error

    Tests open and write to directory paths (e.g., codeflash_temp_dir.joinpath("ignored_dir").open("w")) instead of files, causing IsADirectoryError. Ensure actual file paths are used.

    with codeflash_temp_dir.joinpath("ignored_dir").open("w") as f:
        f.write("def ignored_func(): return 1")
    
    ignored_file_path = codeflash_temp_dir.joinpath("ignored_dir")
    discovered_ignored = find_all_functions_in_file(ignored_file_path)
    modified_functions_ignored = {ignored_file_path: discovered_ignored.get(ignored_file_path, [])}
    
    filtered_ignored, count_ignored = filter_functions(
    Indentation in Generated Code

    The triple-quoted string passed to f.write is indented in the test source, producing leading spaces and a blank first line in the generated module. Dedent the string or strip the leading newline.

    def test_filter_functions():
        with codeflash_temp_dir.joinpath("test_get_functions_to_optimize.py").open("w") as f:
            f.write(
    """
    import copy 
    
    def propagate_attributes(
        nodes: dict[str, dict], edges: list[dict], source_node_id: str, attribute: str
    ) -> dict[str, dict]:

    @github-actions
    Copy link

    github-actions bot commented Jun 4, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Write ignored functions in proper .py file

    Create an actual directory for ignored_dir and place a .py file inside so
    find_all_functions_in_file can detect it. Writing directly to a path without
    extension means the test helper may skip the file.

    tests/test_function_discovery.py [419-420]

    -with codeflash_temp_dir.joinpath("ignored_dir").open("w") as f:
    -    f.write("def ignored_func(): return 1")
    +ignored_dir = codeflash_temp_dir.joinpath("ignored_dir")
    +ignored_dir.mkdir(parents=True, exist_ok=True)
    +with ignored_dir.joinpath("ignored_func.py").open("w") as f:
    +    f.write("def ignored_func(): return 1\n")
    Suggestion importance[1-10]: 6

    __

    Why: The test creates a file without a .py extension so find_all_functions_in_file may skip it; making a directory and a proper .py file ensures the ignore-path logic is actually exercised.

    Low
    Create submodule directory with .py file

    Ensure submodule_dir is created as a directory and that the test function is in a
    .py file, otherwise the discovery utility may skip it.

    tests/test_function_discovery.py [437-438]

    -with codeflash_temp_dir.joinpath("submodule_dir").open("w") as f:
    -    f.write("def submodule_func(): return 1")
    +submodule_dir = codeflash_temp_dir.joinpath("submodule_dir")
    +submodule_dir.mkdir(parents=True, exist_ok=True)
    +with submodule_dir.joinpath("submodule_func.py").open("w") as f:
    +    f.write("def submodule_func(): return 1\n")
    Suggestion importance[1-10]: 6

    __

    Why: Similarly, writing to a path named submodule_dir without an extension won’t be recognized as Python code, so creating a folder with a .py file ensures the submodule filtering is properly tested.

    Low

    @KRRT7 KRRT7 force-pushed the add-tests-for-function-discovery branch from 93801ac to c235508 Compare June 4, 2025 23:02
    @KRRT7 KRRT7 requested review from Saga4 and misrasaurabh1 June 4, 2025 23:17
    @openhands-ai
    Copy link

    openhands-ai bot commented Jun 5, 2025

    Looks like there are a few issues preventing this PR from being merged!

    • GitHub Actions are failing:
      • end-to-end-test
      • end-to-end-test
      • end-to-end-test
      • end-to-end-test
      • end-to-end-test

    If you'd like me to help, just leave a comment, like

    @OpenHands please fix the failing actions on PR #287

    Feel free to include any additional details that might help me get this PR into a better state.

    You can manage your notification settings

    # you can use "[^f]\".*\{LF\}\" to find any lines in your code that use this without the f-string
    LF: str = os.linesep
    if TYPE_CHECKING:
    codeflash_temp_dir: Path
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    this looks odd, why not just add types to the global variables?

    @KRRT7 KRRT7 merged commit 7b770b1 into main Jun 6, 2025
    16 of 26 checks passed
    @KRRT7 KRRT7 deleted the add-tests-for-function-discovery branch June 6, 2025 04:10
    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.

    3 participants