-
Notifications
You must be signed in to change notification settings - Fork 162
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
base: main
Are you sure you want to change the base?
Conversation
matplotlib===3.9.3 | ||
# below, for running tests when developing birdnet-analyzer itself | ||
pytest==8.3.4 |
There was a problem hiding this comment.
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` |
There was a problem hiding this comment.
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' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Finally had time to look at this. |
Thanks for taking time to do the review & respond, @Josef-Haupt ! Just some comments:
Great point. I noticed only that the repository as it is right now only has
How do you set up your environment in windows? Is there an intention to add a
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
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 |
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 You are correct about the environment python commands, I did misread the To answer your question about |
👋 Hello! For your consideration…
This PR adds
birdnet_analyzer.analyze
module testsWhen trying to learn more about how to execute parts of this project, I noticed this comment at the end of
analyze.py
This PR translates these examples directly into tests (under folder
tests/
). The purpose of doing so is:example/
+ command-line argument inputs. Sample outputs have been added undertests/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.analyze
.To run these tests:
Assuming
requirements.txt
are already installed: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 torequirements.txt
Respectfully,
Ken