Merge lp:~jamesodhunt/python-apt/test-for-size_to_str into lp:~mvo/python-apt/debian-sid-mirrored
Proposed by
James Hunt
Status: | Needs review |
---|---|
Proposed branch: | lp:~jamesodhunt/python-apt/test-for-size_to_str |
Merge into: | lp:~mvo/python-apt/debian-sid-mirrored |
Diff against target: |
218 lines (+155/-15) 4 files modified
debian/changelog (+12/-0) python/cache.cc (+1/-1) python/progress.cc (+31/-14) tests/test_apt_pkg.py (+111/-0) |
To merge this branch: | bzr merge lp:~jamesodhunt/python-apt/test-for-size_to_str |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Michael Vogt | Needs Information | ||
Review via email: mp+127435@code.launchpad.net |
Description of the change
* python/progress.cc:
- setattr(): Check return from Py_BuildValue().
- PyFetchProgress
* tests/test_
size_to_str() method.
To post a comment you must log in.
Unmerged revisions
- 620. By James Hunt
-
tests/test_
apt_pkg. py: New test, currently only for
size_to_str() method. - 619. By James Hunt
-
* python/progress.cc:
- setattr(): Check return from Py_BuildValue().
- PyFetchProgress:Pulse( ): Check return from setattr(). - 618. By James Hunt
-
python/cache.cc: PkgCacheGetIsMu
ltiArch( ): Return calculated
value rather than a random one.
Hi, thanks for your branch. Its great to see test improvements/code fixes!
I have some questions:
- setattr() is used a bunch of times with no checking of the return value - should it be checked there as well?
- under what circumstances can setattr() fail, i.e. is this branch a result of a specific bug?
Its great that the test is added, it seems to be unreleated to the changes for PyFetchProgress ::Pulse( ) and its also currently working even without the pulse changes. Is it addressing a specific bug? Or just there to have better test coverage (which is welcome of course :)
As for the test itself, a bit of nitpicking: for_string( ), test_from_data(), test_raises_ on_none( )
- the name "data" in test_apt_pkg.py could probably be something more meaningful for the time when more tests get added.
- I would slightly prefer to have a SizeToStrTestCase() and multiple methods like test_raises_
Sorry for the relatively long review, I went through the code-review school of launchpad recently ;) Happy to talk to you about any of the points I mentioned or help with resolving them.
Thanks,
Michael