Close the fwl logger handlers after each test (#791) - #792
Conversation
3b36e3e to
55cafd5
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #792 +/- ##
=======================================
Coverage 92.65% 92.66%
=======================================
Files 111 111
Lines 15892 15892
Branches 2847 2847
=======================================
+ Hits 14725 14726 +1
+ Misses 1166 1165 -1
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
This depends on #776 being merged first |
|
Merged #776, will now rebase on main and update to also cover the added tests there. |
TestSetupLogger and two TestBootstrapLogger tests attach a FileHandler to the shared 'fwl' logger with a logpath inside an inline `with tempfile.TemporaryDirectory()` but never close it, so the logfile is still open when the directory is torn down. On an NFS TMPDIR the server keeps the open file as a .nfsXXXX entry until the descriptor closes, so rmdir raises OSError [Errno 39] during cleanup and every affected test reports as failed even though its assertions passed. CI misses it because the runners use local-disk temp space. Add a module-level tmpdir_that_closes_logger_files fixture that yields a temp dir and, after the test returns, closes and detaches the 'fwl' handlers before its own `with` block removes the directory. Route every setup_logger / bootstrap_logger test that writes a logfile through it. A plain post-test teardown fixture would be too late: an inline `with` TemporaryDirectory removes the directory when the test method returns, before any fixture teardown runs. pristine_fwl stays for TestBootstrapLogger's setup isolation and is unaffected. Closes #791
55cafd5 to
9281317
Compare
|
Done, ready for review! |
nichollsh
left a comment
There was a problem hiding this comment.
Great! Works for me on the IoA cluster, where the issue with NFS was first noted. Happy for this to be merged.
|
Yes! It seems I didn't finish the comment above where I wanted to say I would rebase on main and then apply the fix to the added tests from the other PR too. That's what I did in the new commit. |
|
Oh wait you referred to the faster CI PR, I was referring to the previous logging one. The faster CI one is not related to this one. |
Description
TestSetupLoggerintests/utils/test_logs.pyerrors on machines whoseTMPDIRis an NFS mount. The tests callsetup_logger(logpath=..., logterm=False)with a path inside an inlinewith tempfile.TemporaryDirectory(), but theFileHandlerthatsetup_loggerattaches to the sharedfwllogger is never closed, so the logfile is still open when the directory is torn down.On a local filesystem an unlinked-but-open file is invisible to
rmdir. On NFS the server keeps the open file as a.nfsXXXXentry until the descriptor closes, sormdirsees a non-empty directory and raisesOSError: [Errno 39] Directory not emptyduring cleanup. Because the failure is inTemporaryDirectoryteardown rather than an assertion, every affected test reports as failed even though its checks passed. CI does not catch this because the runners use local-disk temp space.Fix: add a module-level
tmpdir_that_closes_logger_filesfixture that owns theTemporaryDirectory, yields it to the test, and after the test returns closes and detaches thefwlhandlers before its ownwithblock removes the directory. Everysetup_logger/bootstrap_loggertest that writes a logfile now takes this fixture instead of opening its ownTemporaryDirectory. This covers theTestSetupLoggertests, the twofmttests, and the twoTestBootstrapLoggertests that write a logfile (the latter three were added by #708 / #776).Note on the design:
with tempfile.TemporaryDirectory()inside a test removes the directory when the method returns, which is before any fixture teardown runs. The fixture therefore has to own the directory so the close happens between the test and the removal.TestBootstrapLogger's existingpristine_fwlfixture is kept as-is. It provides a handler-free logger at setup for that class's preconditions; it cannot fix the cleanup itself (same too-late ordering), so its two file-writing tests use the shared fixture for the directory.This branch is rebased onto
mainafter #776 merged, so it applies the fix to the tests that PR added as well.Closes #791
Validation of changes
tests/utils/test_logs.py: 49 tests pass locally;ruff checkandruff format --checkclean.$HOMEon NFS), withTMPDIRpointing at the NFS mount, runningTestSetupLoggerandTestBootstrapLogger:main, post-Configure the fwl logger at CLI entry points (#708) #776):17 failed, 5 passed, withOSError: [Errno 39] Directory not emptyinTemporaryDirectorycleanup. The failures include the twoTestBootstrapLoggerfile-tests added by Configure the fwl logger at CLI entry points (#708) #776. The 5 passes are the tests that never open a FileHandler (the invalid-level test and the console-only bootstrap tests).22 passedon the same NFSTMPDIR.Test configuration: reproduction on a Linux NFS mount (the reported condition); local development checks on macOS with the PROTEUS conda environment (Python 3.12).
Checklist