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

test: add tests to codebase #116

Merged
merged 53 commits into from
Apr 5, 2023
Merged

test: add tests to codebase #116

merged 53 commits into from
Apr 5, 2023

Conversation

marksie1988
Copy link
Collaborator

@marksie1988 marksie1988 commented Jun 28, 2022

Description

Adding tests to the codebase, this should reduce the number of bugs introduced during changes.

Related issues

Motivation and Context

Too many issues / bugs being introduced on each release which is not ideal

How has this been tested

Change List

  • perf!: multiple methods updated to use default Pyarr values for args, allowing reduction in memory usage by dictionaries
  • tests: added tests to whole code base
  • tests: Added nox for testing and updated github workflows to match
  • tests: Run against docker containers where possible to ensure real scenarios, some remain on mock due to complexities with downloads / indexers etc.
  • feat!: errors added where argument pairs are not used together e.g. sort_key and sort_dir
  • feat(request_handler): strip trailing slashes from URL
  • feat(base): added implementation argument to get_notification_schema to allow selection of a single implementation
  • feat(base): added get_import_list_schema
  • feat(base): implemented add_import_list
  • feat(base): added upd_config_download_client
  • feat(base): implemented add_notification
  • feat(base): added id_ to get_root_folder
  • feat(base): added get_command
  • feat(sonarr): get_queue updated to include additional arguments
  • feat(sonarr): added get_parse_title_path
  • feat(sonarr): added option to search TVDB ID in lookup_series
  • feat(sonarr): added add_root_folder
  • feat(sonarr): added get_language_profile, upd_language_profile, del_language_profile, get_language_profile_schema
  • feat(sonarr): get_episode_file updated to allow filter by series id
  • feat(radarr): added add_root_folder
  • feat(readarr): added lookup to search for books and authors
  • feat(readarr): added add_release_profile
  • feat(readarr): added add_delay_profile
  • refactor!: get_calendar changed to require datetime object for date arguments
  • refactor!: multiple methods updated to use same naming and ordering for arguments
  • refactor: multiple methods updated to use Enum to allow easier identification of possible options
  • refactor: get_history updated to allow additional options in SonarrAPI
  • refactor(sonarr)!: updated SonarrSortKeys Enum naming
  • refactor(sonarr): deprecation warning added to get_episode_files_by_series_id
  • refactor(sonarr): deprecation warning added to lookup_series_by_tvdb_id
  • refactor(sonarr): deprecation warning added to get_parsed_title
  • refactor(sonarr): deprecation warning added to get_parsed_path
  • refactor(radarr)!: change db_id to id_ to align rest of API
  • refactor(radarr): deprecation warning added to lookup_movie_by_imdb_id
  • refactor(radarr): deprecation warning added to lookup_movie_by_tmdb_id
  • refactor(radarr): upd_movies updated to show examples of usage
  • refactor(radarr): get_movie_file now able to get multiple movie files
  • refactor(radarr): get_movie_files removed as this would not work use get_movie_file instead
  • refactor(radarr): del_movie now able to delete multiple movie files
  • refactor(radarr): deprecation warning added to del_movies
  • refactor(lidarr)!: add_root_folder arguments re-arranged
  • refactor(lidarr)!: add_artist and add_album changed from using search_term to using id_ (musicbrainz ID) for more predictable results where multiple search items could have previously been returned
  • refactor(lidarr)!: get_wanted updated to be more efficient, options moved to match ordering of other methods
  • refactor(lidarr)!: get_queue updated to be more efficient, options moved to match ordering of other methods
  • refactor(lidarr)!: get_queue_details updated to be more efficient
  • refactor(lidarr): added error to get_tracks if no ids supplied
  • refactor(lidarr): added error to get_track_file if no ids supplied
  • refactor(lidarr): updated get_metadata_profile to use correct return
  • refactor(readarr)!: get_missing updated to be more efficient, options moved to match ordering of other methods
  • refactor(readarr)!: get_cutoff updated to be more efficient, options moved to match ordering of other methods
  • refactor(readarr)!: get_queue updated to be more efficient, options moved to match ordering of other methods
  • refactor(readarr)!: add_author updated search_term to match term format
  • fix: multiple assertion errors resolved
  • fix: delete methods with using data instead of passing the id to the url
  • fix!: blacklist changed to blocklist
  • fix(base): added cutoff to add_quality_profile
  • fix(sonarr): added language_profile_id to add_series
  • fix(sonarr): removed get_quality_profile inherits from base
  • fix(sonarr): upd_tag incorrect url path
  • fix(sonarr)!: add_series changed to remove the use of the _series_json method, now supply a series result from lookup() this also allows more control without Pyarr selecting the first lookup result.
  • fix(radarr): get_movie tmdb not working fixes Type issues with RadarrAPI.get_movie() #119
  • fix(readarr)!: add_quality_profile changed to have additional variables over other Arr's
  • fix(readarr)!: add_book changed to remove the use of the _book_json method, now supply a book result from lookup() this also allows more control without Pyarr selecting the first lookup result.
  • fix(lidarr)!: add_album changed to remove the use of the _book_json method, now supply a book result from lookup() this also allows more control without Pyarr selecting the first lookup result.
  • fix(lidarr): get_album incorrect assertion when returning list or dict
  • fix(lidarr): albumId not working within get_release method
  • chore: added test check to github actions

@marksie1988 marksie1988 added the type/ci CI related changes label Jun 28, 2022
@sourcery-ai
Copy link

sourcery-ai bot commented Jun 28, 2022

Sourcery Code Quality Report

❌  Merging this PR will decrease code quality in the affected files by 2.87%.

Quality metrics Before After Change
Complexity 1.30 ⭐ 3.02 ⭐ 1.72 👎
Method Length 38.37 ⭐ 41.98 ⭐ 3.61 👎
Working memory 6.26 🙂 6.61 🙂 0.35 👎
Quality 82.05% 79.18% -2.87% 👎
Other metrics Before After Change
Lines 3424 4199 775
Changed files Quality Before Quality After Quality Change
pyarr/base.py 91.00% ⭐ 86.91% ⭐ -4.09% 👎
pyarr/const.py 98.33% ⭐ 99.33% ⭐ 1.00% 👍
pyarr/exceptions.py 95.53% ⭐ 94.23% ⭐ -1.30% 👎
pyarr/lidarr.py 76.24% ⭐ 73.75% 🙂 -2.49% 👎
pyarr/radarr.py 88.68% ⭐ 84.69% ⭐ -3.99% 👎
pyarr/readarr.py 73.73% 🙂 74.16% 🙂 0.43% 👍
pyarr/request_handler.py 78.29% ⭐ 76.56% ⭐ -1.73% 👎
pyarr/sonarr.py 88.06% ⭐ 78.84% ⭐ -9.22% 👎
pyarr/models/common.py 98.83% ⭐ 80.72% ⭐ -18.11% 👎
pyarr/models/lidarr.py 81.80% ⭐ 91.06% ⭐ 9.26% 👍
pyarr/models/radarr.py 85.39% ⭐ 90.14% ⭐ 4.75% 👍
pyarr/models/readarr.py 79.68% ⭐ 90.60% ⭐ 10.92% 👍
pyarr/models/sonarr.py 83.28% ⭐ 93.42% ⭐ 10.14% 👍
sphinx-docs/conf.py 65.02% 🙂 64.95% 🙂 -0.07% 👎

Here are some functions in these files that still need a tune-up:

File Function Complexity Length Working Memory Quality Recommendation
pyarr/readarr.py ReadarrAPI.add_book 6 ⭐ 146 😞 14 😞 50.80% 🙂 Try splitting into smaller methods. Extract out complex expressions
pyarr/sonarr.py SonarrAPI.add_series 8 ⭐ 113 🙂 13 😞 54.22% 🙂 Extract out complex expressions
pyarr/lidarr.py LidarrAPI.add_album 6 ⭐ 127 😞 13 😞 54.39% 🙂 Try splitting into smaller methods. Extract out complex expressions
pyarr/lidarr.py LidarrAPI.add_artist 6 ⭐ 119 🙂 13 😞 55.37% 🙂 Extract out complex expressions
pyarr/readarr.py ReadarrAPI.add_author 6 ⭐ 114 🙂 13 😞 56.01% 🙂 Extract out complex expressions

Legend and Explanation

The emojis denote the absolute quality of the code:

  • ⭐ excellent
  • 🙂 good
  • 😞 poor
  • ⛔ very poor

The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request.


Please see our documentation here for details on how these metrics are calculated.

We are actively working on this report - lots more documentation and extra metrics to come!

Help us improve this quality report!

@marksie1988
Copy link
Collaborator Author

In order to avoid static fixtures and have more robust testing, it may make sense to setup an instance of Sonarr/Radarr/etc during testing.

This could be accomplished one of three ways

  1. Directly storing a linux -arr pre-built executables within the tests directory.
  2. Store pre-built executables within separate PyArr branches, then download them from that GitHub branch during testing, if they're not already downloaded. Would require you to commit the -arr DB in order to keep API keys consistent across test runs.
  3. Store only the -arr DBs within this repo then install the -arrs via nox.

Then, the -arrs would need to be spawned during tests via subprocess.run through PyTest or a Nox prior to running tests.

Ok there are two options here, i'm not sure which to go with as there are pros and cons to both:

  1. Create a Docker image that installs all of the required arrs and this could also be used as the devcontainer. then use nox to start / stop services when a test is being run.
    Pros:
    - Easier to use with devcontainer, only one container is required
    - In theory should use lower resources than option 2 when not running tests as the services could be stopped
    Cons:
    - A lot of things installed on one container
    - Would be quite a large dockerfile
  2. Use docker-compose to create each arr container and a dev/test container
    Pros:
    - Everything isn't installed on one container
    - separation of apps and code
    Cons:
    - multiple containers
    - potentially higher resource usage due to multiple containers running all the time.

For the DBs, I think creating a subfolder in tests would allow easy access for the above methods just having to copy it over to the correct directory.

@Archmonger
Copy link
Contributor

Archmonger commented Nov 11, 2022

In this situation, we should probably use option 2 to install the linuxserver or binhex dockers for all the -arrs. Would keep it simple enough so that the tests would run the :latest version of -arrs every time.

pyarr/models/common.py Outdated Show resolved Hide resolved
pyarr/models/common.py Outdated Show resolved Hide resolved
@marksie1988
Copy link
Collaborator Author

In this situation, we should probably use option 2 to install the linuxserver or binhex dockers for all the -arrs. Would keep it simple enough so that the tests would run the :latest version of -arrs every time.

I have used the linuxserver containers and its working well for parts of the API, however I do have issues then where for example we get_queue and get_history and get_episode_files also checking downloads etc. as this would mean adding a download client and downloading episodes which is not ideal.

So based on that, there would only be so much of Pyarr that could be tested against the live arr servers, the rest would still need to be mocked.

@Archmonger
Copy link
Contributor

Given that we're an API for download grabbers (the arrs), it's not unrealistic for us to also set up something like a Deluge instance using linuxserver. But we would need to be careful to only run tests using copyleft content.

@Archmonger
Copy link
Contributor

Worst case we can go back to static fixtures, but that risks being out of sync with -arr APIs.

There has been occasions where they have made breaking API changes with no warning.

@marksie1988
Copy link
Collaborator Author

Worst case we can go back to static fixtures, but that risks being out of sync with -arr APIs.

There has been occasions where they have made breaking API changes with no warning.

I think to start with maybe static fixtures until i can figure a way to do everything needed for using the full clients, or maybe do a mix of both, where it's too complex to use the Arr instance we use the static ones instead.

I know it's not ideal but it will allow moving forwards.

@Archmonger
Copy link
Contributor

Archmonger commented Mar 18, 2023

I say just use static fixtures for everything to move things forward.

Until we develop live testing, we'll just have to remember to manually update the static fixtures every so often.

@marksie1988 marksie1988 added this to the 5.0.0 milestone Mar 29, 2023
@marksie1988
Copy link
Collaborator Author

Ok @Archmonger I think this is ready to merge, All tests that can be are doing it against the live APIs, the rest are still mocks.

I haven't got 100% coverage yet, but its all around 90% or more, which at the moment I think is enough to get this merged, test the new github actions and resolve any issues related before pushing to PyPi

@marksie1988 marksie1988 merged commit e08836d into master Apr 5, 2023
@marksie1988 marksie1988 deleted the 91-add_tests_and_codecov branch June 2, 2024 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/ci CI related changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type issues with RadarrAPI.get_movie() Add tests
2 participants