Patch to increase aifc lib test coverage

[OlegPlakhotnyuk]

BPO	13394
Nosy	@vstinner, @ezio-melotti, @bitdancer
Files	test_aifc.patch test_aifc_3_2.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/ezio-melotti'
closed_at = <Date 2012-03-12.22:10:26.394>
created_at = <Date 2011-11-13.07:34:18.548>
labels = ['tests', 'type-bug', 'library']
title = 'Patch to increase aifc lib test coverage'
updated_at = <Date 2012-03-12.22:41:52.156>
user = 'https://bugs.python.org/OlegPlakhotnyuk'

bugs.python.org fields:

activity = <Date 2012-03-12.22:41:52.156>
actor = 'ezio.melotti'
assignee = 'ezio.melotti'
closed = True
closed_date = <Date 2012-03-12.22:10:26.394>
closer = 'ezio.melotti'
components = ['Library (Lib)', 'Tests']
creation = <Date 2011-11-13.07:34:18.548>
creator = 'Oleg.Plakhotnyuk'
dependencies = []
files = ['23945', '24692']
hgrepos = []
issue_num = 13394
keywords = ['patch']
message_count = 16.0
messages = ['147550', '147551', '147982', '147990', '148978', '149340', '149346', '149390', '149404', '149405', '150366', '150375', '154624', '154671', '155489', '155496']
nosy_count = 5.0
nosy_names = ['vstinner', 'ezio.melotti', 'r.david.murray', 'python-dev', 'Oleg.Plakhotnyuk']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue13394'
versions = ['Python 3.2', 'Python 3.3']

[OlegPlakhotnyuk]

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!

[OlegPlakhotnyuk]

Thanks for your review, Ezio!
Here goes new patch with all issues you've mentioned been fixed.

[OlegPlakhotnyuk]

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.

[OlegPlakhotnyuk]

Minor style fixes

[ezio-melotti]

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.

[OlegPlakhotnyuk]

Sounds perfectly reasonable. Here goes the first patch with pure test coverage.

[OlegPlakhotnyuk]

Second patch goes to bpo-13589

[ezio-melotti]

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.

[OlegPlakhotnyuk]

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.

[OlegPlakhotnyuk]

Third patch goes to bpo-13594

[OlegPlakhotnyuk]

Fourth patch goes to bpo-13680