Centralize test setup #5362

snejus · 2024-07-12T20:07:07Z

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

github-actions · 2024-07-12T20:07:25Z

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.

bal-e

This is an incredibly comprehensive PR! I've left a few notes, but everything looks good. Perhaps you could use pathlib in the functions you've rewritten, so that they don't need to be migrated later? Or we can leave that for the later pathlib-specific PR(s).

test/plugins/test_embedart.py

test/test_plugins.py

test/plugins/test_filefilter.py

test/plugins/test_replaygain.py

Serene-Arc · 2024-07-14T11:03:58Z

I'll try and get my review in over the next day or two! I want to really sit down and go through it all.

snejus · 2024-07-15T03:15:20Z

Perhaps you could use pathlib in the functions you've rewritten, so that they don't need to be migrated later? Or we can leave that for the later pathlib-specific PR(s).

That's a very good point. I was careful to not try and refactor anything to do with paths (out of scope here, and the PR size would have quickly gotten out of hand) but I think it's a good idea to use pathlib in rewrites/new stuff.

For now I made the new methods in ImportHelper to use pathlib and you can see it's already made it cleaner!

bal-e · 2024-07-15T03:50:02Z

That's a very good point. I was careful to not try and refactor anything to do with paths (out of scope here, and the PR size would have quickly gotten out of hand) but I think it's a good idea to use pathlib in rewrites/new stuff.

I agree. When we fully migrate to pytest, we can use its tempdir fixtures which already use pathlib. That should also end up migrating the majority of the tests to pathlib.

test/test_importer.py

bal-e · 2024-07-17T04:14:22Z

I like the new uses of pathlib and f-strings, the code feels a lot cleaner. It also looks like the net line change is becoming even more negative, which is awesome. Good job @snejus!

test/plugins/test_mbsubmit.py

test/plugins/test_replaygain.py

beets/test/helper.py

test/test_importer.py

Serene-Arc · 2024-07-18T05:23:42Z

I think it would be good if I opened a pathlib PR (as I keep saying I will) after this PR is done. Like this PR, it will have a lot of small line changes that are just replacing function calls and simplifying, but this PR being merged will make that shorter and easier to parse.

…orter implementations

A couple of `ConfigTest` would previously fail locally since they somehow depended on the local environment to work fine. This commmit decouples them from any environment by setting up patching of the environment variables properly.

This aids with the removal of custom import files prep implementations.

And rewrite the tests since they were far too confusing to follow.

"test" plugin was not properly cleaned up after loading and would therefore stay around and picked up by other tests.

A constant `preload_plugin` is used to disable loading the plugin in the `setUp` initialisation, allowing the plugin to be loaded manually by the tests. Also added a cleanup instruction to remove listeners from configured plugins, and removed this logic from several tests.

bal-e

Amazing work @snejus!

Outdated

Serene-Arc · 2024-07-31T06:04:45Z

I have no objections! Merge whenever you'd like @snejus

snejus self-assigned this Jul 12, 2024

snejus changed the base branch from master to cleanup-tests July 12, 2024 20:07

snejus requested a review from Serene-Arc July 12, 2024 20:07

snejus force-pushed the centralize-test-setup branch 6 times, most recently from 604a683 to eec8c19 Compare July 13, 2024 18:18

bal-e reviewed Jul 14, 2024

View reviewed changes

test/plugins/test_embedart.py Show resolved Hide resolved

test/test_plugins.py Show resolved Hide resolved

test/plugins/test_filefilter.py Outdated Show resolved Hide resolved

test/plugins/test_replaygain.py Outdated Show resolved Hide resolved

snejus force-pushed the centralize-test-setup branch 3 times, most recently from 1153070 to 179d4d9 Compare July 15, 2024 01:56

snejus force-pushed the centralize-test-setup branch from 179d4d9 to 3e188d6 Compare July 15, 2024 03:16

snejus mentioned this pull request Jul 16, 2024

Enable skipped tests #5363

Draft

snejus force-pushed the centralize-test-setup branch 2 times, most recently from ff9f479 to b91a7f9 Compare July 16, 2024 15:04

bal-e reviewed Jul 17, 2024

View reviewed changes

test/test_importer.py Outdated Show resolved Hide resolved

Serene-Arc previously requested changes Jul 18, 2024

View reviewed changes

test/plugins/test_mbsubmit.py Show resolved Hide resolved

test/plugins/test_replaygain.py Outdated Show resolved Hide resolved

beets/test/helper.py Show resolved Hide resolved

test/test_importer.py Outdated Show resolved Hide resolved

Serene-Arc mentioned this pull request Jul 18, 2024

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

Merged

This was referenced Jul 18, 2024

Migrate tests from unittest to pytest #5361

Open

file testing issue - tmp file cleanup #5285

Closed

snejus changed the base branch from cleanup-tests to enable-skipped-tests July 18, 2024 07:15

snejus force-pushed the centralize-test-setup branch from b91a7f9 to 28ae414 Compare July 18, 2024 07:15

snejus added 20 commits July 28, 2024 18:58

Rename LibTestCase to ItemInDBTestCase

b64eaed

Remove redundant setup_beets and teardown_beets instructions

2d5fd90

Remove redundant library setUp instructions

7d7f163

Centralize db setup on disk

16cf8dd

Create PluginTestCase to dedupe plugin setup

432da56

Remove def suite TestLoader definitions

fcff5d7

Move create_importer to ImportHelper

1f8466f

Centralise 'import_dir' creation

41bbb77

Synchronise ImportHelper._create_import_dir and TestHelper.create_imp…

c2fdf98

…orter implementations

Rename _create_import_dir -> prepare_album_for_import

7e444db

Split album and singleton tests in edit and filefilter

8065ff0

Leave a single source of truth for importer setup

f042f5a

Define AsIsImporterMixin to run autotag=False importer

8d85cfd

Fix ConfigTest failures locally

92f79db

A couple of `ConfigTest` would previously fail locally since they somehow depended on the local environment to work fine. This commmit decouples them from any environment by setting up patching of the environment variables properly.

Add setup_singleton_importer for singleton importer

139b54e

Return paths from import files prep

01c24bb

This aids with the removal of custom import files prep implementations.

Standardize ImportPretendTest import files prep

6dda984

Rewrite FileFilterPluginTest

e097f1a

And rewrite the tests since they were far too confusing to follow.

test_ui: Fix Config/Plugin/Completion tests interdependency

9c75513

"test" plugin was not properly cleaned up after loading and would therefore stay around and picked up by other tests.

snejus force-pushed the centralize-test-setup branch 2 times, most recently from ee49c3b to edc820d Compare July 28, 2024 22:04

bal-e approved these changes Jul 29, 2024

View reviewed changes

snejus force-pushed the centralize-test-setup branch from edc820d to 6ed54e2 Compare July 29, 2024 14:21

Configure plugins using PluginMixin.configure_plugin

5f395ab

snejus force-pushed the centralize-test-setup branch from 6ed54e2 to 5f395ab Compare July 29, 2024 14:33

snejus merged commit c05b3cf into master Jul 31, 2024
12 checks passed

snejus deleted the centralize-test-setup branch July 31, 2024 07:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Centralize test setup #5362

Centralize test setup #5362

snejus commented Jul 12, 2024 •

edited

Loading

github-actions bot commented Jul 12, 2024

bal-e left a comment

Serene-Arc commented Jul 14, 2024

snejus commented Jul 15, 2024

bal-e commented Jul 15, 2024

bal-e commented Jul 17, 2024

Serene-Arc commented Jul 18, 2024

bal-e left a comment

Serene-Arc commented Jul 31, 2024

Centralize test setup #5362

Centralize test setup #5362

Conversation

snejus commented Jul 12, 2024 • edited Loading

Details

Test setup

Removals

Importing

github-actions bot commented Jul 12, 2024

bal-e left a comment

Choose a reason for hiding this comment

Serene-Arc commented Jul 14, 2024

snejus commented Jul 15, 2024

bal-e commented Jul 15, 2024

bal-e commented Jul 17, 2024

Serene-Arc commented Jul 18, 2024

bal-e left a comment

Choose a reason for hiding this comment

Serene-Arc commented Jul 31, 2024

snejus commented Jul 12, 2024 •

edited

Loading