khrystianc
commented
1 month ago I would like to take a shot at resolving this if it has not already been addressed. Please let me know
Open snejus opened 1 month ago
khrystianc
commented
1 month ago I would like to take a shot at resolving this if it has not already been addressed. Please let me know
snejus
commented
1 month ago 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
commented
3 weeks ago 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.
khrystianc
commented
3 weeks ago 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
commented
1 week ago 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
commented
6 days ago @bal-e, consider installing pytest-xdist and try running poe test -n4 - you should see a significant speedup :)
bal-e
commented
6 days ago 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
commented
6 days ago @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
commented
1 day ago @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
commented
1 day ago 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:
beets.test._common.TestCase and beets.helper.TestHelper, making sure we have just a single implementation of that functionalityunittest.skip by pytest.mark.skip and such.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
commented
1 day ago @bal-e there's actually one test module that already uses pytest: see plugins/test_aura.py
snejus
commented
18 hours ago
I have been running tests across the codebase frequently these days and have just discovered thousands of files in my
/tmpdirectory. For example, I checked which tests fromtest/pluginsdirectory leave files behind: