Skip to content

fix(lastgenre): Reset plugin config in fixtured tests#6386

Merged
snejus merged 1 commit intobeetbox:masterfrom
Nukesor:lastgenre-fix-bleeding-tests
Mar 2, 2026
Merged

fix(lastgenre): Reset plugin config in fixtured tests#6386
snejus merged 1 commit intobeetbox:masterfrom
Nukesor:lastgenre-fix-bleeding-tests

Conversation

@Nukesor
Copy link
Contributor

@Nukesor Nukesor commented Feb 21, 2026

Description

Fixes a bug in the lastgenre plugin, where test state bled into the following fixtures.

Each plugin has a view to the global persisted beets.config field. As a result, config variables that aren't explicitly overwritten are persisted in that global config view.

This commit exposes the lastgenre default config as a static method and uses that default config to reset the state in between fixture calls.

There were 3 tests that depended on count: 10 being set on previous test fixtures, which I adjusted accordingly.

Discovered and discussed in #6317 , see #6317 (comment)

@Nukesor Nukesor requested a review from a team as a code owner February 21, 2026 14:55
@github-actions
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.

@codecov
Copy link

codecov bot commented Feb 21, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.33%. Comparing base (31bbd1f) to head (723b4bb).
⚠️ Report is 2 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6386   +/-   ##
=======================================
  Coverage   69.33%   69.33%           
=======================================
  Files         141      141           
  Lines       18793    18793           
  Branches     3061     3061           
=======================================
  Hits        13031    13031           
  Misses       5117     5117           
  Partials      645      645           
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Nukesor Nukesor force-pushed the lastgenre-fix-bleeding-tests branch 3 times, most recently from 9a8b2ac to 67544a4 Compare February 23, 2026 16:48
@Nukesor
Copy link
Contributor Author

Nukesor commented Feb 23, 2026

The test failure seems to be a flaky one?

@Nukesor Nukesor force-pushed the lastgenre-fix-bleeding-tests branch 2 times, most recently from 3d0414c to 5d74d3e Compare February 28, 2026 13:39
Copilot AI review requested due to automatic review settings February 28, 2026 13:39
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a test isolation bug in the lastgenre plugin where global beets config state leaked between parametrized test fixtures. It extracts the plugin's default config dict into a @staticmethod method, and uses that method in the test harness to reset config state before each fixture run.

Changes:

  • Extracted plugin default config dict into LastGenrePlugin.default_configuration() static method and used it in __init__
  • Added config reset before each test_get_genre parametrized fixture by calling plugin.config.set(LastGenrePlugin.default_configuration())
  • Added explicit "count": 10 to 3 test fixtures that previously relied on leaked state from earlier fixtures

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
beetsplug/lastgenre/__init__.py Extracts default config into a @staticmethod, uses it in __init__
test/plugins/test_lastgenre.py Resets plugin config to defaults before each parametrized test; adds explicit "count": 10 to 3 fixtures

@JOJ0
Copy link
Member

JOJ0 commented Feb 28, 2026

@snejus can you take a look here. I might have missed something when I first set up the pytest fixture based _get_genre tests. Would this be the right way to fix it or do you see another?

@snejus
Copy link
Member

snejus commented Mar 2, 2026

@JOJ0 thanks for a ping.

@Nukesor I think you instead want to use config fixture here - see conftest.py::config.

@Nukesor Nukesor force-pushed the lastgenre-fix-bleeding-tests branch from 5d74d3e to 18d4f96 Compare March 2, 2026 23:10
@Nukesor
Copy link
Contributor Author

Nukesor commented Mar 2, 2026

Thanks for the hint @snejus

I added a new config_per_test fixture, as the existing config fixture has a module scope and wouldn't fix the issue.
I also tried to set config to function scope, but that broke some othere tests, specifically the distance test suite.

@Nukesor Nukesor force-pushed the lastgenre-fix-bleeding-tests branch from 18d4f96 to a9a108d Compare March 2, 2026 23:11
@snejus
Copy link
Member

snejus commented Mar 2, 2026

You can redefine it in test_lastgenre.py module in the following way:

@pytest.fixture
def config(config):
    return config

I will be making some changes to how config is used by the tests so best to have this change at the module level for now!

@snejus
Copy link
Member

snejus commented Mar 2, 2026

I also tried to set config to function scope, but that broke some othere tests, specifically the distance test suite.

This would also slow down many of the tests as config initialisation is fairly time-consuming operation.

@Nukesor Nukesor force-pushed the lastgenre-fix-bleeding-tests branch from a9a108d to 0763e64 Compare March 2, 2026 23:40
@Nukesor
Copy link
Contributor Author

Nukesor commented Mar 2, 2026

You can redefine it in test_lastgenre.py module in the following way:

That's a neat trick. Thanks.

Adjusted my MR accordingly :)

@Nukesor Nukesor force-pushed the lastgenre-fix-bleeding-tests branch from 0763e64 to 7afe965 Compare March 2, 2026 23:42
@Nukesor Nukesor force-pushed the lastgenre-fix-bleeding-tests branch from 7afe965 to 723b4bb Compare March 2, 2026 23:43
@snejus
Copy link
Member

snejus commented Mar 2, 2026

my MR

I see you're also forced to use GitLab for work? 😁

@snejus snejus enabled auto-merge March 2, 2026 23:45
@snejus snejus merged commit 86fac4b into beetbox:master Mar 2, 2026
14 checks passed
@JOJ0
Copy link
Member

JOJ0 commented Mar 3, 2026

Niiiice! Thanks @snejus. Thanks @Nukesor.

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.

4 participants