Ensure that tests do not leave any files behind

[snejus]

I have been running tests across the codebase frequently these days and have just discovered thousands of files in my /tmp directory. For example, I checked which tests from test/plugins directory leave files behind:

for fi in test/plugins/*.py; do 
  print Testing $fi && pytest $fi &>/dev/null
  drs=(/tmp/tmp*)
  if (( $#drs )); then 
    print -l $drs && rm -r $drs
  fi
done

Testing test/plugins/__init__.py
Testing test/plugins/lyrics_download_samples.py
Testing test/plugins/test_acousticbrainz.py
Testing test/plugins/test_advancedrewrite.py
Testing test/plugins/test_albumtypes.py
Testing test/plugins/test_art.py
/tmp/tmp2gwz0iin.jpg
/tmp/tmp7gxr676p.jpg
/tmp/tmp88ypd9a4.png
/tmp/tmpc9_dhkt1.jpg
/tmp/tmpjlro_7kl.png
/tmp/tmprehbg2uh.jpg
/tmp/tmps37w1k7g.jpg
/usr/bin/rm: remove 7 arguments recursively? y
removed '/tmp/tmp2gwz0iin.jpg'
removed '/tmp/tmp7gxr676p.jpg'
removed '/tmp/tmp88ypd9a4.png'
removed '/tmp/tmpc9_dhkt1.jpg'
removed '/tmp/tmpjlro_7kl.png'
removed '/tmp/tmprehbg2uh.jpg'
removed '/tmp/tmps37w1k7g.jpg'
Testing test/plugins/test_bareasc.py
/tmp/tmposwaq51j
/tmp/tmpqdoitx3p
/tmp/tmpqj57zdnm
/usr/bin/rm: remove 3 arguments recursively? y
removed directory '/tmp/tmposwaq51j/libdir'
removed directory '/tmp/tmposwaq51j'
removed directory '/tmp/tmpqdoitx3p/libdir'
removed directory '/tmp/tmpqdoitx3p'
removed directory '/tmp/tmpqj57zdnm/libdir'
removed directory '/tmp/tmpqj57zdnm'
Testing test/plugins/test_beatport.py
Testing test/plugins/test_bucket.py
Testing test/plugins/test_convert.py
Testing test/plugins/test_discogs.py
Testing test/plugins/test_edit.py
Testing test/plugins/test_embedart.py
/tmp/tmp04pf5orx
/tmp/tmp0p3zrmvr
/tmp/tmp1c_13z5i
/tmp/tmp1iwishs_.jpg
/tmp/tmpfl7et0y5
/tmp/tmpfu9vpdi9
/tmp/tmph5to5ktl
/tmp/tmpiemkfj5o
/tmp/tmpn__of7he
/tmp/tmpp1hnf3dh
/tmp/tmppumez7dg
/tmp/tmpqxlwwq42.jpg
/tmp/tmpr__5fn99
/tmp/tmptks_h0e7
/tmp/tmpv7f27emo
/tmp/tmpvp2regn7
/tmp/tmpw8dq5diz
/tmp/tmpxy4kl2i8
/usr/bin/rm: remove 18 arguments recursively? y
removed directory '/tmp/tmp04pf5orx'
removed directory '/tmp/tmp0p3zrmvr'
removed directory '/tmp/tmp1c_13z5i'
removed '/tmp/tmp1iwishs_.jpg'
removed directory '/tmp/tmpfl7et0y5'
removed directory '/tmp/tmpfu9vpdi9'
removed directory '/tmp/tmph5to5ktl'
removed directory '/tmp/tmpiemkfj5o'
removed directory '/tmp/tmpn__of7he'
removed directory '/tmp/tmpp1hnf3dh'
removed directory '/tmp/tmppumez7dg'
removed '/tmp/tmpqxlwwq42.jpg'
removed directory '/tmp/tmpr__5fn99'
removed directory '/tmp/tmptks_h0e7'
removed directory '/tmp/tmpv7f27emo'
removed directory '/tmp/tmpvp2regn7'
removed directory '/tmp/tmpw8dq5diz'
removed directory '/tmp/tmpxy4kl2i8'
Testing test/plugins/test_embyupdate.py
Testing test/plugins/test_export.py
Testing test/plugins/test_fetchart.py
Testing test/plugins/test_filefilter.py
Testing test/plugins/test_ftintitle.py
Testing test/plugins/test_hook.py
Testing test/plugins/test_ihate.py
Testing test/plugins/test_importadded.py
Testing test/plugins/test_importfeeds.py
Testing test/plugins/test_info.py
Testing test/plugins/test_ipfs.py
Testing test/plugins/test_keyfinder.py
Testing test/plugins/test_lastgenre.py
Testing test/plugins/test_limit.py
Testing test/plugins/test_lyrics.py
Testing test/plugins/test_mbsubmit.py
Testing test/plugins/test_mbsync.py
Testing test/plugins/test_mpdstats.py
Testing test/plugins/test_parentwork.py
Testing test/plugins/test_permissions.py
Testing test/plugins/test_player.py
Testing test/plugins/test_playlist.py
Testing test/plugins/test_play.py
/tmp/tmp0on9a_ru.m3u
/tmp/tmp2lnwq43r.m3u
/tmp/tmp2rjt0boe.m3u
/tmp/tmp3tenj5ru.m3u
/tmp/tmp_6aguiwh.m3u
/tmp/tmp8ggf8znt.m3u
/tmp/tmpel0a12lq.m3u
/tmp/tmpwd0g39m_.m3u
/tmp/tmpwxpl0eas.m3u
/tmp/tmpxzytwbdp.m3u
/usr/bin/rm: remove 10 arguments recursively? y
removed '/tmp/tmp0on9a_ru.m3u'
removed '/tmp/tmp2lnwq43r.m3u'
removed '/tmp/tmp2rjt0boe.m3u'
removed '/tmp/tmp3tenj5ru.m3u'
removed '/tmp/tmp_6aguiwh.m3u'
removed '/tmp/tmp8ggf8znt.m3u'
removed '/tmp/tmpel0a12lq.m3u'
removed '/tmp/tmpwd0g39m_.m3u'
removed '/tmp/tmpwxpl0eas.m3u'
removed '/tmp/tmpxzytwbdp.m3u'
Testing test/plugins/test_plexupdate.py
Testing test/plugins/test_plugin_mediafield.py
Testing test/plugins/test_random.py
Testing test/plugins/test_replaygain.py
Testing test/plugins/test_smartplaylist.py
Testing test/plugins/test_spotify.py
Testing test/plugins/test_subsonicupdate.py
Testing test/plugins/test_the.py
Testing test/plugins/test_thumbnails.py
Testing test/plugins/test_types_plugin.py
Testing test/plugins/test_web.py
Testing test/plugins/test_zero.py
/tmp/tmp4jxe4d30
/usr/bin/rm: remove 1 argument recursively? y
removed '/tmp/tmp4jxe4d30'

[khrystianc]

I would like to take a shot at resolving this if it has not already been addressed. Please let me know

[snejus]

I would like to take a shot at resolving this if it has not already been addressed. Please let me know

@khrystianc go ahead!!!

[khrystianc]

Submitted the PR in hopes to fix this issue. I was not able to fully recreate it, but I have added code into the files that looked to be creating unnecessary tmp files.

https://github.com/beetbox/beets/pull/5285

[khrystianc]

If my PR does not resolve your issue, the following code can be run in a file to handle tmp file cleanup as well:

import os
import glob

def cleanup_temp_files():
    temp_files = glob.glob('/tmp/testfile*')  # Adjust the pattern to match your temporary test files
    for temp_file in temp_files:
        try:
            os.remove(temp_file)
            print(f'Removed: {temp_file}')
        except OSError as e:
            print(f'Error deleting {temp_file}: {e}')

if __name__ == '__main__':
    cleanup_temp_files()

[bal-e]

I'm very interested in improving the testing infrastructure for all of beets. In particular, I think a better handling of temporary files -- how they are named and created, not just their removal -- would allow us to run tests in parallel, which should significantly cut down the latency of tests locally and on CI. It's a major change, but I'm wondering whether there would be interest in moving in that direction.

[snejus]

@bal-e, consider installing pytest-xdist and try running poe test -n4 - you should see a significant speedup :)

[bal-e]

Yeah, I just discovered pytest-xdist is a thing, and it looks great! I'm thinking of making a big PR for transitioning beets from unittest to pytest (the library not just the util), which would greatly simplify assertions and also let us implement tempdir cleanup really well.

[bal-e]

@snejus I don't know what the specs of the CI runner look like, but using xdist on there could speed stuff up too. It's worth a try, but I think we first need to make sure that the tests don't rely on global state (which is again much easier using pytest fixtures).

[snejus]

@bal-e reagrding this issue specifically, I spent a couple of days looking into these tests that write temp files. In those (easy) cases where tests are responsible for cleaning up, helper.TestHelper.teardown_beets function is available which makes life easy. I can see this being transformed into a pytest context-based fixture which cleans up automatically. For now, though, it's enough to make sure the teardown is appropriately called. Unfortunately, the sad reality is that only the test_bareasc.py module can be fixed this way.

If you have a look at fetchart.py, embedart.py and artresizer.py, you will find that they create temporary files in isolation which aren't that easy to track down for cleanup. In this case the issue lies in the functionality/implementation rather than the test setup, so it's a bit more complicated to fix.

I will soon submit a pull request that addresses the above.

[snejus]

Regarding migration to pytest - I will love seeing this happening, even if that's going to be a monumental task. I think we'd want to think about how can we achieve this iteratively, for example:

1. Deduplicate existing tests, namely functionality in beets.test._common.TestCase and beets.helper.TestHelper, making sure we have just a single implementation of that functionality
2. Replace unittest-specific things like unittest.skip by pytest.mark.skip and such.
3. Replace unittest-specific assert methods by assert calls, like assertEqual(a, b) by assert a == b. I remember working on a sed script that helps with this:

     s/self\.assertTrue.([^)]+)./assert \1/
     s/self\.assertFalse.([^)]+)./assert not \1/
     s/self\.assertIsNone.(.*)\)(, |$)/assert \1 is None/
     s/self\.assertIsNotNone.(.*)\)(, |$)/assert \1 is not None/
     s/self\.assertEqual.([^,]+), (.*).$/assert \1 == \2/
     s/self\.assertNotIn.([^,]+), (.*).$/assert \1 not in \2/
     s/self\.assertIn.([^,]+), (.*).$/assert \1 in \2/
     s/self\.assertGreater.([^,]+), (.*).$/assert \1 > \2/
     s/self\.assertNotEqual.([^,]+), (.*).$/assert \1 != \2/
     s/self\.assertExists.([^)]+)./assert Path(\1).exists()/
     s/self\.assertNotExists.([^)]+)./assert not Path(\1).exists()/
     s/assertIsNone.(.+).$/\1/
   

   Of course it needs to be updated regarding tests in beets.

Once we've replaced all unittest specifics with pytest equivalents (the changes above do not require adjusting the tests structure!), then, I think, we should be able to go ahead with replacing setUp and tearDown implementations with pytest fixtures.

Otherwise, if we go ahead with migrating to pytest fixtures right away, I think we're risking the PRs becoming large with loads of changes in them. Of course, this doesn't apply to tests that have limited usage of unittest - those can be migrated easily I think.

[snejus]

@bal-e there's actually one test module that already uses pytest: see plugins/test_aura.py

[snejus]

See https://github.com/beetbox/beets/pull/5345