test: make the flush and logging output tests self-contained - #1386
Open
NoiceHax wants to merge 1 commit into
Open
test: make the flush and logging output tests self-contained#1386NoiceHax wants to merge 1 commit into
NoiceHax wants to merge 1 commit 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.