Skip to content

Conversation

@aseembits93
Copy link
Contributor

@aseembits93 aseembits93 commented Jun 3, 2025

User description

Created a context manager for detecting and renaming the conftest.py file


PR Type

Enhancement, Tests


Description

  • Introduce AST-based modifiers for autouse fixtures

  • Apply codeflash_no_autouse marker to all tests

  • Extend CLI to set override-fixtures and pytest markers

  • Revert conftest changes after optimization


Changes walkthrough 📝

Relevant files
Configuration changes
cmd_init.py
Initialize override-fixtures and pytest markers                   

codeflash/cli_cmds/cmd_init.py

  • Imported table from tomlkit
  • Added default override-fixtures flag in config
  • Initialized [tool.pytest.ini_options].markers
  • +13/-2   
    Enhancement
    code_replacer.py
    Add AST transformers for pytest fixtures                                 

    codeflash/code_utils/code_replacer.py

  • Added PytestMarkAdder and AutouseFixtureModifier transformers
  • Implemented disable_autouse, modify_autouse_fixture,
    add_custom_marker_to_all_tests
  • Used libcst and isort for AST modifications
  • Imported find_conftest_files and ImportAdder
  • +142/-0 
    code_utils.py
    Add restore_conftest helper                                                           

    codeflash/code_utils/code_utils.py

    • Introduced restore_conftest to revert conftest edits
    +5/-0     
    config_parser.py
    Implement find_conftest_files function                                     

    codeflash/code_utils/config_parser.py

    • Added find_conftest_files to locate conftest.py per test path
    +15/-0   
    function_optimizer.py
    Integrate fixture modification into optimizer                       

    codeflash/optimization/function_optimizer.py

  • Integrated modify_autouse_fixture into optimize flow
  • Applied add_custom_marker_to_all_tests on generated tests
  • Captured and restored original conftest content on failure
  • +16/-1   

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @aseembits93
    Copy link
    Contributor Author

    Closing it as renaming the conftest file could break testing for some repos as other fixtures would be defined there

    @aseembits93 aseembits93 closed this Jun 3, 2025
    @aseembits93 aseembits93 reopened this Jun 3, 2025
    @aseembits93 aseembits93 changed the title Ignore conftest by renaming it temporarily (CF-658) Ignore autouse fixtures in conftest (CF-658) Jun 3, 2025
    @github-actions
    Copy link

    github-actions bot commented Jun 3, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 677f0f8)

    Here are some key observations to aid the review process:

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

    Missing Restore

    The original conftest.py content is restored only on failure paths. On successful optimization, restore_conftest is never called, leaving the repository in a modified state.

    logger.debug("disabling all autouse fixtures associated with the test files")
    original_conftest_content = modify_autouse_fixture(
        generated_test_paths + generated_perf_test_paths + list(instrumented_unittests_created_for_function)
    )
    logger.debug("add custom marker to all tests")
    add_custom_marker_to_all_tests(
        generated_test_paths + generated_perf_test_paths + list(instrumented_unittests_created_for_function)
    )
    
    # Get a dict of file_path_to_classes of fto and helpers_of_fto
    file_path_to_helper_classes = defaultdict(set)
    for function_source in code_context.helper_functions:
        if (
            function_source.qualified_name != self.function_to_optimize.qualified_name
            and "." in function_source.qualified_name
        ):
            file_path_to_helper_classes[function_source.file_path].add(function_source.qualified_name.split(".")[0])
    
    baseline_result = self.establish_original_code_baseline(  # this needs better typing
        code_context=code_context,
        original_helper_code=original_helper_code,
        file_path_to_helper_classes=file_path_to_helper_classes,
    )
    
    console.rule()
    paths_to_cleanup = (
        generated_test_paths + generated_perf_test_paths + list(instrumented_unittests_created_for_function)
    )
    
    if not is_successful(baseline_result):
        restore_conftest(original_conftest_content)
        cleanup_paths(paths_to_cleanup)
        return Failure(baseline_result.failure())
    
    original_code_baseline, test_functions_to_remove = baseline_result.unwrap()
    if isinstance(original_code_baseline, OriginalCodeBaseline) and not coverage_critic(
        original_code_baseline.coverage_results, self.args.test_framework
    ):
        restore_conftest(original_conftest_content)
        cleanup_paths(paths_to_cleanup)
    Conftest Discovery

    find_conftest_files stops searching when cur_path == Path.cwd() and never checks the root-level conftest.py, so fixtures in the project root may be ignored.

    def find_conftest_files(test_paths: list[Path]) -> list[Path]:
        list_of_conftest_files = set()
        for test_path in test_paths:
            # Find the conftest file on the root of the project
            dir_path = Path.cwd()
            cur_path = test_path
            while cur_path != dir_path:
                config_file = cur_path / "conftest.py"
                if config_file.exists():
                    list_of_conftest_files.add(config_file)
                # Search for conftest.py in the parent directories
                cur_path = cur_path.parent
        return list(list_of_conftest_files)
    Duplicate Section

    tool_section is fetched and set twice when building pyproject.toml, which may inadvertently overwrite previously added entries. Consider consolidating into a single update.

    pyproject_data["tool"] = tool_section
    # Create [tool.pytest.ini_options] if it doesn't exist
    tool_section = pyproject_data.get("tool", table())
    pytest_section = tool_section.get("pytest", table())
    ini_options = pytest_section.get("ini_options", table())
    # Define or overwrite the 'markers' array
    ini_options["markers"] = ["codeflash_no_autouse"]
    # Set updated sections back
    pytest_section["ini_options"] = ini_options
    tool_section["pytest"] = pytest_section
    pyproject_data["tool"] = tool_section

    @github-actions
    Copy link

    github-actions bot commented Jun 3, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 677f0f8
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Preserve existing pytest markers

    Preserve any existing pytest markers instead of overwriting them. Retrieve the
    current list, append the new marker if missing, and then assign back to
    ini_options["markers"].

    codeflash/cli_cmds/cmd_init.py [737]

    -ini_options["markers"] = ["codeflash_no_autouse"]
    +existing_markers = ini_options.get("markers", [])
    +if "codeflash_no_autouse" not in existing_markers:
    +    existing_markers.append("codeflash_no_autouse")
    +ini_options["markers"] = existing_markers
    Suggestion importance[1-10]: 8

    __

    Why: Preserving and appending to any existing markers prevents inadvertent configuration loss and respects user-defined pytest markers.

    Medium
    Sort conftest file list

    Return a sorted list of conftest paths instead of an arbitrary order from a set to
    ensure deterministic processing across runs.

    codeflash/code_utils/config_parser.py [34-46]

     def find_conftest_files(test_paths: list[Path]) -> list[Path]:
         list_of_conftest_files = set()
         for test_path in test_paths:
             dir_path = Path.cwd()
             cur_path = test_path
             while cur_path != dir_path:
                 config_file = cur_path / "conftest.py"
                 if config_file.exists():
                     list_of_conftest_files.add(config_file)
                 cur_path = cur_path.parent
    -    return list(list_of_conftest_files)
    +    return sorted(list_of_conftest_files)
    Suggestion importance[1-10]: 6

    __

    Why: Returning a sorted list ensures deterministic order across runs and prevents nondeterministic behavior from iterating over a set.

    Low
    Only rewrite file if changed

    Only write back to the conftest file when the AST transformation actually changes
    its content to avoid unnecessary rewrites and preserve the original formatting.
    Compare the regenerated code with the original text before calling write_text.

    codeflash/code_utils/code_replacer.py [141-147]

     def disable_autouse(test_path: Path) -> str:
         file_content = test_path.read_text(encoding="utf-8")
         module = cst.parse_module(file_content)
    -    disable_autouse_fixture = AutouseFixtureModifier()
    -    modified_module = module.visit(disable_autouse_fixture)
    -    test_path.write_text(modified_module.code, encoding="utf-8")
    +    modified_module = module.visit(AutouseFixtureModifier())
    +    new_code = modified_module.code
    +    if new_code != file_content:
    +        test_path.write_text(new_code, encoding="utf-8")
         return file_content
    Suggestion importance[1-10]: 5

    __

    Why: This avoids unnecessary file writes by only updating when the AST transformation produces a change, preserving formatting and timestamps.

    Low
    Possible issue
    Make yield statement valid

    Use a syntactically complete yield statement to avoid parse errors in the generated
    AST. For example, yield a value explicitly or use cst.Yield directly.

    codeflash/code_utils/code_replacer.py [130]

    -yield_statement = cst.parse_statement("yield")
    +yield_statement = cst.parse_statement("yield None")
    Suggestion importance[1-10]: 3

    __

    Why: While using yield None can avoid parse issues, a bare yield is syntactically valid in a generator, making this a minor stylistic change.

    Low

    Previous suggestions

    Suggestions up to commit 1271f26
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Remove hardcoded override

    Remove the hardcoded test_files assignment so that pytest uses the dynamically
    collected test files. This will avoid overriding discovery with a fixed path.

    codeflash/verification/test_runner.py [56]

    -test_files = ['/Users/codeflash/Downloads/codeflash-dev/codeflash/code_to_optimize/tests/pytest/test_bubble_sort__perfinstrumented.py']
    +# removed hardcoded override; dynamic discovery remains intact
    Suggestion importance[1-10]: 9

    __

    Why: The hardcoded test_files assignment overrides dynamic discovery and forces only one test to run; removing it restores correct test collection.

    High
    General
    Search up to filesystem root

    Change the loop to search until the filesystem root rather than stopping at the
    current working directory, and ensure you check each directory including the initial
    one. This avoids missing a conftest.py when tests_path == cwd.

    codeflash/code_utils/config_parser.py [38-43]

    -while cur_path != dir_path:
    +while True:
         config_file = cur_path / "conftest.py"
         if config_file.exists():
             return config_file
    +    if cur_path.parent == cur_path:
    +        break
         cur_path = cur_path.parent
    +return None
    Suggestion importance[1-10]: 7

    __

    Why: Allowing the loop to run to the filesystem root ensures conftest.py is found even when tests_path equals cwd, resolving a missed lookup.

    Medium
    Use robust temp path rename

    Use Path.with_suffix instead of string concatenation for the temp file and guard the
    rename to avoid silent failures. This ensures correct path handling and safer
    restores.

    codeflash/code_utils/code_utils.py [97-98]

     if conftest_file:
    -    tmp_conftest_file = Path(str(conftest_file) + ".tmp")
    -    conftest_file.rename(tmp_conftest_file)
    +    tmp_conftest_file = conftest_file.with_suffix(conftest_file.suffix + ".tmp")
    +    try:
    +        conftest_file.rename(tmp_conftest_file)
    +    except OSError:
    +        logger.warning(f"Failed to rename {conftest_file} to {tmp_conftest_file}")
    Suggestion importance[1-10]: 5

    __

    Why: Switching to with_suffix ensures correct temp file naming and adding a rename guard logs failures instead of failing silently.

    Low
    Rename variable to avoid shadowing

    Avoid shadowing the built-in input function by renaming the variable to something
    like data or arr. This prevents unexpected behavior in tests.

    code_to_optimize/tests/pytest/test_bubble_sort.py [6]

    -input = [5, 4, 3, 2, 1, 0]
    +data = [5, 4, 3, 2, 1, 0]
    Suggestion importance[1-10]: 4

    __

    Why: Renaming the input variable avoids shadowing the built-in input() function and improves readability with minimal impact.

    Low

    @openhands-ai
    Copy link

    openhands-ai bot commented Jun 6, 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
      • 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 #268

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

    You can manage your notification settings

    @aseembits93 aseembits93 marked this pull request as ready for review June 6, 2025 03:10
    @aseembits93 aseembits93 requested a review from misrasaurabh1 June 6, 2025 03:10
    @github-actions
    Copy link

    github-actions bot commented Jun 6, 2025

    Persistent review updated to latest commit 677f0f8


    # Both fixtures should be modified
    code = modified_module.code
    assert code.count('if request.node.get_closest_marker("codeflash_no_autouse"):') == 2
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    just compare with the actual full string


    code = modified_module.code
    assert 'if request.node.get_closest_marker("codeflash_no_autouse"):' in code
    assert "try:" in code
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    full string

    misrasaurabh1
    misrasaurabh1 previously approved these changes Jun 6, 2025
    @aseembits93 aseembits93 enabled auto-merge June 6, 2025 20:20
    @misrasaurabh1 misrasaurabh1 disabled auto-merge June 6, 2025 20:59
    @misrasaurabh1 misrasaurabh1 merged commit 2d7db72 into main Jun 6, 2025
    15 of 16 checks passed
    @aseembits93 aseembits93 deleted the ignore-conftest branch June 6, 2025 23:55
    @aseembits93 aseembits93 restored the ignore-conftest branch June 13, 2025 23:00
    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