Repository navigation
save one failure snapshot per test, not one per run - #15061
Conversation
Nir-Az
left a comment
Nir-Az
left a comment
There was a problem hiding this comment.
Auto-generated review by rs-agentic bot
Overall this is a clean and well-motivated fix — changing from a single boolean flag to a per-test-name set is the right approach to allow each test to save its own failure snapshot. A few observations below.
|
|
||
|
|
||
| _snapshot_saved = False | ||
| _snapshot_saved = set() |
There was a problem hiding this comment.
[rs-agentic bot] _snapshot_saved is a module-level set mutated without any lock. The check-then-add pattern (if name in _snapshot_saved … _snapshot_saved.add(name)) is not atomic. If tests ever run concurrently (e.g. via pytest-xdist or any future parallel runner), two threads could both pass the guard and both save a snapshot.
If concurrent execution is possible, consider protecting the guard with a threading.Lock:
import threading
_snapshot_lock = threading.Lock()
_snapshot_saved = set()
# inside the function:
with _snapshot_lock:
if name in _snapshot_saved:
return
_snapshot_saved.add(name)If the test framework is strictly single-threaded this is fine as-is, but worth a comment.
There was a problem hiding this comment.
concurrent execution not possible
| """ | ||
| global _snapshot_saved | ||
| if _snapshot_saved: | ||
| name = os.path.basename( test_file ).replace( '.py', '' ) |
There was a problem hiding this comment.
[rs-agentic bot] 💡 Enhancement — use os.path.splitext for robustness: .replace('.py', '') does a string replace, which would incorrectly strip .py if it appeared anywhere else in the filename (e.g. pytest-helper.py.bak). The idiomatic alternative is:
name = os.path.splitext(os.path.basename(test_file))[0]This only removes the final extension and is the standard pattern in the stdlib.
There was a problem hiding this comment.
won't change, just a line moved
| cv2.imwrite( filename, image ) | ||
| log.i( f"Saved failure snapshot: {filename}" ) | ||
| _snapshot_saved = True | ||
| _snapshot_saved.add( name ) |
There was a problem hiding this comment.
[rs-agentic bot] 🔍 Module-level state persists across the entire process lifetime: Because _snapshot_saved is a module-level set, it is shared and never reset for the lifetime of the Python process. This means:
- If a test is re-run within the same session (e.g.
pytest --count,rerun-failuresplugin, or interactive debugging), the second run will not save a snapshot even if it fails — the name is already in the set.
If repeated-run support matters, consider clearing the set in a pytest fixture (e.g. session-scoped autouse) or documenting this intentional "first failure only" semantic in the docstring.
There was a problem hiding this comment.
Known limitation, not a bug
No description provided.