I got a little bit nerdsniped by the problems observed in #5027. In
short, my high-level diagnosis in
https://github.com/beetbox/beets/pull/5027#issuecomment-1857953929 seems
to have been correct: other tests were suppressing the legitimate
failure of a flaky test.
I found the problem by running other tests before the problem test, like
this:
```
$ pytest -k 'test_nonexistant_db or test_delete_removes_item' test/test_ui.py
```
When running `test_nonexistant_db` alone, it fails. When running it like
this with another test that goes first, it passes. That's the problem.
However, `test_delete_removes_item` is just one example that works to
make this problem happen. It appeared that _any_ test in a class that
used our `_common.TestCase` base class had this power. I tracked down
the issue to our `DummyIO` utility, which was having an unintentional
effect even when it was never actually used.
Here's the solution. Instead of restoring `sys.stdin` to
`sys.__stdin__`, we now restore it to whatever it was before we
installed out dummy I/O hooks. This is relevant in pytest, for example,
which installs its *own* `sys.stdin`, which we were then clobbering.
This was leading to the suppression of test failures observed in #5021
and addressed in #5027.
The CI will fail for this PR because it now (correctly) exposes a
failing test. Hopefully by combining this with the fixes in the works in
#5027, we'll be back to a passing test suite. 😃 @Phil305, could
you perhaps help validate that hypothesis?
Edit: @snejus:
I've now consolidated test I/O handling by removing the legacy
`control_stdin`/`capture_stdout` context managers and the custom
`DummyOut` stream, replacing them with a pytest-driven `io` fixture
that:
- provides controllable `stdin` via a lightweight `DummyIn`
- captures `stdout` via `capteesys`
- attaches a `DummyIO` helper to test classes as `self.io`
Fixes a bug where existing tags were set to None, if they weren't whitelisted, but an whitelisted canonicalized parent existed up the tree.
In all other cases, the original genres are canonicalized and considered for the final genre, except in the keep_existing logic branch.
This PR fixes the issue and results in the expected behavior for this combination of options.
For the bug to trigger several conditions had to be met:
- Canonicalization is enabled and a whitelist is specified.
- `force` and `keep_existing` are set. Meaning, that Lastfm is queried for a genre, but the existing genres are still left around when none are found online.
- A release with a non-whitelisted genre exists, but that genre has a whitelisted genre parent up the tree.
- That very release has no genre on lastfm.
This is rather convoluted, but stay with me :D
What would happen is the following:
- `keep_genres` is set to the existing genres, as `force` and `keep_existing` is set.
- Genres for `track`/`album`/`artist` aren't found for this release, as they don't exist in lastfm.
- Then the `keep_existing` logic is entered.
- The old logic only checks if the existing genres have an **exact** match for the whitelist. In contrast to all other code branches, we don't do the `_try_resolve_stage` in case there's no direct match, resulting in no match.
- We continue to the fallback logic, which returns the fallback (`None` in my case)
This patch results in one last try to resolve the existing genres when `keep_existing` is set, which includes canonicalization (if enabled).
* master: (37 commits)
importsource: Test skip, Test reimport-skip
Fix initial importsource plugin #4748 changelog
importsource: fix potential prevent_suggest_removal crash
Ensure that inc are joined with a plus
Add retries for connection errors
Add Usage block to RequestHandler
Refactor HTTP request handling with RequestHandler base class
Make musicbrainzngs dependency optional and requests required
musicbrainz: remove error handling
musicbrainz: access the custom server directly, if configured
musicbrainz: browse directly
musicbrainz: search directly
musicbrainz: lookup recordings directly
musicbrainz: lookup release directly
Move pseudo release lookup under the plugin
Add missing blame ignore revs from musicbrainz plugin
Define MusicBrainzAPI class with rate limiting
Move TimeoutSession under beetsplug._utils
expand tests to include check for track artists
remove changes for lastgenre as there was an existing PR for that work
...
