test: make the flush and logging output tests self-contained - #1386
test: make the flush and logging output tests self-contained#1386NoiceHax wants to merge 2 commits into
Conversation
tests/test_logging_output.py only asserts on the logger output, but its trackers were left with the default CSV output, so every run also appended to emissions.csv in the working directory. _persist_data calls the handlers in a loop and the CSV handler runs first, so once anything went wrong with that shared file the LoggerOutput handler was never reached. flush() and stop() are wrapped in @Suppress(Exception), so the failure was silent and showed up later as a row count assertion, which is what mlco2#1371 reports. Pass save_to_file=False to the three trackers there and check in tearDown that emissions.csv in the working directory was not touched. tests/test_emissions_tracker_flush.py pointed every test at one fixed file name under tempfile.gettempdir(), so two tests counting rows could count in the same file. Give each test its own temporary directory instead. Also add an autouse fixture that stops PeriodicScheduler timers a test left armed. That is only a net, tests are still expected to stop their trackers.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1386 +/- ##
=======================================
Coverage 91.43% 91.43%
=======================================
Files 49 49
Lines 5057 5057
=======================================
Hits 4624 4624
Misses 433 433 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
davidberenstein1957
left a comment
davidberenstein1957
left a comment
There was a problem hiding this comment.
Verified this against current master rather than taking it on trust, and it holds up.
Reproduced the coupling first: the logging tests kept the default CSV output on, so all three trackers also wrote emissions.csv into the repo working directory. _persist_data iterates the handlers with the CSV one first and no per-handler try/except, and both flush() and stop() are wrapped in @Suppress(Exception) — so anything that makes the CSV write raise silently aborts the whole persist call, the LoggerOutput handler after it never runs, and the test fails later on a row count with no traceback. Making the shared CSV unwritable and running the file reproduces all three failures on master, and this branch is unaffected.
On the changes: save_to_file=False plus the tearDown assertion that the working-dir CSV was not touched is the actual root-cause removal, and the assertion keeps it from regressing. The per-test TemporaryDirectory also removes the fixed shared filename under gettempdir(), which mattered for parallel checkouts on one machine. The _stop_leaked_schedulers fixture is correct — PeriodicScheduler arms threading.Timer(interval, self._run), so thread.function.self is the scheduler and .stop() reaches it.
Merged onto current master locally: full suite 628 passed / 20 skipped, and the two files pass 3/3 on repeat runs. Confirmed they no longer write to the working directory. No sleeps, no retries, no library changes.
Worth noting for a follow-up, and not a blocker: the library-side silent failure is untouched. A user with an unwritable CSV path still loses their API and logger output too, with nothing logged. A per-handler try/except plus a warning in _persist_data would turn that class of bug into a visible message.
Thanks for the careful fix.
2 participants
Description
tests/test_logging_output.pyonly checks the logger output, but its trackers kept the default CSV output on, so every run also appended a row toemissions.csvin the working directory._persist_datacalls the output handlers in a loop and the CSV one runs first, so once anything went wrong with that shared file the logger handler was never reached.flush()andstop()are wrapped in@suppress(Exception), so nothing was printed and the test failed later on a row count instead. The three trackers there now passsave_to_file=False, andtearDownchecks thatemissions.csvin the working directory was not touched.tests/test_emissions_tracker_flush.pypointed every test at one fixed file name undertempfile.gettempdir(), so two tests could end up counting rows in the same file. Each test now gets its own temporary directory.tests/conftest.pygets an autouse fixture that stopsPeriodicSchedulertimers a test left armed. The issue asks for that to stay a safety net, so it stays one.One thing I left out on purpose:
_persist_datacould wrap eachhandler.out()call in try/except so a failing output method cannot drop the ones after it. That would have turned this silent failure into a warning, but it changes runtime behaviour, so I would rather do it separately if you want it.Related Issue
#1371
Motivation and Context
These two test modules shared files with each other and with the rest of the suite, which is why they pass alone and fail in a full run. This removes the shared state. It does not touch library code.
How Has This Been Tested?
Ran the full suite five times on master and five times on this branch, on Windows 11 with Python 3.14. Both sets gave the same numbers: the three tests named in the issue passed every time, and the only failure was
test_task_energy_with_live_update_interference, which already fails on master on this machine. So I could not reproduce the intermittent failure here and cannot claim a drop in the failure rate.What I could reproduce is the coupling behind it. Replace
emissions.csvin the repository root with something the CSV handler cannot write to, and all three tests intest_logging_output.pyfail on master, even though none of them assert anything about CSV output. On this branch they pass.Screenshots (if appropriate):
Types of changes
What types of changes does your code introduce? Put an
xin all the boxes that apply:AI Usage Disclosure
Please refer to docs/how-to/ai-policy.md for detailed guidelines on how to disclose AI usage in your PR. Accurately completing this section is mandatory.
An AI agent wrote this patch and this description under my direction, and I checked the reasoning and ran the tests locally before opening the PR.
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.