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

Tests for birdnet_analyzer.analyze module #527

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Conversation

kenshih
Copy link
Contributor

@kenshih kenshih commented Dec 15, 2024

👋 Hello! For your consideration…

This PR adds birdnet_analyzer.analyze module tests

When trying to learn more about how to execute parts of this project, I noticed this comment at the end of analyze.py

A few examples to test
python3 analyze.py --i example/ --o example/ --slist example/ --min_conf 0.5 --threads 4
python3 analyze.py --i example/soundscape.wav --o example/soundscape.BirdNET.selection.table.txt --slist example/species_list.txt --threads 8
python3 analyze.py --i example/ --o example/ --lat 42.5 --lon -76.45 --week 4 --sensitivity 1.0 --rtype table --locale de

This PR translates these examples directly into tests (under folder tests/). The purpose of doing so is:

  1. To have specific outputs matching specific example/ + command-line argument inputs. Sample outputs have been added under tests/resources/. This way the tester knows exactly what is expected, including something to validate their setup. I found these tests useful, as well, in learning the specifics of the interface.
  2. When making changes to the codebase, these tests can be used as a sort of sanity test to check that no regression/unexpected change was introduced in the output.
  3. If accepted, to provide a pattern to add other tests of this sort, since I noticed other modules had similar comments to analyze.

To run these tests:

Assuming requirements.txt are already installed:

$ pytest tests/test_analyze__main.py
======================= test session starts =======================
platform darwin -- Python 3.11.11, pytest-8.3.4, pluggy-1.5.0
rootdir: /Users/ken/Documents/wk/BirdNET-Analyzer
plugins: anyio-4.7.0
collected 3 items                                                 

tests/test_analyze__main.py ...                             [100%]

======================= 3 passed in 19.90s ========================

Some things to check when reviewing

  • tests/test_analyze__main.py is the main addition, with the rest of changes simply to support this addition.
  • pytest , a common python testing lib, is a dependency that has been added to requirements.txt

Respectfully,
Ken

matplotlib===3.9.3
# below, for running tests when developing birdnet-analyzer itself
pytest==8.3.4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally added this as requirements-dev.txt, but then thought to keep-it-simple by leaving it here

#
# How to run these tests;
# 1. (Prerequisite) from root of this repository run: `pip install -r requirements.txt`
# 2. From root of this repository run: `pytest tests/test_analyze__main.py`
Copy link
Contributor Author

@kenshih kenshih Dec 15, 2024

Choose a reason for hiding this comment

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

I originally started adding a DEVELOPMENT.adoc readme, not quite knowing where to add useful how-to/context, but then realized that was too noisy a change.

If you, the maintainers of this project, like having these and/or other kinds of tests, I’d be happy to help to add more of them and provide usable documentation in other PRs, as preferred. Happy also to add a simple github action PR to run such tests automatically, e.g. when someone opens a PR these could do a baseline check for regression in advance of your review.

# GIVEN: analyze is called on example/ dir with 1 sound file, soundscape.wav, with a minimum confidence level of 0.5
output_dir = tmp_path
# comment the following line in in order to write a file to troubleshoot or take a new snapshot
# output_dir = 'birdnet_analyzer/example'
Copy link
Contributor Author

@kenshih kenshih Dec 15, 2024

Choose a reason for hiding this comment

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

This comment is added, because this code intentionally uses pytest’s tmp_path mechanism, which cleans up after itself on each test so as not to clutter the developer’s environment or leave behind anything she might mistake for real (non-test) output.

As well, an appropriate time to change the SNAPSHOTs themselves would be when a new model is introduced, so specific values like confidence might be expected to change. Generating a new SNAPSHOT is as simple as uncommenting this line, reviewing it, then copying it over the old snapshot. The diff of such an update can sometimes be enlightening.

Copy link
Contributor Author

@kenshih kenshih Dec 15, 2024

Choose a reason for hiding this comment

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

I added postfix __main to signal to the reader that this test is not a “unit test” so much as an “end-to-end” test.

Reason: If unit tests are eventually added, it will help with the organization, since e2e tests usually take more time to run (seconds instead of milliseconds) & aren’t very precise about “what is broken” compared to unit tests, so it’s good to know which is which. I find there is inevitably a desire to separate them.

Happy to suggest a rename to test_analyze.py, if that is preferred.

@kenshih kenshih marked this pull request as ready for review December 18, 2024 05:26
@Josef-Haupt
Copy link
Collaborator

Finally had time to look at this.
First of all thank you for your effort!
But there are a few issues.
The way the test works does not allow for it to be run from a virtual environment without adjusting the script for each possible platform, all the libraries need to be installed globally for it to work.
Also python3 is not a default alias on my win11 python installation.
Lastly, there is already a library based on birdnet that is thoroughly tested and will in the future be used by this repository. That's why there is no effort from our side to add more tests to this repo right now.

@kenshih
Copy link
Contributor Author

kenshih commented Dec 21, 2024

Thanks for taking time to do the review & respond, @Josef-Haupt !

Just some comments:

But there are a few issues.
The way the test works does not allow for it to be run from a virtual environment without adjusting the script for each possible platform, all the libraries need to be installed globally for it to work.

Great point. I noticed only that the repository as it is right now only has requirements.txt, so instead of adding a pyproject.toml (since I didn't know intention of the maintainers) I just made the assumption that folks working on the project did this before running python commands...

python -m venv myenv # to initialize a virtual environment
source myenv/bin/activate # enter venv instead of global
pip install -r requirements.txt

How do you set up your environment in windows? Is there an intention to add a pyproject.toml, poetry build, or docker build/dev environment, or would you like someone to?

Also python3 is not a default alias on my win11 python installation.

Also a good point. This isn't actually the alias I use on my macOs, either. I copied that command from the comment I found in analyze.py, so I thought that meant something the developers in this project assumed. But, yes, that makes sense to me. By the way, simply changing python3 to python should fix the os compatibility issue, especially since windows supports the / path as well as \. Did you find this not to be true or encounter other issues?

Lastly, there is already a library based on birdnet that is thoroughly tested and will in the future be used by this repository. That's why there is no effort from our side to add more tests to this repo right now.

Aha! Good to know. That makes tons of sense to have a library for that. Though, the tests I wrote actually are an integration test, so doesn't do double-duty with testing lib functionality. I still might have some value to validate that end-to-end things are stitched together as expected.

Let me know what you think. No worries, if the time to add tests is after the integration with the lib you mention if it should just be done in a different way. I can close this PR, if so.

Just curious, what is the relationship of this project to birdnet-team/birdnet and the birdnet-team organization? For that matter, what is the birdnet-team organization? :)

@kenshih kenshih marked this pull request as draft December 27, 2024 06:27
@Josef-Haupt
Copy link
Collaborator

Sorry for the late reply @kenshih !

We do have big plans for this year and there will be some structural changes concerning the repo, because all of this is not yet finalized in any way, we are kind of holding off an more spezialized additions (such as pyproject.toml) for the repo in it's current state.

You are correct about the environment python commands, I did misread the subprocess.run call.
And you're right again, our readme uses python3, that should probably be changed, but again we will rework the whole documentation this year.

To answer your question about birdnet-team, it is the organization for all future birdnet related repos, coming from us (the birdnet-team. We would love to move BirdNET-Analyzer under the birdnet-team umbrella, but honestly it could break some stuff, not just for us, but also for people using the repo.

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.

2 participants