Make sure tests do not leave any temp files behind #5345
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
…Case And move the definition to a shared module. The problem was that EmbedartCliTest ran `_common.TestCase.setUp` method which initialised temporary directory for the tests AND ran `helper.TestHelper.setup_beets` method which initialised another set of temporary directories. This meant that the first set of directories could not be tracked down for the cleanup.
This allows to clean them up in art (1) fetching, (2) resizing and (3) deinterlace tests.
This way they get automatically removed with removal of temp_dir. After this change `test/plugins/test_zero.py` does not any more leave any mediafield fixtures hanging around.
Due to some weird race conditions the temporary directories were not getting torn down for four of the tests. I failed to figure out why, and I found that using TestHelper to manage the temporary directory somehow fixes this.
|
Another reason why the switch to pytest would be a great thing. I'll give a review but what is the relation to the other PR, #5285? Do they conflict or are they about different classes? |
|
I think this supersedes #5285 |
And most importantly, remove a redudant invocation of `setup_beets` which left temporary directories hanging around without getting cleaned up.
|
Double checking again, sorry, what are the conflicts with this and #5362 ? |
| [tool.poe.tasks.check-temp-files] | ||
| help = "Run each test module one by one and check for leftover temp files" | ||
| shell = """ | ||
| setopt nullglob | ||
| for file in test/**/*.py; do | ||
| print Temp files created by $file && poe test $file &>/dev/null | ||
| tempfiles=(/tmp/**/tmp* /tmp/beets/**/*) | ||
| if (( $#tempfiles )); then | ||
| print -l $'\t'$^tempfiles | ||
| rm -r --interactive=never $tempfiles &>/dev/null | ||
| fi | ||
| done | ||
| """ | ||
| interpreter = "zsh" | ||
|
|
There was a problem hiding this comment.
Should we add this to our testing CI? It seems like there's no downside.
There was a problem hiding this comment.
I think the downside here is how long it takes to run - I have done it now and it took longer than 4 minutes.
In any case, there's probably little value for it once these issues are fixed - but it's good to have it around in case this issue comes up again.
There was a problem hiding this comment.
That's interesting, why does it take longer to run than the normal test suite?
I suppose it doesn't actually matter, once we switch to pytest, it'll handle all of the temp files and directories and fix issues.
Additionally this issue is more of a nice to have and to stop any possible dependence between test runs because the OS should clear out temp files, at the very least every reboot. That's what the temp files are for.
There was a problem hiding this comment.
That's interesting, why does it take longer to run than the normal test suite?
Because it runs pytest for each of the test files :)
There was a problem hiding this comment.
Do you mean that it runs the entire test suite for each of the files?
There was a problem hiding this comment.
Nope, each invocation runs tests found in each of these files
$ centralize-test-setup print -l test/**/*.py beets 2.0.0 │ 🐍 3.8 🐶
test/__init__.py
test/plugins/__init__.py
test/plugins/lyrics_download_samples.py
test/plugins/test_acousticbrainz.py
test/plugins/test_advancedrewrite.py
test/plugins/test_albumtypes.py
test/plugins/test_art.py
test/plugins/test_aura.py
test/plugins/test_bareasc.py
test/plugins/test_beatport.py
This PR deduplicates shared tests setup that was used across test files. It attempts to centralise the setup into a single source of truth implementation in `beets.test.helper`. This works towards the eventual migration to `pytest` (#5361) making it easier to replace the tests setup with `pytest` fixtures. It builds upon the temporary files cleanup in #5345. ## Details ### Test setup * Mostly duplicate test setup implementations in `beets.test._common.TestCase` and `beets.test.helper.TestHelper` were merged into a single implementation as `beets.test.helper.BeetsTestCase`. * Replaced `unittest.TestCase` and `beets.test.helper.TestHelper` combination with `beets.test.helper.ImportTestCase` * Refactored `test/test_plugins.py` removing duplicate import files setup. * Centralised setting up the database on disk. * Centralised plugin test setup into `beets.test.helpers.PluginMixin`. ### Removals * Around 1/3 of the removed lines correspond to the removal of `def suite()` functions that were previously used for tests collection. * Removed redundant `setUp` and `tearDown` methods from many test files given that the functions that they performed had been centralised in `beets.test.helper`. * Removed redundant library initialisations ### Importing * Centralised import directory creation (only gets created on the first access). * Deduplicated `_setup_import_session` implementations in `TerminalImportSessionSetup` and `ImportHelper`. * Merged `ImportHelper._create_import_dir` and `TestHelper.create_importer` implementations into `ImportHelper.setup_importer`. * Merged various takes on import files prep into `ImportHelper.prepare_albums_for_import` and `ImportHelper.prepare_album_for_import`. * Seeing that many tests used it, defined `AsIsImporterMixin` which provides a method `run_asis_importer` to setup _asis_ (non-autotag) importer and run it
Fixes #5229, is part of #5361 and relates to #5285.
I have to admit thsi was a fairly tough task - I initially assumed that the problem lies
with how the tests are setup, and that we're probably missing some
teardown_beetscallshere and there.
Unfortunately, it was not so simple. I came across several issues that gave rise to
leftover temporary files:
fetchart,artresizerandplayhandling of temporary files. These plugins createdisolated temporary files outside of the directories that tests clean up. You will find
I added a couple of functions (namely
get_module_tempdir) that force these plugins tocreate files in directories determined by their module names. This way we can clean up
after them using the new
CleanupModulesMixin.Tests that ran temporary directories setup twice, running
_common.TestCase.setUpandtest.helper.TestHelper.setup_beets. Both of these ranself.temp_dir = mkdtemp(),therefore the directories created by the initial setup persisted since those have been
overridden and thus unreachable in the teardown. Here, I removed the
setUpcalls, seetest/plugins/test_embedart.pytest/test_importer.pytest/test_plugins.pywheresetup_beetswas called twicetest/test_config_command.pyattempted to manage the temporary directory by itself,where I found that
tearDownfailed to remove the directory for four tests. Could notfigure out the cause, and found that delegating this task to
TestHelperfixed theissue.
Mediafile fixture removal depended on calling
remove_mediafile_fixturesmethod, whichtest/plugins/test_zero.pyfailed to do. I made the fixtures to be created within thesame
temp_dirdirectory that gets removed in the teardown, so now they are taken careof automatically.
In summary, see the test modules that left files behind:
And that's what we have right now:
Note that the command which provides the output is now available through
poe.