classification
Title: Patch to increase aifc lib test coverage
Type: behavior Stage: resolved
Components: Library (Lib), Tests Versions: Python 3.2, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ezio.melotti Nosy List: Oleg.Plakhotnyuk, ezio.melotti, haypo, python-dev, r.david.murray
Priority: normal Keywords: patch

Created on 2011-11-13 07:34 by Oleg.Plakhotnyuk, last changed 2012-03-12 22:41 by ezio.melotti. This issue is now closed.

Files
File name Uploaded Description Edit
test_aifc.patch Oleg.Plakhotnyuk, 2011-12-13 19:38 review
test_aifc_3_2.patch Oleg.Plakhotnyuk, 2012-03-01 05:02
Messages (16)
msg147550 - (view) Author: Oleg Plakhotnyuk (Oleg.Plakhotnyuk) * Date: 2011-11-13 07:34
I've increased coverage of aifc.py by test_aifc.py:

before:
[1/1] test_aifc
lines   cov%   module   (path)
  560    63%   aifc   (/Users/family/Documents/code/python/repo/Lib/aifc.py)
...

after:
[1/1] test_aifc
lines   cov%   module   (path)
  561    82%   aifc   (/Users/family/Documents/code/python/repo/Lib/aifc.py)
...

I have also encountered couple of problems in aifc.py:
1. When using setmark() with string name to write to aiff file, you get Exception TypeError: "'str' does not support the buffer interface".
2. When using close() of Aifc_write object, it attempts to close it again in __del__() which ends with ValueError: write to closed file.

I propose following solutions to above problems in patch attached:
1. Use bytes type for mark names, because entire aifc library already uses bytes anyway.
2. Make file pointer check in close() and reset it after actually closing the file.

I don't think that these fixes will cause major backward compatibility problems, because with current version of aifc there is no way to write markers at all anyway.

I plan to increase test coverage of aifc.py further in subsequent patches. Just want to make it in several small steps :-)

Please provide any feedback regarding to what should I do to make this patch committed.

Thanks!
msg147551 - (view) Author: Oleg Plakhotnyuk (Oleg.Plakhotnyuk) * Date: 2011-11-13 08:14
Thanks for your review, Ezio!
Here goes new patch with all issues you've mentioned been fixed.
msg147982 - (view) Author: Oleg Plakhotnyuk (Oleg.Plakhotnyuk) * Date: 2011-11-20 10:35
1. Test coverage increased to 95%.
2. G722 compressed files reading in aifc.py fixed (it used to use 0 bytes frame size).
3. audioop's ulaw2lin, alaw2lin and adpcm2lin length checks fixed (width should be used for output only, because input sequence frame length is always 1 byte).
4. aifc.py _write_float infinity and NaN proper checking.
5. Other minor aifc.py clean ups.
msg147990 - (view) Author: Oleg Plakhotnyuk (Oleg.Plakhotnyuk) * Date: 2011-11-20 14:21
Minor style fixes
msg148978 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-12-07 16:56
I think the patch includes too many changes.  It would be better if you can split it in two separate patches: one with the increased coverage, the other with the bugs you found and their tests.
I can easily commit the first (assuming it doesn't break the test suite), but the second requires someone familiar with the aifc module for a review.
If you have found (and fixed) several unrelated bugs in aifc, it might be better to split the second patch further, and possibly open different issues, stating what the bugs are and how you fixed them.
msg149340 - (view) Author: Oleg Plakhotnyuk (Oleg.Plakhotnyuk) * Date: 2011-12-12 17:52
Sounds perfectly reasonable. Here goes the first patch with pure test coverage.
msg149346 - (view) Author: Oleg Plakhotnyuk (Oleg.Plakhotnyuk) * Date: 2011-12-12 18:35
Second patch goes to issue 13589
msg149390 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2011-12-13 14:11
Thanks for splitting the patch.
I tried to apply the patch to 3.2 and I have 3 comments:
1) you changed a commented-out assertEqual with an assertNotEqual, because "ULAW is lossy compression, so frames *may* not match".  Does it mean that sometimes they might match and make the test fail or that they are always different for this specific test?  In the first case the test should make sure to have a consistent result and avoid sporadic failures.  In the second case the check could be removed altogether, because producing different results is not a feature of the format, but just a detail that depends on the input.
2) when I run the tests I now get 3 warnings:
Warning: bad COMM chunk size
Warning: bad COMM chunk size
Warning: MARK chunk contains only 0 markers instead of 1
These should be silenced, or better, tested with self.assertWarns (assertWarns should silence them too).
3) there's some trailing space just before test_write_header_comptype_raises.
msg149404 - (view) Author: Oleg Plakhotnyuk (Oleg.Plakhotnyuk) * Date: 2011-12-13 19:38
The split is in progress. There will be at least two more patches.
Regarding your comments:
1) Sorry for my english :-) It is fully determined by the input. With this particular test input the assertNotEqual will always pass. So I've removed it.
2) These were simple prints, not warnings. I've replaced them with warnings and tested with assertWarns. It resulted in some changes to aifc library itself, but they are trivial and cannot affect behavior.
3) Fixed.
Thanks.
msg149405 - (view) Author: Oleg Plakhotnyuk (Oleg.Plakhotnyuk) * Date: 2011-12-13 19:38
Third patch goes to issue 13594
msg150366 - (view) Author: Oleg Plakhotnyuk (Oleg.Plakhotnyuk) * Date: 2011-12-30 09:24
Fourth patch goes to issue 13680
msg150375 - (view) Author: Oleg Plakhotnyuk (Oleg.Plakhotnyuk) * Date: 2011-12-30 11:24
The last, fifth, patch goes to issue 13681
msg154624 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2012-02-29 12:13
Converting prints to warnings on 3.2 might not be a good idea.  I can probably still apply the rest of the patch there, and change the warnings on 3.3 only.
msg154671 - (view) Author: Oleg Plakhotnyuk (Oleg.Plakhotnyuk) * Date: 2012-03-01 04:59
For convenience I have uploaded the separate patch without warnings. Which means that it will generate some output during tests run.
msg155489 - (view) Author: Roundup Robot (python-dev) Date: 2012-03-12 21:57
New changeset fc3a63ed1f67 by Ezio Melotti in branch '3.2':
#13394: add more tests for the aifc module.  Patch by Oleg Plakhotnyuk.
http://hg.python.org/cpython/rev/fc3a63ed1f67

New changeset 512d3ad81fb9 by Ezio Melotti in branch 'default':
#13394: add more tests for the aifc module and use warnings.warn instead of print.  Patch by Oleg Plakhotnyuk.
http://hg.python.org/cpython/rev/512d3ad81fb9
msg155496 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2012-03-12 22:10
Fixed, thanks for the patches!
History
Date User Action Args
2012-03-12 22:41:52ezio.melottisetassignee: ezio.melotti
2012-03-12 22:10:26ezio.melottisetstatus: open -> closed
resolution: fixed
messages: + msg155496

stage: patch review -> resolved
2012-03-12 21:57:37python-devsetnosy: + python-dev
messages: + msg155489
2012-03-01 05:02:58Oleg.Plakhotnyuksetfiles: + test_aifc_3_2.patch
2012-03-01 05:02:24Oleg.Plakhotnyuksetfiles: - test_aifc_3_2.patch
2012-03-01 04:59:07Oleg.Plakhotnyuksetfiles: + test_aifc_3_2.patch

messages: + msg154671
2012-02-29 12:13:43ezio.melottisetmessages: + msg154624
2011-12-30 11:24:54Oleg.Plakhotnyuksetmessages: + msg150375
2011-12-30 09:24:23Oleg.Plakhotnyuksetmessages: + msg150366
2011-12-13 19:38:48Oleg.Plakhotnyuksetmessages: + msg149405
2011-12-13 19:38:16Oleg.Plakhotnyuksetfiles: + test_aifc.patch

messages: + msg149404
2011-12-13 19:29:41Oleg.Plakhotnyuksetfiles: - test_aifc.patch
2011-12-13 14:11:10ezio.melottisetmessages: + msg149390
2011-12-13 05:22:07Oleg.Plakhotnyuksettitle: Patch to increase aifc lib test coverage with couple of minor fixes -> Patch to increase aifc lib test coverage
2011-12-12 18:50:51Oleg.Plakhotnyuksetfiles: + test_aifc.patch
2011-12-12 18:50:20Oleg.Plakhotnyuksetfiles: - test_aifc.patch
2011-12-12 18:35:45Oleg.Plakhotnyuksetmessages: + msg149346
2011-12-12 17:52:53Oleg.Plakhotnyuksetfiles: + test_aifc.patch

messages: + msg149340
2011-12-12 17:49:55Oleg.Plakhotnyuksetfiles: - test_aifc.patch
2011-12-07 16:56:00ezio.melottisetnosy: + ezio.melotti
messages: + msg148978
2011-11-20 14:24:39Oleg.Plakhotnyuksetfiles: - test_aifc.patch
2011-11-20 14:21:12Oleg.Plakhotnyuksetfiles: + test_aifc.patch

messages: + msg147990
2011-11-20 10:37:29Oleg.Plakhotnyuksetfiles: - test_aifc.patch
2011-11-20 10:35:40Oleg.Plakhotnyuksetfiles: + test_aifc.patch
nosy: + haypo
messages: + msg147982

2011-11-20 10:11:19Oleg.Plakhotnyuksetfiles: - test_aifc2.patch
2011-11-13 08:14:12Oleg.Plakhotnyuksetfiles: + test_aifc2.patch

messages: + msg147551
2011-11-13 07:46:46ezio.melottisetnosy: + r.david.murray
stage: patch review

versions: + Python 3.2, Python 3.3, - Python 3.4
2011-11-13 07:34:18Oleg.Plakhotnyukcreate