Added a load all plugins test. #5878
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a test to verify that all plugins in the beetsplug namespace can be imported without errors. This test helps catch import issues early and ensures plugin compatibility.
- Adds a new test class
TestImportAllPluginswith functionality to discover and load all available plugins - Implements error handling for missing optional dependencies by checking module availability
- Updates the changelog to document this new test addition
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/test_plugins.py | Adds comprehensive test to import all beetsplug namespace plugins with dependency checking |
| docs/changelog.rst | Documents the addition of the new plugin import test |
Comments suppressed due to low confidence (1)
test/test_plugins.py:581
- The method name '_is_spec_available' is ambiguous. Consider renaming to '_is_module_available' since it checks module availability, not specification availability.
def _is_spec_available(self, spec_name):
semohr
force-pushed
the
test_load_all_plugins
branch
from
5df46df to
84007ab
Compare
|
It might be worth revisiting the plugin loading infrastructure, particularly with respect to side effects. Ideally, loading and unloading a plugin should be safe and idempotent. However, in practice, it appears that loading a plugin currently introduces significant side effects. A list with plugin loading side effects would be useful for the dev docs. |
to create an instance which is not wanted here.
| def _is_spec_available(self, spec_name): | ||
| """Check if a module is available by its name.""" | ||
| return importlib.util.find_spec(spec_name) is not None |
There was a problem hiding this comment.
What is this for? Didn't pkgutil.iter_modules only return legitimate names?
There was a problem hiding this comment.
Yes but we check for different things here. This function just check if a dependency of a plugin is installed.
Not the prettiest thing but a workaround as we dont know if a plugin needs a dependency and which one is optional. See also above.
| # Check for warnings, is a bit hacky but we can make full use of the beets | ||
| # load_plugins code that way | ||
| records = [] | ||
| pattern = r"ModuleNotFoundError: No module named '(.*?)'" |
There was a problem hiding this comment.
Why are we looking specifically for this issue only? What about any other issues?
There was a problem hiding this comment.
In combination with the _is_spec_available function (see below) this is used to skip plugins only if they fail to load with a ModuleNotFoundError. This only happens if a dependency of a plugin is not installed.
Locally this allows that the tests still finish if e.g. librosa is not installed.
Improved the comment a bit 🙃
There was a problem hiding this comment.
Just exclude them explicitly - I'm not convinced that dynamism is worth the complexity cost
There was a problem hiding this comment.
I would argue keeping a list of plugins with optimal dependencies is a bit more cumbersome? We might be able to parse the pyproject toml for that, seems more complex and unmaintainable if you ask me.
There was a problem hiding this comment.
Wait.. What am I saying... In CI, on ubuntu-latest and Python 3.9 all plugins are expected to load (at least the ones that would get imported by tests). Maybe we can make this test run in this environment only? See .github/workflows/ci.yaml: you can check for IS_MAIN_PYTHON == "true" in the environment.
There was a problem hiding this comment.
And of course we can skip it locally by checking presence of GITHUB_ACTIONS env var (see other tests that use it)
There was a problem hiding this comment.
So you'd skip the tests entirely when running locally? We could do that, but honestly, the current solution, while a bit hacky, still feels more useful.
There was a problem hiding this comment.
So you'd skip the tests entirely when running locally?
Yes, in order to reduce complexity and make these tests deterministic. This way, one at least knows how to activate them locally, simply provide the relevant env variable: IS_MAIN_PYTHON=true poe test.
Generally, tests need to be as dumb as possible: otherwise, if they have conditional logic, we end up in a situation where tests need to be tested.
I think, I see another issue with the current approach: seems like it will ignore legitimate issues due to missing plugin dependencies.
test/test_plugins.py
Outdated
|
|
||
| caplog.set_level(logging.WARNING) | ||
| caplog.clear() | ||
| load_plugins(plugin_name) |
There was a problem hiding this comment.
This test is not guaranteed to run before tests that may import plugins from beetsplug package. Maybe we should unload / unimport plugins before this test rather than after?
There was a problem hiding this comment.
I think at the moment we run all tests are in serial, dont we? Assuming all other tests are also idempotent, it should be fine ;)
But yeah would probably be the best to just clear the plugins before and after. Updated the code to call the cleanup before and after.
There was a problem hiding this comment.
I think at the moment we run all tests are in serial, dont we? Assuming all other tests are also idempotent, it should be fine ;)
I don't think I understand what you mean by idempotency here.
Relatedly, see test_discogs.py as an example:
# test_discogs.py
...
from beetsplug.discogs import DiscogsPlugin
...If there's a problem with the import, this exception will be raised before tests had a chance to start. Is this something we should consider/and or handle somehow?
There was a problem hiding this comment.
If there's a problem with the import, this exception will be raised before tests had a chance to start. Is this something we should consider/and or handle somehow?
Yes but only for plugins which are actually properly tested. I just started this PR as this is actually not the case. Otherwise we would have catched this: #5876
Idempotent just means that test do not have side effects i.e. we clean up after them properly. Which is not the case for plugin test as they e.g. populate the global _classes variable. I'm just saying that we would not have to make sure everything is unloaded before the test, if everything actually was idempotent. More of a side note, not meant too seriously ;)
…lable plugins function.
Adds a test that loads all available plugins in the beetsplug namespace. This came up during #5876.