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

Add a simple testsuite #79

Merged
merged 5 commits into from
Jan 3, 2019
Merged

Add a simple testsuite #79

merged 5 commits into from
Jan 3, 2019

Conversation

ssssam
Copy link
Contributor

@ssssam ssssam commented Dec 26, 2018

I wrote a simple testsuite using py.test while investigating #78.

Currently it only tests the audioread.audio_open() method. I'd like to add a separate set of tests for each backend, as I think there are some bugs in both the GStreamer and libmad backends that are causing everything to end up being run through ffmpeg. I don't know if I'll get time for that though.

@ssssam
Copy link
Contributor Author

ssssam commented Dec 26, 2018

Now I spot that #73 also contains a test suite, which looks more comprehensive than this one.

@sampsyo
Copy link
Member

sampsyo commented Dec 26, 2018

Awesome! These tests look great already—but you're right; it looks like we let that other PR languish. I don't see any outstanding issues with that PR, so maybe we should merge that one and try to unify the two test suites.

@ssssam
Copy link
Contributor Author

ssssam commented Dec 28, 2018

I've looked at PR #73 and found several issues: the seeking support is incomplete, the tests don't pass for me, and the commit history is a bit messed up.

I could make a new PR with the test suite from #73 minus the 'seeking' feature that's not finished. The main difference from this branch is that it uses the Python 'unittest' module rather than Pytest. Also, it has a tool to generate a wav file and assert in the tests that the data is read correctly from the file. It will take some time to get this working though as the expected results don't seem to match the generated file. I'm not actually sure if this kind of test will work on different platforms as I don't know if different versions of an MP3 decoder are guaranteed to return the same bit stream as each other ...

I'm not interested in the 'seeking' feature, but I am willing to do a bit more work to ensure the project has a test suite. I could do a new branch using the 'unittest' tests from #73 as a base, but without the parts that don't work ... or we could merge this branch now and ask @pconerly to rebase his branch on top of master if/when he gets a chance to revisit. What's your preference @sampsyo ?

@sampsyo
Copy link
Member

sampsyo commented Dec 29, 2018

Got it. Thanks for investigating. Let's forge ahead with merging a test suite and worry about seeking separately.

I think starting with files on disk (instead of generating them) seems simplest, especially if we can keep the files small! Let's do that.

As for unittest vs. py.test: I typically opt for the built-in stdlib module, which makes it easy to run the tests without any dependencies using any old test runner (e.g., nose), but I don't have any objection to py.test if that's what you're more familiar with. One thing that would be great to steal form #79 is the tox configuration, which makes it easy to run the tests without worrying about dependencies (and across different Python versions).

The testsuite can be run simply by executing 'pytest' or 'pytest-3'
from the top directory of the project.
This makes it possible to integrate Py.test with setup.py.
This allows running the test suite with Python 2.6 and Python 3.6,
with dependencies automatically installed, just by running `tox`.
@ssssam
Copy link
Contributor Author

ssssam commented Jan 2, 2019

I happen to prefer py.test, I find the test code a lot easier to read. If you're happy with that then let's go with this branch for a test suite. I've added a Tox configuration; I have left the Travis CI config running pytest directly because it seems to already manage multiple versions of Python.

@sampsyo
Copy link
Member

sampsyo commented Jan 3, 2019

Looks great; thanks!! I'll merge this now.

@sampsyo sampsyo merged commit 92f73b3 into beetbox:master Jan 3, 2019
@ssssam ssssam deleted the sam/testsuite branch January 3, 2019 17:49
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