test/rewrite test suite

[2e0byo]

@tehkillerbee here's a start on the work of refactoring the test suite to be (i) more correct and (ii) easier to maintain.

So far I've not fixed the broken tests, but I've focused on making the current tests descriptive. This is a good exercise and led to finding a few bugs, like #137, where before I'd blindly asserted our behaviour without checking it made sense.

As well as general criticism I'd really appreciate feedback on how clear the new tests are, and how obviously they set out what they're testing. The new tests are in test_library.py: the rest is just cleanup.

We're always going to have some 'magic', but I've taken a few steps to minimise surprises:

• prefer factory functions to predefined test data (so make_tidal_track over tidal_tracks) -> the test is more self-contained, and it's obvious where the data is coming from
• move funny logic into conftest.py
• set specs on all the mocks (basically making them look like the class they're mocking)

I've also fixed a few bugs and done some refactoring.

I mean to chip away at this, but when you're happy with things feel free to merge this pr in (we might as well improve incrementally).

I mean to:

• continue the process for all tests
• fix broken tests and get our coverage up to 100% again
• refactor some of the code which is rather verbose (mostly along the lines of the ifs I refactored in this)
• split out some of the tests of e.g. caching, which have too many assertions
• dig up some PRs I have lying around as branches to start adding in track caching (which I've nearly finished and am using locally)

over the next few months in odd moments, so hopefully there'll be a steady drip of small PRs rather than one massive chunk.

[codecov]

Codecov Report

Attention: Patch coverage is 88.88889% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 95.47%. Comparing base (ece4cdc) to head (0b017fc).
Report is 52 commits behind head on master.

❗ Current head 0b017fc differs from pull request most recent head dee7c26. Consider uploading reports for the commit dee7c26 to get more accurate results

Files	Patch %	Lines
mopidy_tidal/library.py	87.50%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #138      +/-   ##
==========================================
- Coverage   99.91%   95.47%   -4.44%     
==========================================
  Files          14       14              
  Lines        1120     1171      +51     
  Branches      210      234      +24     
==========================================
- Hits         1119     1118       -1     
- Misses          0       43      +43     
- Partials        1       10       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[2e0byo]

sorry, the new tests are kindof buried in trivial changes. You probably want to look over here:
https://github.com/tehkillerbee/mopidy-tidal/blob/test/rewrite-test-suite/tests/test_library.py#L147-L297

This is the style I'm trying to roll out everywhere.

[tehkillerbee]

@2e0byo Thanks, I will take a look at it. If the tests are more descriptive, I am sure it will help with my (and other developers) understanding. Automated tests in python is still a new topic for me but it is very nice to see an experienced python developer working on it!

[2e0byo]

@tehkillerbee this rewrite is 64% complete by files and probably 75% complete by lines. I need to fix up 3.9 (if we still support it) but it might be worth having a look and maybe merging it in in this state.

Two things are still needed to hit the original goal of making the tests obvious:

1. I need to finish the other files, i.e.:

• test_image_getter.py
• test_library.py # partially done
• test_playback.py
• test_playlist_cache.py
• test_playlist.py
• test_search.py

2. mocking python-tidal needs to get easier.

I'm inclined to attack 2. first. It would be cool if we could just do from tidalapi.testing import MockTrack; MockTrack("name") and it would Just Work (TM).

Off the top of my head a slight refactor over at python-tidal (dropping None + hydration) would make this easier. In any case I'm inclined to work on that first and then come back and finish this here. It doesn't make sense that mopidy-tidal is growing code to make mock tidal objects, which everyone who uses tidal can benefit from. It also doesn't make sense that we have to use mocks (because we don't want network access) when most of the object is just data, but I'll raise that over there.

I don't know what you want to do about this PR, but it's a lot of red/green to review. It might be worth just looking at the new test files and highlighting what doesn't make sense, rather than looking at the tests side-by-side. I'm happy to clarify or re-write anything which isn't clear.

[2e0byo]

The test failures are mostly spurious:

• all the integration tests against upstream mopidy have failed, because it now requires python >= 3.11 (poetry takes a strict approach to python versions which is highly annoying, so it refuses to install even on 3.11 because we claim to support old python... oh well)
• codecov/project complains that I've xfailed rather than fixing broken tests, slightly reducing our coverage
• codecov/patch complains that some of the PR isn't tested, which is because... it's testing.

[tehkillerbee]

Hey, good to see you've had time to work on this.

mocking python-tidal needs to get easier.
Agreed, sounds like this is necessary before proceeding with this.

It might be worth just looking at the new test files and highlighting what doesn't make sense, rather than looking at the tests side-by-side. I'm happy to clarify or re-write anything which isn't clear.

Ok I'll do as you suggest. I'll try to get it reviewed this coming week.

It would be nice to get it merged, especially before next mopidy-tidal release. There hasn't been one for a while, but I am planning to release it when I have gotten Hires/HLS stream playback working.

[2e0byo]

Thanks for your time! Sorry this PR's been open for so long...

[tehkillerbee]

This might not be an HTTPError anymore after my latest tidalapi changes.

[tehkillerbee]

@2e0byo I think it looks very good, great work! I like the structure of the new tests added to test_library.py. It looks relatively easy to understand what is going on.

Only minor comments. I think we can get this merged in and then deal with the comments in a later MR.

[2e0byo]

Thanks! I'll go through after work and cleanup.

[2e0byo]

@tehkillerbee I'd like to check that error hasn't changed upstream, then this is good to merge from my POV.

[tehkillerbee]

@2e0byo If you have time, you can rebase to latest master then I will merge this PR before v0.3.5 release 👍

[2e0byo]

@tehkillerbee I'll get on that early next week (away till then). Sorry for the delay.

[2e0byo]

btw [str] (in backend) should be list[str]. Annoyingly [] doesn't mean 'list literal' in a typing context. Fwiw I've made it a tuple anyway, as it might as well be immutable.

[2e0byo]

Lots of tests need fixing up with the latest changes; I'll try to get to it soon. This is rebased, however.

[2e0byo]

login=hack is broken, so if you want to release pronto you should remove it. I see there's now an AUTO method: if you get to this soon, what does that do? (else I can just read the code). We may not need the hack login if we have something better.

[tehkillerbee]

@2e0byo are you sure? Login hack worked fine last time I tested, after refactoring. I tested with PKCE and Oauth. It also requires latest tidalapi from master.

Auto=hack, I just added auto as an alias as it sounds more user friendly.

EDIT: Just tested HACK (AUTO) again and it works fine with both PKCE and OAUTH.

[tehkillerbee]

Thanks for the rebase. No rush with the tests, I'll make a branch and I'll try to take a look at the broken tests.

[2e0byo]

oh that's a nice surprise. I didn't test, just read the code. No wonder I was wrong...

Where the tests are related to changes in python-tidal (and not in test_login_hack) feel free to @pytest.mark.xfail(reason="upstream_changed") them. Testing against python-tidal is currently not trivial and involves far too much mocking, but that's a problem to fix on that side.

[2e0byo]

BTW if you do get round to looking at tests, I think the integration tests are the first thing to fix. They're all broken because the log output has changed, but should be very easy to fix. That would be a nice way to confirm that everything does in fact work.