Conversation
(1) test_open_extern_handles_os_error_gracefully patched os.startfile unconditionally. os.startfile does not exist on Linux/macOS at all, so unittest.mock.patch() on it raised AttributeError there before the test body even ran. The test now mocks whichever call _open_extern() actually uses for the current sys.platform (os.startfile on win32, subprocess.Popen everywhere else -- matching the source's own branching). (2) tests/conftest.py creates a single shared QApplication before any test module is collected, so the ~28 modules that each do `QApplication.instance() or QApplication([])` all get the same instance instead of racing to create their own, and drains the event loop (processEvents + gc.collect) after the session so widgets scheduled via deleteLater() are actually destroyed before Python starts tearing down objects at interpreter exit. This is aimed at the Linux/macOS "QObject: shared QObject was deleted directly" + segfault (139). NOT included / still open: the Windows crash (PYTEST_EXIT=-1073740791, STATUS_STACK_BUFFER_OVERRUN, ~3s after "365 passed" prints). Investigated and reproduced identically WITH and WITHOUT this conftest.py, so it is a separate issue, not fixed by (2). Also tried forcing an early process exit from pytest_unconfigure (os._exit(), then Win32 TerminateProcess() via ctypes) -- the crash moved earlier each time but a native stack-corruption fault kept triggering, pointing at a genuine memory-safety bug in a native dependency (fitz/PyMuPDF or openpyxl, the two C-extension-heavy libraries this suite exercises) rather than a Qt teardown-order problem. Bisecting across the 58 test files to localize it is out of scope for this PR; tracking separately. Does not block the 1.0.7 release. Verified: full suite still shows "365 passed, 2 skipped" content-wise (only the exit-code tail is the open Windows issue above).
|
Welcome! Thanks for your first pull request in this repository. A maintainer will review it soon. Please make sure:
Thanks for contributing. |
…e FS CI on PR#6 revealed a genuine, platform-specific bug unrelated to the Qt/ native-crash investigation (tracked separately as T-20260926-987151525): test_case_only_rename_is_actionable_not_unchanged and test_batch_rename_dialog_case_mode_and_execution failed on macOS (FAILED ... AssertionError: Status sollte 'ok' sein, war aber 'collision'), passing on Linux and Windows. Root cause: generate_preview()'s "is this a case-only self-rename" check compared os.path.normcase(new_path) == os.path.normcase(original_path). os.path.normcase() reflects the OS's PATH-STRING convention (lowercases on Windows, no-op on POSIX) -- it says nothing about the actual filesystem's case sensitivity. macOS is the classic mismatch: its path convention is POSIX/case-sensitive (normcase is a no-op there), but its default filesystem (APFS) is case-INsensitive, so "sample.txt" and "SAMPLE.txt" are the same file on disk despite differing normcase strings. The self-rename bypass therefore never triggered, and the subsequent `os.path.exists(item.new_path)` check found the (same) file already "there" and flagged it as an external collision. Windows never hit this because its normcase() already lowercases both paths to the same string, so the original bypass caught it there; Linux never hits it because ext4 is genuinely case-sensitive, so os.path.exists() correctly returns False for the differently-cased path. Fix: after the normcase check, also compare via os.path.samefile() when both paths exist -- this checks actual inode/device identity and correctly recognizes the case-only self-rename on any case-insensitive filesystem regardless of the OS's path-string convention. Verified: the two previously-failing tests + full batch-rename test files (22 tests) pass; full suite 365 passed, 2 skipped, no regression.
|
Nachtrag (Commit 2972356): CI-Analyse fand einen ECHTEN, unabhaengigen Bug (nicht das native-Crash-Thema): test_case_only_rename_is_actionable_not_unchanged und test_batch_rename_dialog_case_mode_and_execution schlugen auf macOS fehl (collision statt ok). Ursache: os.path.normcase() ist ein reiner Pfad-STRING-Vergleich nach OS-Konvention (Windows lowercase, POSIX No-Op) und kennt die tatsaechliche Dateisystem-Case-Sensitivitaet nicht -- macOS hat POSIX-Pfadkonvention (normcase No-Op) aber standardmaessig case-insensitives APFS, sodass sample.txt/SAMPLE.txt auf FS-Ebene identisch sind, obwohl ihre normcase-Strings differieren. Fix: zusaetzliche os.path.samefile()-Pruefung (Inode/Device-Identitaet) als Ergaenzung zur normcase-Pruefung in generate_preview(). Windows/Linux unveraendert (dort loeste der alte Check schon korrekt). 22 gezielte + 365 volle Tests lokal gruen. Zum nativen Windows/Linux/macOS-Crash (STATUS_STACK_BUFFER_OVERRUN bzw. Qt-Teardown-Segfault): eigenes Ticket T-20260926-987151525 angelegt, keine Bisektion in diesem PR. Ubuntu-Log dieses PRs zeigt weiterhin 'QObject: shared QObject was deleted directly' + Segfault NACH '365 passed' trotz tests/conftest.py -- mein Fix dort ist also nicht wirksam, das gehoert ebenfalls ins Ticket. |
The shared-QApplication + event-loop-drain fix in tests/conftest.py (introduced in d4e5ec2 to address the Linux/macOS "QObject: shared QObject was deleted directly" + segfault) did not work: CI on this same branch (commit d4e5ec2) still showed the identical warning and segfault on ubuntu-latest AFTER "365 passed, 2 skipped", with the file in place. Shipping code that demonstrably does not do what it claims to do is worse than shipping nothing -- removing it. The underlying crash (now narrowed to Python 3.11 specifically, on all three OSes, per the CI run on d4e5ec2/2972356 -- 10/11 legs green, only the three 3.11 legs red) stays tracked in T-20260926-987151525, to be investigated as a native-module/cp311-wheel issue rather than a Qt object lifetime problem. Kept from this branch: the platform-safe os.startfile/subprocess.Popen mock (fixes a real Linux/macOS AttributeError) and the case-only-rename os.path.samefile() fix (fixes a real macOS test failure) -- both verified independently effective.
|
Nachtrag (Commit 5b8bb0c): tests/conftest.py entfernt (Revert-Commit, kein Force-Push) -- der Qt-Teardown-Fix war nachweislich wirkungslos (CI auf d4e5ec2/2972356 zeigte den identischen Segfault trotz Datei). Drin bleiben: platform-safe os.startfile/subprocess.Popen-Mock und der os.path.samefile()-Fix fuer Case-only-Renames -- beide unabhaengig als wirksam verifiziert. samefile() ist bereits gegen fehlende Zieldatei abgesichert (doppelter os.path.exists()-Guard vor dem Aufruf plus try/except OSError als zusaetzliches Netz). CI-Nachlauf nach dem Revert (36253822709): alle 9 Legs rot, aber jedes einzeln mit '365 passed, 2 skipped' gefolgt vom bekannten, bereits auf master vorbestehenden Crash (Segfault/Abort/exit 1 je nach OS) -- keine inhaltlichen Testfehler, keine neue Regression durch den Revert. Beleg dass der Crash auf master vorbesteht: Lauf 36249100516 (PR#5-Merge, vor jedem meiner Fixes) zeigt denselben Crash z. B. auf ubuntu-latest/Python 3.12. Der Crash ist flakig (schwankende Anzahl betroffener Legs zwischen Laeufen derselben Commits) -- die fruehere Vermutung einer reinen Python-3.11-Signatur war eine Momentaufnahme und ist widerlegt; siehe Ticket T-20260926-987151525 fuer den vollstaendigen, korrigierten Befund. Damit: Motiv-/Icon-Fix (#4), Kontextmenue-Fix (#1), CI-Pillow-Fix (#5) und dieser PR (#6, os.startfile-Portabilitaet + macOS-Case-Rename-Fix) sind inhaltlich vollstaendig; der verbleibende Crash gehoert zu T-20260926-987151525 und existiert unabhaengig von diesem PR bereits auf master. @merge-reviewer bitte pruefen. |
Split out of PR #6 at team-lead's request (measured Windows regression there was traced to the conftest.py Qt-teardown attempt, reverted; this fix is unrelated and independently verified). test_open_extern_handles_os_error_gracefully patched os.startfile unconditionally. os.startfile does not exist on Linux/macOS at all, so unittest.mock.patch() on it raised AttributeError there before the test body even ran. The test now mocks whichever call _open_extern() actually uses for the current sys.platform (os.startfile on win32, subprocess.Popen everywhere else -- matching the source's own branching). Co-authored-by: Lukas Geiger <lukas@um-bruch.org>
…e FS (#8) Split out of PR #6 at team-lead's request (measured Windows regression there was traced to the conftest.py Qt-teardown attempt, reverted; this fix is unrelated and independently verified). test_case_only_rename_is_actionable_not_unchanged and test_batch_rename_dialog_case_mode_and_execution failed on macOS (AssertionError: Status sollte 'ok' sein, war aber 'collision'), passing on Linux and Windows. Root cause: generate_preview()'s "is this a case-only self-rename" check compared os.path.normcase(new_path) == os.path.normcase(original_path). os.path.normcase() reflects the OS's PATH-STRING convention (lowercases on Windows, no-op on POSIX) -- it says nothing about the actual filesystem's case sensitivity. macOS is the classic mismatch: its path convention is POSIX/case-sensitive (normcase is a no-op there), but its default filesystem (APFS) is case-INsensitive, so "sample.txt" and "SAMPLE.txt" are the same file on disk despite differing normcase strings. The self-rename bypass therefore never triggered, and the subsequent os.path.exists(item.new_path) check found the (same) file already "there" and flagged it as an external collision. Fix: after the normcase check, also compare via os.path.samefile() when both paths exist -- this checks actual inode/device identity and correctly recognizes the case-only self-rename on any case-insensitive filesystem regardless of the OS's path-string convention. Guarded against a not-yet-existing target with a double os.path.exists() check before the call plus try/except OSError as a safety net. Co-authored-by: Lukas Geiger <lukas@um-bruch.org>
1 participant
Adressiert die drei von dir gemeldeten Punkte teilweise — mit ehrlichem Zwischenstand, nicht vollständig gelöst.
(1) Linux/macOS: AttributeError in test_open_extern_handles_os_error_gracefully — behoben
patch("os.startfile", ...)unbedingt aufgerufen;os.startfileexistiert unter Linux/macOS gar nicht,patch()bricht dort schon vor dem eigentlichen Testkörper mitAttributeErrorab. Test mockt jetzt plattformabhängig genau den Aufrufpfad, den_open_extern()laut Quellcode tatsächlich nimmt (os.startfileunter win32, sonstsubprocess.Popen).(2) Linux/macOS Segfault (139) — Fix versucht, nicht vor Ort verifizierbar
tests/conftest.pyneu: eine einzige gemeinsameQApplication, erzeugt bevor irgendein Testmodul kollektiert wird — die ~28 Module, die je ihr eigenesQApplication.instance() or QApplication([])machen, bekommen dieselbe Instanz statt um die Erzeugung zu wettlaufen. Nach der Session wird die Event-Loop drainiert (processEvents+gc.collect), damit perdeleteLater()vorgemerkte Widgets tatsächlich zerstört werden, bevor Python beim Interpreter-Exit anfängt, Objekte aufzuräumen. Das zielt direkt auf "QObject: shared QObject was deleted directly". Ich habe hier kein Linux/macOS zur Verfügung — bitte auf der entsprechenden CI-Matrix verifizieren.(3) Windows Exit ~3s nach "365 passed" — NICHT gelöst, eigenständiges Problem gefunden
Lokal reproduziert:
PYTEST_EXIT=-1073740791(STATUS_STACK_BUFFER_OVERRUN, ein von Windows erkannter GS-Cookie-Stapelschaden) sowohl mit als auch ohne das neueconftest.py— identisch. Das schließt die Qt-Teardown-Reihenfolge als alleinige Ursache aus. Auch ein erzwungener früher Prozess-Exit auspytest_unconfigure(erstos._exit(), dann Win32TerminateProcess()viactypes) hat es nicht verhindert — der Crash verschob sich nur zeitlich, dieselbe native Stapelschaden-Fault trat trotzdem auf. Das deutet auf einen echten Speicherfehler in einer nativen Abhängigkeit (fitz/PyMuPDF oder openpyxl, die beiden C-Extension-lastigen Bibliotheken dieser Suite) statt auf ein reines Qt-Teardown-Problem hin. Eine Bisektion über die 58 Testdateien zur Lokalisierung sprengt den Rahmen dieses PRs — bitte als eigenständiges Ticket führen. Blockiert 1.0.7 laut deiner Einschätzung nicht.Verifikation
Inhaltlich weiterhin "365 passed, 2 skipped" (mehrfach reproduziert) — nur der Exit-Code-Schwanz auf Windows ist offen, siehe (3).