Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip autobpm tests if librosa isn't available #5516

Merged
merged 1 commit into from
Nov 30, 2024

Conversation

stefanor
Copy link
Contributor

@stefanor stefanor commented Nov 25, 2024

Description

Debian doesn't have librosa. Allow the tests to continue.

Copy link

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.

@snejus
Copy link
Member

snejus commented Nov 29, 2024

Does this fix the issue for you?

I'd be surprised if it does - autobpm is not expected to exist as a command. Rather, the tests run beet autobpm. If the dependencies are missing on your end, you can install them using the autobpm extra, see here.

@stefanor
Copy link
Contributor Author

Ah, I misunderstood self.run_command(). Of course it worked by always skipping the tests.

@stefanor stefanor changed the title Skip autobpm tests if the autobpm binary isn't available Skip autobpm tests if librosa isn't available Nov 29, 2024
@stefanor stefanor requested a review from snejus November 29, 2024 17:09
import pytest

from beets.test.helper import ImportHelper, PluginMixin

if not importlib.util.find_spec("librosa"):
Copy link
Member

@snejus snejus Nov 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I initially thought 'why not simple import librosa in a try catch block' but why import it if it's not required in this module.

Note that we always want to run these tests in CI: after this change, if librosa installation was accidentally removed from the CI, these tests would be skipped silently,

See test_lyrics.py for an example condition which checks whether tests are being run in CI or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I initially thought 'why not simple import librosa in a try catch block' but why import it if it's not required in this module.

I had that, and the CI told me not to do it that way :P

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the good ol' F401

Except under GitHub CI, where we expect all tests to run.
Copy link
Member

@snejus snejus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done, thanks!

@snejus snejus merged commit 0eab8b6 into beetbox:master Nov 30, 2024
12 checks passed
@stefanor stefanor deleted the skip-autobpm branch November 30, 2024 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants