Make sure tests do not leave any temp files behind (#5345)

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_beets` calls here and there. Unfortunately, it was not so simple. I came across several issues that gave rise to leftover temporary files: 1. `fetchart`, `artresizer` and `play` handling of temporary files. These plugins created isolated 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 to create files in directories determined by their module names. This way we can clean up after them using the new `CleanupModulesMixin`. 2. Tests that ran temporary directories setup twice, running `_common.TestCase.setUp` and `test.helper.TestHelper.setup_beets`. Both of these ran `self.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 `setUp` calls, see - `test/plugins/test_embedart.py` - `test/test_importer.py` - and `test/test_plugins.py` where `setup_beets` was called twice 3. `test/test_config_command.py` attempted to manage the temporary directory by itself, where I found that `tearDown` failed to remove the directory for four tests. Could not figure out the cause, and found that delegating this task to `TestHelper` fixed the issue. 4. Mediafile fixture removal depended on calling `remove_mediafile_fixtures` method, which `test/plugins/test_zero.py` failed to do. I made the fixtures to be created within the same `temp_dir` directory that gets removed in the teardown, so now they are taken care of automatically. In summary, see the test modules that left files behind: ``` Temp files created by test/__init__.py Temp files created by test/plugins/__init__.py Temp files created by test/plugins/lyrics_download_samples.py Temp files created by test/plugins/test_acousticbrainz.py Temp files created by test/plugins/test_advancedrewrite.py Temp files created by test/plugins/test_albumtypes.py Temp files created by test/plugins/test_art.py /tmp/tmp11nicahe.jpg /tmp/tmp1bjmodum.png /tmp/tmped7nhls4.jpg /tmp/tmpflnzr9wz.jpg /tmp/tmpjngkauqs.png /tmp/tmpkzy9mn6t.jpg /tmp/tmpph_wmuea.jpg /tmp/tmps6gk58i_.jpg /tmp/tmpz2eji_o4.jpg Temp files created by test/plugins/test_aura.py Temp files created by test/plugins/test_bareasc.py /tmp/tmphl3kzhug /tmp/tmpnh2q6v02 /tmp/tmpppw5qrhz Temp files created by test/plugins/test_beatport.py Temp files created by test/plugins/test_bucket.py Temp files created by test/plugins/test_convert.py Temp files created by test/plugins/test_discogs.py Temp files created by test/plugins/test_edit.py Temp files created by test/plugins/test_embedart.py /tmp/tmp1ayvqzhx /tmp/tmp58k6mdfx.jpg /tmp/tmp64c2lqiv /tmp/tmp6nar4kr5 /tmp/tmp6u0d5dex /tmp/tmpacoq7w_f /tmp/tmpajnr_sxr /tmp/tmpasj16beh /tmp/tmpboyaixb5 /tmp/tmpcrmcyt5r /tmp/tmpdomje5g3 /tmp/tmplu3o6t6g /tmp/tmpns_xvkns /tmp/tmpo87o1h6o.jpg /tmp/tmpqem39h_j /tmp/tmprlzm18pb /tmp/tmpt22v4u6x /tmp/tmptp3rxdgv Temp files created by test/plugins/test_embyupdate.py Temp files created by test/plugins/test_export.py Temp files created by test/plugins/test_fetchart.py Temp files created by test/plugins/test_filefilter.py Temp files created by test/plugins/test_ftintitle.py Temp files created by test/plugins/test_hook.py Temp files created by test/plugins/test_ihate.py Temp files created by test/plugins/test_importadded.py Temp files created by test/plugins/test_importfeeds.py Temp files created by test/plugins/test_info.py Temp files created by test/plugins/test_ipfs.py Temp files created by test/plugins/test_keyfinder.py Temp files created by test/plugins/test_lastgenre.py Temp files created by test/plugins/test_limit.py Temp files created by test/plugins/test_lyrics.py Temp files created by test/plugins/test_mbsubmit.py Temp files created by test/plugins/test_mbsync.py Temp files created by test/plugins/test_mpdstats.py Temp files created by test/plugins/test_parentwork.py Temp files created by test/plugins/test_permissions.py Temp files created by test/plugins/test_player.py Temp files created by test/plugins/test_playlist.py Temp files created by test/plugins/test_play.py /tmp/tmp6ohknmve.m3u /tmp/tmp8rw2z_j4.m3u /tmp/tmp9vi27ypx.m3u /tmp/tmpa_s66jh8.m3u /tmp/tmpb7h3cn3n.m3u /tmp/tmpexbmqvry.m3u /tmp/tmpinbqrt80.m3u /tmp/tmpql02hax5.m3u /tmp/tmpvbdzprsf.m3u /tmp/tmpzipim36x.m3u Temp files created by test/plugins/test_plexupdate.py Temp files created by test/plugins/test_plugin_mediafield.py Temp files created by test/plugins/test_random.py Temp files created by test/plugins/test_replaygain.py Temp files created by test/plugins/test_smartplaylist.py Temp files created by test/plugins/test_spotify.py Temp files created by test/plugins/test_subsonicupdate.py Temp files created by test/plugins/test_the.py Temp files created by test/plugins/test_thumbnails.py Temp files created by test/plugins/test_types_plugin.py Temp files created by test/plugins/test_web.py Temp files created by test/plugins/test_zero.py /tmp/tmp3ub9xmzy Temp files created by test/rsrc/beetsplug/test.py Temp files created by test/rsrc/convert_stub.py Temp files created by test/testall.py Temp files created by test/test_art_resize.py /tmp/tmp3p7p60ih.jpg /tmp/tmp8exclgit.jpg /tmp/tmpkrrjsitl.jpg /tmp/tmpw6n8ee8e.jpg /tmp/tmpygws_0aw.jpg Temp files created by test/test_autotag.py Temp files created by test/test_config_command.py /tmp/tmp333f0r2j /tmp/tmphr356z5r /tmp/tmporp4rag2 /tmp/tmpy7sjqdsw Temp files created by test/test_datequery.py Temp files created by test/test_dbcore.py Temp files created by test/test_files.py Temp files created by test/test_hidden.py Temp files created by test/test_importer.py /tmp/tmp0m363gfb /tmp/tmp2n3i13mc /tmp/tmpxk3v304s Temp files created by test/test_library.py Temp files created by test/test_logging.py Temp files created by test/test_m3ufile.py Temp files created by test/test_mb.py Temp files created by test/test_metasync.py Temp files created by test/test_pipeline.py Temp files created by test/test_plugins.py /tmp/tmp6pxhx67u /tmp/tmpb8pqi9ui /tmp/tmpcx_658g7 /tmp/tmp_giqb9jz /tmp/tmpgm9xk94_ /tmp/tmpk60l6bt3 /tmp/tmpqoj4la68 /tmp/tmptcdu20rp /tmp/tmpvr7k5shn /tmp/tmpwnfnzs91 Temp files created by test/test_query.py Temp files created by test/test_sort.py Temp files created by test/test_template.py Temp files created by test/test_ui_commands.py /tmp/tmpns2u94w6 Temp files created by test/test_ui_importer.py Temp files created by test/test_ui_init.py Temp files created by test/test_ui.py Temp files created by test/test_util.py Temp files created by test/test_vfs.py ``` And that's what we have right now: ``` Temp files created by test/__init__.py Temp files created by test/plugins/__init__.py Temp files created by test/plugins/lyrics_download_samples.py Temp files created by test/plugins/test_acousticbrainz.py Temp files created by test/plugins/test_advancedrewrite.py Temp files created by test/plugins/test_albumtypes.py Temp files created by test/plugins/test_art.py Temp files created by test/plugins/test_aura.py Temp files created by test/plugins/test_bareasc.py Temp files created by test/plugins/test_beatport.py Temp files created by test/plugins/test_bucket.py Temp files created by test/plugins/test_convert.py Temp files created by test/plugins/test_discogs.py Temp files created by test/plugins/test_edit.py Temp files created by test/plugins/test_embedart.py Temp files created by test/plugins/test_embyupdate.py Temp files created by test/plugins/test_export.py Temp files created by test/plugins/test_fetchart.py Temp files created by test/plugins/test_filefilter.py Temp files created by test/plugins/test_ftintitle.py Temp files created by test/plugins/test_hook.py Temp files created by test/plugins/test_ihate.py Temp files created by test/plugins/test_importadded.py Temp files created by test/plugins/test_importfeeds.py Temp files created by test/plugins/test_info.py Temp files created by test/plugins/test_ipfs.py Temp files created by test/plugins/test_keyfinder.py Temp files created by test/plugins/test_lastgenre.py Temp files created by test/plugins/test_limit.py Temp files created by test/plugins/test_lyrics.py Temp files created by test/plugins/test_mbsubmit.py Temp files created by test/plugins/test_mbsync.py Temp files created by test/plugins/test_mpdstats.py Temp files created by test/plugins/test_parentwork.py Temp files created by test/plugins/test_permissions.py Temp files created by test/plugins/test_player.py Temp files created by test/plugins/test_playlist.py Temp files created by test/plugins/test_play.py Temp files created by test/plugins/test_plexupdate.py Temp files created by test/plugins/test_plugin_mediafield.py Temp files created by test/plugins/test_random.py Temp files created by test/plugins/test_replaygain.py Temp files created by test/plugins/test_smartplaylist.py Temp files created by test/plugins/test_spotify.py Temp files created by test/plugins/test_subsonicupdate.py Temp files created by test/plugins/test_the.py Temp files created by test/plugins/test_thumbnails.py Temp files created by test/plugins/test_types_plugin.py Temp files created by test/plugins/test_web.py Temp files created by test/plugins/test_zero.py Temp files created by test/rsrc/beetsplug/test.py Temp files created by test/rsrc/convert_stub.py Temp files created by test/testall.py Temp files created by test/test_art_resize.py Temp files created by test/test_autotag.py Temp files created by test/test_config_command.py Temp files created by test/test_datequery.py Temp files created by test/test_dbcore.py Temp files created by test/test_files.py Temp files created by test/test_hidden.py Temp files created by test/test_importer.py Temp files created by test/test_library.py Temp files created by test/test_logging.py Temp files created by test/test_m3ufile.py Temp files created by test/test_mb.py Temp files created by test/test_metasync.py Temp files created by test/test_pipeline.py Temp files created by test/test_plugins.py Temp files created by test/test_query.py Temp files created by test/test_sort.py Temp files created by test/test_template.py Temp files created by test/test_ui_commands.py Temp files created by test/test_ui_importer.py Temp files created by test/test_ui_init.py Temp files created by test/test_ui.py Temp files created by test/test_util.py Temp files created by test/test_vfs.py ``` Note that the command which provides the output is now available through `poe`.
beetbox · Jul 18, 2024 · 1163645 · 1163645
2 parents 61e885c + 83f1016
commit 1163645
Show file tree

Hide file tree

Showing 17 changed files with 181 additions and 136 deletions.
diff --git a/beets/test/helper.py b/beets/test/helper.py
@@ -29,6 +29,7 @@
 - The `TestHelper` class encapsulates various fixtures that can be set up.
 """
 
+from __future__ import annotations
 
 import os
 import os.path
@@ -39,7 +40,9 @@
 from enum import Enum
 from io import StringIO
 from tempfile import mkdtemp, mkstemp
+from typing import ClassVar
 
+import responses
 from mediafile import Image, MediaFile
 
 import beets
@@ -49,7 +52,12 @@
 from beets.library import Album, Item, Library
 from beets.test import _common
 from beets.ui.commands import TerminalImportSession
-from beets.util import MoveOperation, bytestring_path, syspath
+from beets.util import (
+    MoveOperation,
+    bytestring_path,
+    clean_module_tempdir,
+    syspath,
+)
 
 
 class LogCapture(logging.Handler):
@@ -397,18 +405,14 @@ def add_album_fixture(
         return self.lib.add_album(items)
 
     def create_mediafile_fixture(self, ext="mp3", images=[]):
-        """Copies a fixture mediafile with the extension to a temporary
-        location and returns the path.
-
-        It keeps track of the created locations and will delete the with
-        `remove_mediafile_fixtures()`
+        """Copy a fixture mediafile with the extension to `temp_dir`.
 
         `images` is a subset of 'png', 'jpg', and 'tiff'. For each
         specified extension a cover art image is added to the media
         file.
         """
         src = os.path.join(_common.RSRC, util.bytestring_path("full." + ext))
-        handle, path = mkstemp()
+        handle, path = mkstemp(dir=self.temp_dir)
         path = bytestring_path(path)
         os.close(handle)
         shutil.copyfile(syspath(src), syspath(path))
@@ -424,17 +428,8 @@ def create_mediafile_fixture(self, ext="mp3", images=[]):
             mediafile.images = imgs
             mediafile.save()
 
-        if not hasattr(self, "_mediafile_fixtures"):
-            self._mediafile_fixtures = []
-        self._mediafile_fixtures.append(path)
-
         return path
 
-    def remove_mediafile_fixtures(self):
-        if hasattr(self, "_mediafile_fixtures"):
-            for path in self._mediafile_fixtures:
-                os.remove(syspath(path))
-
     def _get_item_count(self):
         if not hasattr(self, "__item_count"):
             count = 0
@@ -925,3 +920,39 @@ def _make_album_match(self, artist, album, tracks, distance=0, missing=0):
             albumtype="soundtrack",
             data_source="match_source",
         )
+
+
+class FetchImageHelper:
+    """Helper mixin for mocking requests when fetching images
+    with remote art sources.
+    """
+
+    @responses.activate
+    def run(self, *args, **kwargs):
+        super().run(*args, **kwargs)
+
+    IMAGEHEADER = {
+        "image/jpeg": b"\x00" * 6 + b"JFIF",
+        "image/png": b"\211PNG\r\n\032\n",
+    }
+
+    def mock_response(self, url, content_type="image/jpeg", file_type=None):
+        if file_type is None:
+            file_type = content_type
+        responses.add(
+            responses.GET,
+            url,
+            content_type=content_type,
+            # imghdr reads 32 bytes
+            body=self.IMAGEHEADER.get(file_type, b"").ljust(32, b"\x00"),
+        )
+
+
+class CleanupModulesMixin:
+    modules: ClassVar[tuple[str, ...]]
+
+    @classmethod
+    def tearDownClass(cls) -> None:
+        """Remove files created by the plugin."""
+        for module in cls.modules:
+            clean_module_tempdir(module)
diff --git a/beets/util/__init__.py b/beets/util/__init__.py
@@ -13,6 +13,7 @@
 # included in all copies or substantial portions of the Software.
 
 """Miscellaneous utility functions."""
+from __future__ import annotations
 
 import errno
 import fnmatch
@@ -26,9 +27,11 @@
 import tempfile
 import traceback
 from collections import Counter, namedtuple
+from contextlib import suppress
 from enum import Enum
 from logging import Logger
 from multiprocessing.pool import ThreadPool
+from pathlib import Path
 from typing import (
     Any,
     AnyStr,
@@ -58,6 +61,7 @@
 WINDOWS_MAGIC_PREFIX = "\\\\?\\"
 T = TypeVar("T")
 Bytes_or_String: TypeAlias = Union[str, bytes]
+PathLike = Union[str, bytes, Path]
 
 
 class HumanReadableException(Exception):
@@ -1076,3 +1080,46 @@ def __get__(self, instance, owner):
             self.cache[owner] = self.getter(owner)
 
         return self.cache[owner]
+
+
+def get_module_tempdir(module: str) -> Path:
+    """Return the temporary directory for the given module.
+
+    The directory is created within the `/tmp/beets/<module>` directory on
+    Linux (or the equivalent temporary directory on other systems).
+
+    Dots in the module name are replaced by underscores.
+    """
+    module = module.replace("beets.", "").replace(".", "_")
+    return Path(tempfile.gettempdir()) / "beets" / module
+
+
+def clean_module_tempdir(module: str) -> None:
+    """Clean the temporary directory for the given module."""
+    tempdir = get_module_tempdir(module)
+    shutil.rmtree(tempdir, ignore_errors=True)
+    with suppress(OSError):
+        # remove parent (/tmp/beets) directory if it is empty
+        tempdir.parent.rmdir()
+
+
+def get_temp_filename(
+    module: str,
+    prefix: str = "",
+    path: PathLike | None = None,
+    suffix: str = "",
+) -> bytes:
+    """Return temporary filename for the given module and prefix.
+
+    The filename starts with the given `prefix`.
+    If 'suffix' is given, it is used a the file extension.
+    If 'path' is given, we use the same suffix.
+    """
+    if not suffix and path:
+        suffix = Path(os.fsdecode(path)).suffix
+
+    tempdir = get_module_tempdir(module)
+    tempdir.mkdir(parents=True, exist_ok=True)
+
+    _, filename = tempfile.mkstemp(dir=tempdir, prefix=prefix, suffix=suffix)
+    return bytestring_path(filename)
diff --git a/beets/util/artresizer.py b/beets/util/artresizer.py
@@ -22,11 +22,10 @@
 import re
 import subprocess
 from itertools import chain
-from tempfile import NamedTemporaryFile
 from urllib.parse import urlencode
 
 from beets import logging, util
-from beets.util import bytestring_path, displayable_path, syspath
+from beets.util import displayable_path, get_temp_filename, syspath
 
 PROXY_URL = "https://images.weserv.nl/"
 
@@ -48,15 +47,6 @@ def resize_url(url, maxwidth, quality=0):
     return "{}?{}".format(PROXY_URL, urlencode(params))
 
 
-def temp_file_for(path):
-    """Return an unused filename with the same extension as the
-    specified path.
-    """
-    ext = os.path.splitext(path)[1]
-    with NamedTemporaryFile(suffix=os.fsdecode(ext), delete=False) as f:
-        return bytestring_path(f.name)
-
-
 class LocalBackendNotAvailableError(Exception):
     pass
 
@@ -141,7 +131,9 @@ def resize(
         Use the ``magick`` program or ``convert`` on older versions. Return
         the output path of resized image.
         """
-        path_out = path_out or temp_file_for(path_in)
+        if not path_out:
+            path_out = get_temp_filename(__name__, "resize_IM_", path_in)
+
         log.debug(
             "artresizer: ImageMagick resizing {0} to {1}",
             displayable_path(path_in),
@@ -208,7 +200,8 @@ def get_size(self, path_in):
             return None
 
     def deinterlace(self, path_in, path_out=None):
-        path_out = path_out or temp_file_for(path_in)
+        if not path_out:
+            path_out = get_temp_filename(__name__, "deinterlace_IM_", path_in)
 
         cmd = self.convert_cmd + [
             syspath(path_in, prefix=False),
@@ -366,7 +359,9 @@ def resize(
         """Resize using Python Imaging Library (PIL).  Return the output path
         of resized image.
         """
-        path_out = path_out or temp_file_for(path_in)
+        if not path_out:
+            path_out = get_temp_filename(__name__, "resize_PIL_", path_in)
+
         from PIL import Image
 
         log.debug(
@@ -442,7 +437,9 @@ def get_size(self, path_in):
             return None
 
     def deinterlace(self, path_in, path_out=None):
-        path_out = path_out or temp_file_for(path_in)
+        if not path_out:
+            path_out = get_temp_filename(__name__, "deinterlace_PIL_", path_in)
+
         from PIL import Image
 
         try:

diff --git a/beetsplug/fetchart.py b/beetsplug/fetchart.py
@@ -19,14 +19,13 @@
 import re
 from collections import OrderedDict
 from contextlib import closing
-from tempfile import NamedTemporaryFile
 
 import confuse
 import requests
 from mediafile import image_mime_type
 
 from beets import config, importer, plugins, ui, util
-from beets.util import bytestring_path, sorted_walk, syspath
+from beets.util import bytestring_path, get_temp_filename, sorted_walk, syspath
 from beets.util.artresizer import ArtResizer
 
 try:
@@ -412,17 +411,17 @@ def fetch_image(self, candidate, plugin):
                         ext,
                     )
 
-                suffix = os.fsdecode(ext)
-                with NamedTemporaryFile(suffix=suffix, delete=False) as fh:
+                filename = get_temp_filename(__name__, suffix=ext.decode())
+                with open(filename, "wb") as fh:
                     # write the first already loaded part of the image
                     fh.write(header)
                     # download the remaining part of the image
                     for chunk in data:
                         fh.write(chunk)
                 self._log.debug(
-                    "downloaded art to: {0}", util.displayable_path(fh.name)
+                    "downloaded art to: {0}", util.displayable_path(filename)
                 )
-                candidate.path = util.bytestring_path(fh.name)
+                candidate.path = util.bytestring_path(filename)
                 return
 
         except (OSError, requests.RequestException, TypeError) as exc:

diff --git a/beetsplug/play.py b/beetsplug/play.py
@@ -18,12 +18,12 @@
 import shlex
 import subprocess
 from os.path import relpath
-from tempfile import NamedTemporaryFile
 
 from beets import config, ui, util
 from beets.plugins import BeetsPlugin
 from beets.ui import Subcommand
 from beets.ui.commands import PromptChoice
+from beets.util import get_temp_filename
 
 # Indicate where arguments should be inserted into the command string.
 # If this is missing, they're placed at the end.
@@ -194,15 +194,15 @@ def _exceeds_threshold(
     def _create_tmp_playlist(self, paths_list):
         """Create a temporary .m3u file. Return the filename."""
         utf8_bom = config["play"]["bom"].get(bool)
-        m3u = NamedTemporaryFile("wb", suffix=".m3u", delete=False)
+        filename = get_temp_filename(__name__, suffix=".m3u")
+        with open(filename, "wb") as m3u:
+            if utf8_bom:
+                m3u.write(b"\xEF\xBB\xBF")
 
-        if utf8_bom:
-            m3u.write(b"\xEF\xBB\xBF")
+            for item in paths_list:
+                m3u.write(item + b"\n")
 
-        for item in paths_list:
-            m3u.write(item + b"\n")
-        m3u.close()
-        return m3u.name
+        return filename
 
     def before_choose_candidate_listener(self, session, task):
         """Append a "Play" choice to the interactive importer prompt."""

diff --git a/pyproject.toml b/pyproject.toml
@@ -230,6 +230,21 @@ env.OPTS = """
 --cov-context=test
 """
 
+[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"
+
 [tool.black]
 line-length = 80
 target-version = ["py38", "py39", "py310", "py311"]