Skip to content

Conversation

@KRRT7
Copy link
Contributor

@KRRT7 KRRT7 commented Jun 6, 2025

PR Type

Enhancement, Tests


Description

Add caching to skip unchanged tests
Extract single-file test processing helper
Improve pytest and unittest parameter parsing
Refactor process_test_files implementation


Changes walkthrough 📝

Relevant files
Enhancement
discover_unit_tests.py
Refactor test file processing with caching                             

codeflash/discovery/discover_unit_tests.py

  • Extracted _process_single_test_file helper function
  • Added caching per test file hash
  • Structured process_test_files to invoke helper
  • Enhanced pytest/unittest parameter detection
  • +169/-164

    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 6, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit a81bb64)

    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

    Inconsistent return types

    Cached tests are added as raw TestsInFile instances, but new tests are wrapped as FunctionCalledInTest, causing type mismatches in function_to_test_map.

    cached_tests = tests_cache.get_tests_for_file(str(test_file), file_hash)
    if cached_tests:
        self_cur = tests_cache.cur
        self_cur.execute(
            "SELECT qualified_name_with_modules_from_root FROM discovered_tests WHERE file_path = ? AND file_hash = ?",
            (str(test_file), file_hash),
        )
        qualified_names = [row[0] for row in self_cur.fetchall()]
        for cached, qualified_name in zip(cached_tests, qualified_names):
            function_to_test_map[qualified_name].add(cached)
        return
    Indentation error

    The progress.advance(task_id) call appears misaligned after invoking _process_single_test_file, which may lead to a syntax or runtime indentation issue in process_test_files.

    progress.advance(task_id)
    Unreachable code

    Some test_functions.add calls in the unittest branch lack the '+' marker in the diff and may not be executed, indicating copy-paste or indentation errors that break test discovery.

    if is_parameterized and new_function == def_name.name:
        test_functions.add(
            TestFunction(
                function_name=def_name.name,
                test_class=matched_name,
                parameters=parameters,
                test_type=functions[0].test_type,
            )
        )
    elif function == def_name.name:
        test_functions.add(
            TestFunction(

    @github-actions
    Copy link

    github-actions bot commented Jun 6, 2025

    PR Code Suggestions ✨

    Latest suggestions up to a81bb64
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Preserve correct test_type

    Always using functions[0].test_type ignores the actual test type of each matching
    function. Build a map from function.test_function to its TestsInFile object and use
    that to retrieve the correct test_type.

    codeflash/discovery/discover_unit_tests.py [368-373]

    +function_map = {elem.test_function: elem for elem in functions}
    +...
     TestFunction(
         function_name=def_name.name,
         test_class=matched_name,
         parameters=parameters,
    -    test_type=functions[0].test_type,
    +    test_type=function_map[function].test_type,
     )
    Suggestion importance[1-10]: 8

    __

    Why: Always using functions[0].test_type ignores the actual test type for each matching function, which can lead to misclassification; building a map to retrieve the correct test_type per function fixes this.

    Medium
    Reduce redundant regex calls

    You’re calling the same regex split twice, which is inefficient and error-prone.
    Perform the split once into a tuple or list, then unpack the function_name and
    parameters.

    codeflash/discovery/discover_unit_tests.py [332-334]

     if "[" in function.test_function:
    -    function_name = PYTEST_PARAMETERIZED_TEST_NAME_REGEX.split(function.test_function)[0]
    -    parameters = PYTEST_PARAMETERIZED_TEST_NAME_REGEX.split(function.test_function)[1]
    +    parts = PYTEST_PARAMETERIZED_TEST_NAME_REGEX.split(function.test_function)
    +    function_name, parameters = parts[0], parts[1]
    Suggestion importance[1-10]: 4

    __

    Why: Calling split twice with the same regex is redundant; unpacking the result once improves readability and offers a small performance benefit.

    Low

    Previous suggestions

    Suggestions up to commit 26b738f
    CategorySuggestion                                                                                                                                    Impact
    General
    Handle worker exceptions gracefully

    Wrap the call to future.result() in a try/except block so that one worker failure
    doesn’t crash the entire discovery loop. Log the exception and continue to the next
    future.

    codeflash/discovery/discover_unit_tests.py [452-453]

     for future in as_completed(future_to_file):
    -    _, local_results = future.result()
    +    try:
    +        _, local_results = future.result()
    +    except Exception as e:
    +        logger.error(f"Error processing {future_to_file[future]}: {e}")
    +        continue
    +    for qualified_name, function_called_in_test in local_results:
    +        function_to_test_map[qualified_name].add(function_called_in_test)
    Suggestion importance[1-10]: 8

    __

    Why: Wrapping future.result() in a try/except prevents a single worker failure from aborting the entire discovery process, improving robustness without altering functionality.

    Medium
    Consolidate module import placement

    Move the import of module_name_from_file_path to the top of the module alongside
    other imports to avoid repeated local imports inside loops and improve readability.

    codeflash/discovery/discover_unit_tests.py [412]

    -from codeflash.code_utils.code_utils import module_name_from_file_path
    +# at top of file among other code_utils imports
    +from codeflash.code_utils.code_utils import ImportErrorPattern, custom_addopts, get_run_tmp_file, module_name_from_file_path
    Suggestion importance[1-10]: 5

    __

    Why: Moving the module_name_from_file_path import to the top reduces repeated local imports and enhances readability, but has minimal impact on core functionality.

    Low
    Possible issue
    Use correct test_type per function

    Do not always use functions[0].test_type for every match, as this can assign the
    wrong test type. Instead, lookup the TestsInFile instance matching the current
    function to get its actual test_type.

    codeflash/discovery/discover_unit_tests.py [347-351]

     TestFunction(
         function_name=def_name.name,
         test_class=matched_name,
         parameters=parameters,
    -    test_type=functions[0].test_type,
    +    test_type=next(elem.test_type for elem in functions if elem.test_function == function),
     )
    Suggestion importance[1-10]: 8

    __

    Why: Always using functions[0].test_type can misassign test_type for non-first matches; looking up the matching TestsInFile ensures each TestFunction gets the correct test_type.

    Medium

    @KRRT7 KRRT7 closed this Jun 7, 2025
    @KRRT7 KRRT7 force-pushed the mp-test-processing branch from e3a84bc to 4de9323 Compare June 7, 2025 00:36
    @KRRT7 KRRT7 reopened this Jun 7, 2025
    @github-actions
    Copy link

    github-actions bot commented Jun 7, 2025

    Persistent review updated to latest commit a81bb64

    @KRRT7 KRRT7 closed this Jun 8, 2025
    @aseembits93 aseembits93 deleted the mp-test-processing branch August 17, 2025 19:26
    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