Test Suite

[2e0byo]

I've had quite a few trains and planes in the last few weeks so I thought I'd write a test suite for mopidy_tidal before doing anything else. (Mostly listening to music offline via the cache :D )

This test suite covers everything except one line, which I think is unreachable. I've made a few small modifications to mopidy_tidal. Could those be reviewed?

I've marked all the lines I can't see how to exercise with # NOTE: unreachable? before them. Could these be reviewed?

I've also found a few errors I think---there are some pytest.mark.xfailed tests which set out what I think should be happening. Could these be looked at?

To run the test you need pytest-cov and pytest-mock (pip). If you like, I can migrate the setup.py over to a pyproject.toml and add them in as dev deps; or maybe there's an older way (I'm only really used to poetry....).

Note that this makes heavy use of mocks and was basically reverse-engineered. Hopefully I've worked out the logic correctly.

I'll add a github actions workflow to run the tests as well at some point. (Unless someone wants to get travis working again. I don't know travis at all.)

I intend to rebase the lazy connection on top of this branch, and then the caching on top of that as I think it should really be implemented as a proper http proxy with tests for flaky connections.

This PR also blackens and isorts everything, and adds a makefile so make lint does the right thing.

[2e0byo]

Oops, some inadvertent commits in here; I'll clean up tomorrow. (Resolved)

[tehkillerbee]

Woah, you are on fire! I will take a look ASAP, although due to family issues it will probably take a couple of days before I have the time to finish review.

[2e0byo]

I've added a github actions workflow and got everything working for python >= 3.8. There are quite a lot of failures for 3.7. Given that 3.7 is quite old, is it worth supporting it?

[2e0byo]

We're up to 100% branch and line coverage!

I've taken some opinionated decisions:

1. dropped travis as I don't know how it works, and it's been failing for ages, and replaced with github actions
2. dropped support for python <3.8 [i.e. not testing against, and no attempt to make the tests run on 3.7]

Ofc if you want to convert this back into tox/travis I'll remove the current matrix actions stuff. I just used what I know.

I note the old travis build had coveralls. Some coverage service would be really good so new PRs can get coverage feedback once this is merged. Purely because I know it, I've added codecov to this branch (you can see the report here). If you want I can look at using coveralls; or if you allow codecov access to your github it should work out of the box.

[2e0byo]

I've added draft docs which will probably need rewriting.

[tehkillerbee]

@2e0byo Regarding pytest. I have not used this framework, do you have a good place to look to get familiar with it?

[tehkillerbee]

We're up to 100% branch and line coverage!

I've taken some opinionated decisions:

1. dropped travis as I don't know how it works, and it's been failing for ages, and replaced with github actions

2. dropped support for python <3.8 [i.e. not testing against, and no attempt to make the tests run on 3.7]

Ofc if you want to convert this back into tox/travis I'll remove the current matrix actions stuff. I just used what I know.

I note the old travis build had coveralls. Some coverage service would be really good so new PRs can get coverage feedback once this is merged. Purely because I know it, I've added codecov to this branch (you can see the report here). If you want I can look at using coveralls; or if you allow codecov access to your github it should work out of the box.

I think this is a good call. I am not familiar with travis etc. and as you say, the existing tests have not worked since forever, so its about time to get it fixed :)

I have not used codecov but lets try to get it integrated with github.

[tehkillerbee]

I've added a github actions workflow and got everything working for python >= 3.8. There are quite a lot of failures for 3.7. Given that 3.7 is quite old, is it worth supporting it?

Ubuntu 20.04 and above supports python 3.8 while Ubuntu 18.04 has 3.6(?). In any case, I think the user can install a supported python version manually, in case he/she does not have python >= 3.8. IMO, we can remove support for Python 3.7 but I am not sure if any of the other contributors and testers has any comments. @BlackLight

[2e0byo]

Thank you for going through all of this! I'll go through the reviews and start sorting things out.

Pytest is a marvellous testing framework. It's actually the framework which was already being used (under its old name py.test, invoked from within tox---which I think is the default for tox anyhow) but I've used its "fixtures" quite extensively. Basically if the argument to a test function is the name of a fixture (a fixture being a callable decorated with pytest.mark.fixture) then the fixture is called, iterated if it returns a generator, and then the result is passed to the test function in place of the argument. Once the test is completed a generator fixture is iterated again, calling any cleanup code. This makes resource sharing a breeze, but more importantly it puts your construction/tear down logic in the fixture (where it belongs) and not the test. Any fixtures defined in conftest.py are available anywhere; else they're defined in the test file itself. The pytest docs are excellent, including the section on fixtures. Fixtures can be provided by plugins, such as the pytest-mock plugin which provides a mocker plugin which is basically unittest.mock, but with automatic teardown of mocks after the test is run (so you don't need context managers).

I intended the tests to function as documentation, so do point out anything unclear and I'll think about rewriting it/documenting it.

In the long run, will you squash-merge? If not, are the commits in this branch clean enough? I tend to commit a lot...

[fmarzocca]

@tehkillerbee did you realize that installing current mopidy-tidal, tidalapi is get downgraded?

I have just installed mopidy-tidal from pip:

sudo pip3 install mopidy-tidal

but it downgraded tidalApi:

Installing collected packages: tidalapi, mopidy-tidal
  Attempting uninstall: tidalapi
    Found existing installation: tidalapi 0.7.0rc1
    Uninstalling tidalapi-0.7.0rc1:
      Successfully uninstalled tidalapi-0.7.0rc1
Successfully installed mopidy-tidal-0.2.7 tidalapi-0.6.10

Should I have to manually upgrade tidalapi to 0.7?

[2e0byo]

@tehkillerbee did you realize that installing current mopidy-tidal, tidalapi is get downgraded?

It seems like the latest mopidy-tidal hasn't been published to pypi yet: it's still at 0.27. However did you mean this to be a new issue rather than on this PR?

[fmarzocca]

However did you mean this to be a new issue rather than on this PR?

Yes. I have reported it here, but I didn't get any answer yet.

[tehkillerbee]

@fmarzocca @2e0byo I did have a similar issue back when I was testing #68 as mentioned here: https://github.com/tehkillerbee/mopidy-tidal/pull/68#issuecomment-1214328258

Back then, I had an issue where the old version of tidalapi was used, if it was already installed. But it should be fixed when 0.7.x support was merged in. Did you install from master or from pypi? I have not updated pypi, simply because so much development has been taking place these last couple of months and I wanted to get everything merged before releasing the next major version.

EDIT: I see that you installed using pip. That would explain why the old version is used. For now, you will have to install using the latest master, until the next pypi release.

[fmarzocca]

For now, you will have to install using the latest master, until the next pypi release.

Thanks. I am getting lost with the versions, but I will try to make order.

[2e0byo]

@fmarzocca btw you can use pip to pull from github as well:

pip install git+https://github.com/tehkillerbee/mopidy-tidal

[blacklight]

IMO, we can remove support for Python 3.7 but I am not sure if any of the other contributors and testers has any comments.

mopidy's default policy is to support the current Debian stable and oldstable branches.

stable = Bullseye, which comes with Python 3.9. oldstable = Buster, which still comes with Python 3.7.

So Python 3.7 is indeed the minimum version requirement for mopidy: https://github.com/mopidy/mopidy/blob/develop/setup.cfg#L36.

I don't think there's anything strictly wrong with a mopidy extension with a Python version requirement higher than that of mopidy itself, but it would probably break the "functional expectations" a bit.

[tehkillerbee]

@2e0byo I finally got time to look at the test suite. After installing the dependencies, tests can be executed as described in the README.

FYI; for me the tests fail with:

================================================== FAILURES ===================================================
____________________________________________ test_newstyle_update _____________________________________________

lru_cache = LruCache()

    @pytest.mark.gt_3_7
    @pytest.mark.gt_3_8
    def test_newstyle_update(lru_cache):
        assert "tidal:uri:val" not in lru_cache
>       lru_cache |= {"tidal:uri:val": "hi", "tidal:uri:otherval": 17}
E       TypeError: unsupported operand type(s) for |=: 'LruCache' and 'dict'

tests/test_lru_cache.py:62: TypeError
============================================== warnings summary ===============================================
tests/test_lru_cache.py:58
  /home/jlinde/git/mopidy-tidal-2e0byo/tests/test_lru_cache.py:58: PytestUnknownMarkWarning: Unknown pytest.mark.gt_3_7 - is this a typo?  You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/stable/how-to/mark.html
    @pytest.mark.gt_3_7

tests/test_backend.py::test_logs_in_only_client_secret
  /home/jlinde/git/mopidy-tidal-2e0byo/mopidy_tidal/backend.py:55: DeprecationWarning: The 'warn' method is deprecated, use 'warning' instead
    logger.warn(

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html

---------- coverage: platform linux, python 3.8.10-final-0 -----------
Coverage HTML written to dir htmlcov

=========================================== short test summary info ===========================================
FAILED tests/test_lru_cache.py::test_newstyle_update - TypeError: unsupported operand type(s) for |=: 'LruCa...
============================ 1 failed, 149 passed, 3 xfailed, 2 warnings in 2.20s =============================

Have you seen the same error?

[tehkillerbee]

IMO, we can remove support for Python 3.7 but I am not sure if any of the other contributors and testers has any comments.

mopidy's default policy is to support the current Debian stable and oldstable branches.

stable = Bullseye, which comes with Python 3.9. oldstable = Buster, which still comes with Python 3.7.

So Python 3.7 is indeed the minimum version requirement for mopidy: https://github.com/mopidy/mopidy/blob/develop/setup.cfg#L36.

I don't think there's anything strictly wrong with a mopidy extension with a Python version requirement higher than that of mopidy itself, but it would probably break the "functional expectations" a bit.

If the tests fail under 3.7, it makes maintaining this version difficult. If only a few tests fail, we can try to fix them for 3.7. Otherwise, we should mention that tests are only supported on python >3.7 and allow users to continue using this version for now. When mopidy removes support for 3.7 we can remove it. How does that sound @2e0byo @BlackLight ?

As a sidenote, it turns out my rpi uses 3.7.x. Probably time to upgrade it ;)

[blacklight]

If it's only a problem with a test then I'm fine with disabling 3.7 support for the tests, as long as most of the features keep working.

Btw I still have many RPis running Buster, and it's very common to have devices running the oldstable - that's I guess the rationale behind supporting the version of Python installed both on stable and oldstable.

[2e0byo]

Righty oh. That makes sense: I don't think we should break actual use cases. I definitely have code running on python 3.6 on one embedded device if it comes to that. I'll mark the failing tests and reword the suggested docs. We might as well have as much test coverage as we can get, but I'm not sure I've got the strength to work out what's actually going wrong...

Also @BlackLight I'm jealous of anyone who has "many rpis". Wish I could get one atm...

[2e0byo]

@2e0byo I finally got time to look at the test suite. After installing the dependencies, tests can be executed as described in the README.

FYI; for me the tests fail with:

================================================== FAILURES ===================================================
____________________________________________ test_newstyle_update _____________________________________________

lru_cache = LruCache()

    @pytest.mark.gt_3_7
    @pytest.mark.gt_3_8
    def test_newstyle_update(lru_cache):
        assert "tidal:uri:val" not in lru_cache
>       lru_cache |= {"tidal:uri:val": "hi", "tidal:uri:otherval": 17}
E       TypeError: unsupported operand type(s) for |=: 'LruCache' and 'dict'

tests/test_lru_cache.py:62: TypeError
============================================== warnings summary ===============================================
tests/test_lru_cache.py:58
  /home/jlinde/git/mopidy-tidal-2e0byo/tests/test_lru_cache.py:58: PytestUnknownMarkWarning: Unknown pytest.mark.gt_3_7 - is this a typo?  You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/stable/how-to/mark.html
    @pytest.mark.gt_3_7

tests/test_backend.py::test_logs_in_only_client_secret
  /home/jlinde/git/mopidy-tidal-2e0byo/mopidy_tidal/backend.py:55: DeprecationWarning: The 'warn' method is deprecated, use 'warning' instead
    logger.warn(

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html

---------- coverage: platform linux, python 3.8.10-final-0 -----------
Coverage HTML written to dir htmlcov

=========================================== short test summary info ===========================================
FAILED tests/test_lru_cache.py::test_newstyle_update - TypeError: unsupported operand type(s) for |=: 'LruCa...
============================ 1 failed, 149 passed, 3 xfailed, 2 warnings in 2.20s =============================

Have you seen the same error?

Interesting. Which python are you running and how are you invoking the tests? This is exactly what this test is trying to catch, but I wonder if it's just a python version which doesn't support |=.

EDIT: sorry, 3.8. That test shouldn't run under 3.8. Did you run it with make test or with pytest tests/ -k "not gt_3_8" ?

[2e0byo]

Right, I've rebased against the latest master and added a test for newstyle track uris. Per that PR, we don't seem to be keeping both apis in the code, and we formally require tidalapi 0.7, so I could just prune out all the old api stuff.

Is this likely to take a while yet to be mergeable? In which case, would you like me to cherry-pick the commits which apply black/isort so that can be merged quickly and new PRs can take advantage of it?

[tehkillerbee]

@2e0byo I finally got time to look at the test suite. After installing the dependencies, tests can be executed as described in the README. FYI; for me the tests fail with:

================================================== FAILURES ===================================================
____________________________________________ test_newstyle_update _____________________________________________

lru_cache = LruCache()

    @pytest.mark.gt_3_7
    @pytest.mark.gt_3_8
    def test_newstyle_update(lru_cache):
        assert "tidal:uri:val" not in lru_cache
>       lru_cache |= {"tidal:uri:val": "hi", "tidal:uri:otherval": 17}
E       TypeError: unsupported operand type(s) for |=: 'LruCache' and 'dict'

tests/test_lru_cache.py:62: TypeError
============================================== warnings summary ===============================================
tests/test_lru_cache.py:58
  /home/jlinde/git/mopidy-tidal-2e0byo/tests/test_lru_cache.py:58: PytestUnknownMarkWarning: Unknown pytest.mark.gt_3_7 - is this a typo?  You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/stable/how-to/mark.html
    @pytest.mark.gt_3_7

tests/test_backend.py::test_logs_in_only_client_secret
  /home/jlinde/git/mopidy-tidal-2e0byo/mopidy_tidal/backend.py:55: DeprecationWarning: The 'warn' method is deprecated, use 'warning' instead
    logger.warn(

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html

---------- coverage: platform linux, python 3.8.10-final-0 -----------
Coverage HTML written to dir htmlcov

=========================================== short test summary info ===========================================
FAILED tests/test_lru_cache.py::test_newstyle_update - TypeError: unsupported operand type(s) for |=: 'LruCa...
============================ 1 failed, 149 passed, 3 xfailed, 2 warnings in 2.20s =============================

Have you seen the same error?

Interesting. Which python are you running and how are you invoking the tests? This is exactly what this test is trying to catch, but I wonder if it's just a python version which doesn't support |=.

EDIT: sorry, 3.8. That test shouldn't run under 3.8. Did you run it with make test or with pytest tests/ -k "not gt_3_8" ?

Ah, perhaps that is why. I initially ran the tests as described in the readme;

pytest tests/ -k "not gt_3_10" --cov=mopidy_tidal --cov-report=html

When running with make test, no tests fail (but the script complains about python binary missing so perhaps we need to make sure python3 is found, if python does not point to python3.

jlinde@jlinde-ThinkPad-T430:~/git/mopidy-tidal-2e0byo$ make test
pytest tests/ \
-k "not gt_$(python --version | sed 's/Python \([0-9]\).\([0-9]*\)\..*/\1_\2/')" \
--cov=mopidy_tidal --cov-report=html --cov-report=xml --cov-report=term-missing --cov-branch
/bin/sh: 2: python: not found
============================================= test session starts =============================================
platform linux -- Python 3.8.10, pytest-7.1.3, pluggy-1.0.0
rootdir: /home/jlinde/git/mopidy-tidal-2e0byo, configfile: pytest.ini
plugins: cov-3.0.0, mock-3.8.2
collected 153 items / 1 deselected / 152 selected                                                             

tests/test_backend.py .........                                                                         [  5%]
tests/test_cache_search_key.py ....                                                                     [  8%]
tests/test_context.py .                                                                                 [  9%]
tests/test_extension.py ...                                                                             [ 11%]
tests/test_full_model_mappers.py ..                                                                     [ 12%]
tests/test_image_getter.py ................                                                             [ 23%]
tests/test_library.py ...x..............................................x.......                        [ 61%]
tests/test_lru_cache.py ..............x                                                                 [ 71%]
tests/test_playback.py ..                                                                               [ 72%]
tests/test_playlist.py ..................                                                               [ 84%]
tests/test_playlist_cache.py ....                                                                       [ 86%]
tests/test_ref_models_mappers.py .                                                                      [ 87%]
tests/test_search.py ...........                                                                        [ 94%]
tests/test_search_cache.py ..                                                                           [ 96%]
tests/test_to_timestamp.py ....                                                                         [ 98%]
tests/test_utils.py ..                                                                                  [100%]

============================================== warnings summary ===============================================
tests/test_lru_cache.py:58
  /home/jlinde/git/mopidy-tidal-2e0byo/tests/test_lru_cache.py:58: PytestUnknownMarkWarning: Unknown pytest.mark.gt_3_7 - is this a typo?  You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/stable/how-to/mark.html
    @pytest.mark.gt_3_7

tests/test_backend.py::test_logs_in_only_client_secret
  /home/jlinde/git/mopidy-tidal-2e0byo/mopidy_tidal/backend.py:55: DeprecationWarning: The 'warn' method is deprecated, use 'warning' instead
    logger.warn(

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html

---------- coverage: platform linux, python 3.8.10-final-0 -----------
Name                                  Stmts   Miss Branch BrPart  Cover   Missing
---------------------------------------------------------------------------------
mopidy_tidal/__init__.py                 25      0      2      0   100%
mopidy_tidal/backend.py                  70      0     16      0   100%
mopidy_tidal/context.py                   7      0      2      0   100%
mopidy_tidal/full_models_mappers.py      39      0     20      0   100%
mopidy_tidal/helpers.py                   9      0      6      0   100%
mopidy_tidal/library.py                 303      0    138      0   100%
mopidy_tidal/lru_cache.py               122      0     34      0   100%
mopidy_tidal/playback.py                 15      0      4      0   100%
mopidy_tidal/playlists.py               139      0     52      0   100%
mopidy_tidal/ref_models_mappers.py       41      0     18      0   100%
mopidy_tidal/search.py                   92      1     40      1    98%   199
mopidy_tidal/utils.py                    11      0      4      0   100%
mopidy_tidal/workers.py                  22      0     16      0   100%
---------------------------------------------------------------------------------
TOTAL                                   895      1    352      1    99%
Coverage HTML written to dir htmlcov
Coverage XML written to file coverage.xml

========================== 149 passed, 1 deselected, 3 xfailed, 2 warnings in 2.24s ===========================

[tehkillerbee]

Right, I've rebased against the latest master and added a test for newstyle track uris. Per that PR, we don't seem to be keeping both apis in the code, and we formally require tidalapi 0.7, so I could just prune out all the old api stuff.

Is this likely to take a while yet to be mergeable? In which case, would you like me to cherry-pick the commits which apply black/isort so that can be merged quickly and new PRs can take advantage of it?

I think we can merge this PR now, unless you plan to change anything wrt. the path to python binary, in case python does not point to python3

-k "not gt_$(python --version | sed 's/Python \([0-9]\).\([0-9]*\)\..*/\1_\2/')" \
--cov=mopidy_tidal --cov-report=html --cov-report=xml --cov-report=term-missing --cov-branch
/bin/sh: 2: python: not found

[2e0byo]

I'll fix that. (Edit: done). It's a bit of a naughty hack with sed tbh... but then sed is POSIX.

Brilliant!