file testing issue - tmp file cleanup

[khrystianc]

Description

Fixes #5229

(#5229)

To Do

• Documentation. (If you've added a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
• Changelog. (Add an entry to docs/changelog.rst to the bottom of one of the lists near the top of the document.)
• Tests. (Very much encouraged but not strictly required.)

[snejus]

Hi @khrystianc , thanks for this!

I did a quick test using the same shell command as I mentioned in the issue

for fi in test/plugins/*.py; do
  print Testing $fi && pytest $fi --no-cov &>/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/tmp2nh4cgqg.png
/tmp/tmpb0b2215s.jpg
/tmp/tmpflpki4b3.jpg
/tmp/tmpk_viivu1.jpg
/tmp/tmpoad9cbic.jpg
/tmp/tmps13osk82.png
/tmp/tmps_prgygd.jpg
/usr/bin/rm: remove 7 arguments recursively? y
removed '/tmp/tmp2nh4cgqg.png'
removed '/tmp/tmpb0b2215s.jpg'
removed '/tmp/tmpflpki4b3.jpg'
removed '/tmp/tmpk_viivu1.jpg'
removed '/tmp/tmpoad9cbic.jpg'
removed '/tmp/tmps13osk82.png'
removed '/tmp/tmps_prgygd.jpg'
Testing test/plugins/test_bareasc.py
/tmp/tmp08cv_39g
/tmp/tmp69gvrdko
/tmp/tmpukgzr05o
/usr/bin/rm: remove 3 arguments recursively? y
removed directory '/tmp/tmp08cv_39g/libdir'
removed directory '/tmp/tmp08cv_39g'
removed directory '/tmp/tmp69gvrdko/libdir'
removed directory '/tmp/tmp69gvrdko'
removed directory '/tmp/tmpukgzr05o/libdir'
removed directory '/tmp/tmpukgzr05o'
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/tmp3uz5wqip
/tmp/tmp5cb9i01a
/tmp/tmp6htzlrvh
/tmp/tmpcasn0w7p
/tmp/tmpdswl3q6c
/tmp/tmpf0zeqn3s
/tmp/tmph46_2pl6
/tmp/tmphxyp5but
/tmp/tmpjbab_5fw
/tmp/tmpk6omw8ea
/tmp/tmpq0qzf1dm
/tmp/tmpquflonrj
/tmp/tmp_rnoob49
/tmp/tmps0vvpoh7.jpg
/tmp/tmpu3ay62wr
/tmp/tmpuallp51s.jpg
/tmp/tmpw3d7rxdp
/tmp/tmpzgtf8mu2
/usr/bin/rm: remove 18 arguments recursively? y
removed directory '/tmp/tmp3uz5wqip'
removed directory '/tmp/tmp5cb9i01a'
removed directory '/tmp/tmp6htzlrvh'
removed directory '/tmp/tmpcasn0w7p'
removed directory '/tmp/tmpdswl3q6c'
removed directory '/tmp/tmpf0zeqn3s'
removed directory '/tmp/tmph46_2pl6'
removed directory '/tmp/tmphxyp5but'
removed directory '/tmp/tmpjbab_5fw'
removed directory '/tmp/tmpk6omw8ea'
removed directory '/tmp/tmpq0qzf1dm'
removed directory '/tmp/tmpquflonrj'
removed directory '/tmp/tmp_rnoob49'
removed '/tmp/tmps0vvpoh7.jpg'
removed directory '/tmp/tmpu3ay62wr'
removed '/tmp/tmpuallp51s.jpg'
removed directory '/tmp/tmpw3d7rxdp'
removed directory '/tmp/tmpzgtf8mu2'
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/tmp0wsi_ndn.m3u
/tmp/tmp7jsdhdhu.m3u
/tmp/tmp87a0xpnn.m3u
/tmp/tmpc3xz17ls.m3u
/tmp/tmpcors6_n1.m3u
/tmp/tmpikokxhu3.m3u
/tmp/tmpipeusix7.m3u
/tmp/tmpno9u_kfy.m3u
/tmp/tmpw3k0pbc9.m3u
/tmp/tmpzx73p2os.m3u
/usr/bin/rm: remove 10 arguments recursively? y
removed '/tmp/tmp0wsi_ndn.m3u'
removed '/tmp/tmp7jsdhdhu.m3u'
removed '/tmp/tmp87a0xpnn.m3u'
removed '/tmp/tmpc3xz17ls.m3u'
removed '/tmp/tmpcors6_n1.m3u'
removed '/tmp/tmpikokxhu3.m3u'
removed '/tmp/tmpipeusix7.m3u'
removed '/tmp/tmpno9u_kfy.m3u'
removed '/tmp/tmpw3k0pbc9.m3u'
removed '/tmp/tmpzx73p2os.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/tmpsb3u4lj7
/usr/bin/rm: remove 1 argument recursively? y
removed '/tmp/tmpsb3u4lj7'

it seems like the situation stays the same?

[snejus]

I see that this cleanup only runs when this script is called directly

if __name__ == "__main__":

Since we use pytest to run tests, I think this logic may not run. Have you tried running, say, pytest test/plugins/test_art.py and checking whether the files have been removed?

[khrystianc]

Thank you for the quick response. Let me review and get back to you.

[snejus]

@khrystianc from what I've seen most of the offending tests use inherit from the TestHelper class in beets/test/helpers.py. If you have a look at its setup_beets method I think it gives some clues about what's going on here

    def setup_beets(self, disk=False):
        """Setup pristine global configuration and library for testing.

        Sets ``beets.config`` so we can safely use any functionality
        that uses the global configuration.  All paths used are
        contained in a temporary directory

        Sets the following properties on itself.

        - ``temp_dir`` Path to a temporary directory containing all
          files specific to beets

        - ``libdir`` Path to a subfolder of ``temp_dir``, containing the
          library's media files. Same as ``config['directory']``.

        - ``config`` The global configuration used by beets.

        - ``lib`` Library instance created with the settings from
          ``config``.

        Make sure you call ``teardown_beets()`` afterwards.
        """
        self.create_temp_dir()
        os.environ["BEETSDIR"] = util.py3_path(self.temp_dir)

        self.config = beets.config
        self.config.clear()
        self.config.read()

        self.config["plugins"] = []
        self.config["verbose"] = 1
        self.config["ui"]["color"] = False
        self.config["threaded"] = False

        self.libdir = os.path.join(self.temp_dir, b"libdir")
        os.mkdir(syspath(self.libdir))
        self.config["directory"] = util.py3_path(self.libdir)

        if disk:
            dbpath = util.bytestring_path(self.config["library"].as_filename())
        else:
            dbpath = ":memory:"
        self.lib = Library(dbpath, self.libdir)

Note the last sentence in the docstring with the following warning:

    Make sure you call ``teardown_beets()`` afterwards.

I have a feeling that these tests may be misusing this functionality, where teardown_beets method may not get called?

[bal-e]

I'm really worried about this implementation. If the user has other files named /tmp/tmp*, which is entirely possible, they will get wiped out completely by this. We need a much more sophisticated solution to solve this -- or we should use /tmp/beets-test/* for all our temporary files. Less importantly, the cleanup_tmp_files() function has been duplicated to a bunch of test modules -- why not define it once and import it everywhere else?

[khrystianc]

@bal-e I agree. I see what you mean. If you've got an idea on how to implement this, please give it a shot. If not, I will continue to work this issue and definitely take this observation to into account. Thank you so much

[bal-e]

Shall we close this in favor of #5362 and #5345?

[snejus]

This got superseded by #5345