Skip to content

Conversation

@aseembits93
Copy link
Contributor

@aseembits93 aseembits93 commented Oct 7, 2025

PR Type

Bug fix


Description

  • Prevent None pytest_rootdir on pickle failure

  • Default to empty tests list on exception

  • Improve robustness of pytest discovery cleanup


Diagram Walkthrough

flowchart LR
  A["Run pytest discovery subprocess"] -- "writes pickle" --> B["Read tmp pickle"]
  B -- "load succeeds" --> C["exitcode, tests, pytest_rootdir"]
  B -- "load fails (Exception)" --> D["tests=[], pytest_rootdir=None; exitcode=-1"]
  C -- "cleanup" --> E["unlink tmp pickle"]
  D -- "cleanup" --> E
Loading

File Walkthrough

Relevant files
Bug fix
discover_unit_tests.py
Handle pickle load errors with safe defaults                         

codeflash/discovery/discover_unit_tests.py

  • On pickle load exception, set tests=[] and pytest_rootdir=None
  • Preserve logging of exception and set exitcode=-1
  • Ensure temp pickle file is unlinked in finally
+1/-0     

@github-actions
Copy link

github-actions bot commented Oct 7, 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

Behavior Change

On pickle load failure, pytest_rootdir is now set to None and tests to an empty list; confirm downstream consumers handle None rootdir and do not silently treat this as a successful discovery.

except Exception as e:
    tests, pytest_rootdir = [], None
    logger.exception(f"Failed to discover tests: {e}")
    exitcode = -1
Logging Detail

The exception log may lack context (e.g., tmp file path, subprocess stderr/stdout). Consider including more diagnostics to aid troubleshooting discovery failures.

logger.exception(f"Failed to discover tests: {e}")

@github-actions
Copy link

github-actions bot commented Oct 7, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid clobbering existing results

Preserve the original values of tests and pytest_rootdir when the pickle file is
missing, only defaulting them if they weren't previously set. This prevents
overwriting valid results produced by a previous fallback discovery path and avoids
masking data on non-pickle exceptions.

codeflash/discovery/discover_unit_tests.py [419-423]

 except Exception as e:
-    tests, pytest_rootdir = [], None
+    # Only default if not already set by earlier logic
+    try:
+        tests  # type: ignore[name-defined]
+    except NameError:
+        tests = []
+    try:
+        pytest_rootdir  # type: ignore[name-defined]
+    except NameError:
+        pytest_rootdir = None
     logger.exception(f"Failed to discover tests: {e}")
     exitcode = -1
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly references the new hunk and aims to avoid overwriting existing tests/pytest_rootdir, which could be beneficial, but it assumes prior initialization that isn't evident here and introduces name-checking complexity. Impact is moderate and context-dependent, so the improvement is not critical.

Low

Copy link
Contributor

@KRRT7 KRRT7 left a comment

Choose a reason for hiding this comment

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

LGTM, however I think we should get to the root cause as to why this happened

@aseembits93 aseembits93 merged commit 22cc41b into main Oct 8, 2025
19 of 22 checks passed
@KRRT7 KRRT7 deleted the fix-pytest-rootdir branch October 8, 2025 04:52
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