Skip to content

Conversation

@misrasaurabh1
Copy link
Contributor

@misrasaurabh1 misrasaurabh1 commented Jul 3, 2025

PR Type

Bug fix


Description

  • Add broad exception handling in discovery logic

  • Continue processing on mapping errors

  • Log caught exceptions at debug level

  • Preserve test discovery count increment


Changes walkthrough 📝

Relevant files
Error handling
discover_unit_tests.py
Wrap discovery logic in try-except                                             

codeflash/discovery/discover_unit_tests.py

  • Wrapped function-to-test mapping in try-except
  • Logged exceptions with logger.debug
  • Continued loop on any caught exception
  • Kept test discovery counter increment
  • +40/-38 

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @misrasaurabh1 misrasaurabh1 requested a review from a team July 3, 2025 21:33
    @github-actions
    Copy link

    github-actions bot commented Jul 3, 2025

    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

    Broad exception catch

    Catching all Exception hides specific failures and can make debugging harder. Consider narrowing the exception types or handling expected error cases explicitly.

    try:
        if not definition or definition[0].type != "function":
            continue
        definition_obj = definition[0]
        definition_path = str(definition_obj.module_path)
    
        project_root_str = str(project_root_path)
        if (
            definition_path.startswith(project_root_str + os.sep)
            and definition_obj.module_name != name.module_name
            and definition_obj.full_name is not None
        ):
            # Pre-compute common values outside the inner loop
            module_prefix = definition_obj.module_name + "."
            full_name_without_module_prefix = definition_obj.full_name.replace(module_prefix, "", 1)
            qualified_name_with_modules_from_root = f"{module_name_from_file_path(definition_obj.module_path, project_root_path)}.{full_name_without_module_prefix}"
    
            for test_func in test_functions_by_name[scope]:
                if test_func.parameters is not None:
                    if test_framework == "pytest":
                        scope_test_function = f"{test_func.function_name}[{test_func.parameters}]"
                    else:  # unittest
                        scope_test_function = f"{test_func.function_name}_{test_func.parameters}"
                else:
                    scope_test_function = test_func.function_name
    
                function_to_test_map[qualified_name_with_modules_from_root].add(
                    FunctionCalledInTest(
                        tests_in_file=TestsInFile(
                            test_file=test_file,
                            test_class=test_func.test_class,
                            test_function=scope_test_function,
                            test_type=test_func.test_type,
                        ),
                        position=CodePosition(line_no=name.line, col_no=name.column),
                    )
                )
                num_discovered_tests += 1
    except Exception as e:
        logger.debug(str(e))
        continue
    Large try block

    The try block spans the entire inner logic, so a single error aborts the rest of the loop. Break it into smaller scopes to avoid skipping unrelated iterations.

    try:
        if not definition or definition[0].type != "function":
            continue
        definition_obj = definition[0]
        definition_path = str(definition_obj.module_path)
    
        project_root_str = str(project_root_path)
        if (
            definition_path.startswith(project_root_str + os.sep)
            and definition_obj.module_name != name.module_name
            and definition_obj.full_name is not None
        ):
            # Pre-compute common values outside the inner loop
            module_prefix = definition_obj.module_name + "."
            full_name_without_module_prefix = definition_obj.full_name.replace(module_prefix, "", 1)
            qualified_name_with_modules_from_root = f"{module_name_from_file_path(definition_obj.module_path, project_root_path)}.{full_name_without_module_prefix}"
    
            for test_func in test_functions_by_name[scope]:
                if test_func.parameters is not None:
                    if test_framework == "pytest":
                        scope_test_function = f"{test_func.function_name}[{test_func.parameters}]"
                    else:  # unittest
                        scope_test_function = f"{test_func.function_name}_{test_func.parameters}"
                else:
                    scope_test_function = test_func.function_name
    
                function_to_test_map[qualified_name_with_modules_from_root].add(
                    FunctionCalledInTest(
                        tests_in_file=TestsInFile(
                            test_file=test_file,
                            test_class=test_func.test_class,
                            test_function=scope_test_function,
                            test_type=test_func.test_type,
                        ),
                        position=CodePosition(line_no=name.line, col_no=name.column),
                    )
                )
                num_discovered_tests += 1
    except Exception as e:
        logger.debug(str(e))
        continue
    Incomplete logging

    Logging only str(e) loses stack trace context. Use logger.exception(...) or include traceback to aid debugging.

    except Exception as e:
        logger.debug(str(e))

    @github-actions
    Copy link

    github-actions bot commented Jul 3, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Eliminate overly broad exception catch

    Remove the broad try/except around the entire processing block so unexpected errors
    aren't silently swallowed. Let unanticipated issues surface instead of hiding them.

    codeflash/discovery/discover_unit_tests.py [627-667]

    -try:
    -    if not definition or definition[0].type != "function":
    -        continue
    -    # large block of processing...
    -    num_discovered_tests += 1
    -except Exception as e:
    -    logger.debug(str(e))
    +if not definition or definition[0].type != "function":
         continue
    +# large block of processing...
    +num_discovered_tests += 1
    Suggestion importance[1-10]: 8

    __

    Why: Removing the broad try/except prevents swallowing unexpected errors, allowing real issues to surface and be fixed appropriately.

    Medium
    Log full exception traceback

    Replace the broad logger.debug(str(e)) with logger.exception(...) to capture full
    traceback and include context about the failed definition name. This ensures you get
    stack traces in logs for easier debugging.

    codeflash/discovery/discover_unit_tests.py [627-667]

     try:
         if not definition or definition[0].type != "function":
             continue
         definition_obj = definition[0]
         ...
         num_discovered_tests += 1
    -except Exception as e:
    -    logger.debug(str(e))
    +except Exception:
    +    logger.exception("Failed processing definition %r in %r", name, test_file)
         continue
    Suggestion importance[1-10]: 7

    __

    Why: Using logger.exception captures the full traceback and provides context, improving debuggability without altering core logic.

    Medium
    Add context to debug logging

    Enhance the debug log by including relevant context such as test_file and scope to
    quickly identify where the error occurred.

    codeflash/discovery/discover_unit_tests.py [666]

    -logger.debug(str(e))
    +logger.debug("Error processing %r in file %r for scope %r: %s", name, test_file, scope, e)
    Suggestion importance[1-10]: 5

    __

    Why: Including name, test_file, and scope in the debug log makes it easier to pinpoint failures, improving traceability.

    Low
    Possible issue
    Prevent KeyError on missing key

    Guard against missing scope keys in test_functions_by_name to avoid KeyError. Using
    .get(scope, []) returns an empty list when the key is absent.

    codeflash/discovery/discover_unit_tests.py [644]

    -for test_func in test_functions_by_name[scope]:
    +for test_func in test_functions_by_name.get(scope, []):
    Suggestion importance[1-10]: 5

    __

    Why: Guarding the lookup with .get(scope, []) avoids potential KeyError and safely handles missing scopes with minimal impact.

    Low

    @misrasaurabh1 misrasaurabh1 merged commit 1c02b75 into main Jul 3, 2025
    16 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