Merge lp:~jamesodhunt/python-apt/test-for-size_to_str into lp:~mvo/python-apt/debian-sid-mirrored

[James Hunt]

  * python/progress.cc:
    - setattr(): Check return from Py_BuildValue().
    - PyFetchProgress:Pulse(): Check return from setattr().
  * tests/test_apt_pkg.py: New test, currently only for
    size_to_str() method.

[Michael Vogt (mvo)]

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:
- 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_for_string(), test_from_data(), test_raises_on_none()

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

review: Needs Information

[Michael Vogt (mvo)]

One more thing, returning "false" from PyFetchProgress::Pulse() will result in the fetching getting cancelled (see the comment some lines down in ::Pulse). I'm not sure this is desired behavior, but then I don't know yet when setattr() may fail so maybe it is :)

[Barry Warsaw (barry)]

A few additional comments. To fix bug 1030278 (sorry about crossing signals on that one!) I added a test specifically for that bug, which of course fails without the patch. I think I called it test_lp1030278.py modeled after another LP bug test.

I would suggest moving that into test_apt_pkg.py. Maybe you can combine that with grabbing my patch for the bug (only applied to the ubuntu package) and combining it with this merge proposal?

[Barry Warsaw (barry)]

Note that if you wanted to be strict about it (generally not a bad idea when dealing with the Python C API), setattr() in progress.cc should really be checking the return value of Py_BuildValue() too; if that returns NULL, then setattr() should return 0 before calling PyObject_SetAttrString().

As for checking the return value of setattr(), that's probably a good idea to do. The alternative is to be optimistic, don't check the return values, and then do a blanket check of PyErr_Occurred() to see if any of them failed. In some sense, it's not as safe, but practically speaking it's probably good enough, especially since if any of them fail, you're probably in trouble. It's probably not a good idea to just let exceptions pass unchecked - that was one of the fundamental problems with LP: #1030278.

[Michael Vogt (mvo)]

On Tue, Oct 02, 2012 at 02:20:24PM -0000, Barry Warsaw wrote:
> A few additional comments. To fix bug 1030278 (sorry about crossing signals on that one!) I added a test specifically for that bug, which of course fails without the patch. I think I called it test_lp1030278.py modeled after another LP bug test.
>
> I would suggest moving that into test_apt_pkg.py. Maybe you can combine that with grabbing my patch for the bug (only applied to the ubuntu package) and combining it with this merge proposal?

Thanks for this suggestion, I had a similar thought :)

I merge them into the debian-sid branch now and put them there as
"test_size_to_str.py" for now.

Thanks!
 Michael

Unmerged revisions

620. By James Hunt on 2012-10-02

tests/test_apt_pkg.py: New test, currently only for
size_to_str() method.

619. By James Hunt on 2012-10-02

* python/progress.cc:
  - setattr(): Check return from Py_BuildValue().
  - PyFetchProgress:Pulse(): Check return from setattr().

618. By James Hunt on 2012-10-01

python/cache.cc: PkgCacheGetIsMultiArch(): Return calculated
value rather than a random one.

=== modified file 'debian/changelog'
--- debian/changelog	2012-06-22 08:37:31 +0000
+++ debian/changelog	2012-10-02 08:29:26 +0000
@@ -1,3 +1,15 @@
+python-apt (0.8.5ubuntu3) UNRELEASED; urgency=low
+
+  * python/cache.cc: PkgCacheGetIsMultiArch(): Return calculated
+    value rather than a random one.
+  * python/progress.cc:
+    - setattr(): Check return from Py_BuildValue().
+    - PyFetchProgress:Pulse(): Check return from setattr().
+  * tests/test_apt_pkg.py: New test, currently only for
+    size_to_str() method. 
+
+ -- James Hunt <[e-mail]>  Tue, 02 Oct 2012 09:25:45 +0100
+
 python-apt (0.8.5) unstable; urgency=low
 
   [ Michael Vogt ]

=== modified file 'python/cache.cc'
--- python/cache.cc	2012-04-17 19:17:10 +0000
+++ python/cache.cc	2012-10-02 08:29:26 +0000
@@ -286,7 +286,7 @@
 
 static PyObject *PkgCacheGetIsMultiArch(PyObject *Self, void*) {
     pkgCache *Cache = GetCpp<pkgCache *>(Self);
-    PyBool_FromLong(Cache->MultiArchCache());
+    return PyBool_FromLong(Cache->MultiArchCache());
 } 
 
 static PyGetSetDef PkgCacheGetSet[] = {

=== modified file 'python/progress.cc'
--- python/progress.cc	2011-11-10 16:20:58 +0000
+++ python/progress.cc	2012-10-02 08:29:26 +0000
@@ -28,6 +28,8 @@
     if (!object)
         return false;
     PyObject *value = Py_BuildValue(fmt, arg);
+    if (! value)
+        return false;
 
     int result = PyObject_SetAttrString(object, attr, value);
     Py_DECREF(value);
@@ -280,14 +282,22 @@
       return false;
    }
 
-   setattr(callbackInst, "last_bytes", "N", MkPyNumber(LastBytes));
-   setattr(callbackInst, "current_cps", "N", MkPyNumber(CurrentCPS));
-   setattr(callbackInst, "current_bytes", "N", MkPyNumber(CurrentBytes));
-   setattr(callbackInst, "total_bytes", "N", MkPyNumber(TotalBytes));
-   setattr(callbackInst, "fetched_bytes", "N", MkPyNumber(FetchedBytes));
-   setattr(callbackInst, "elapsed_time", "N", MkPyNumber(ElapsedTime));
-   setattr(callbackInst, "current_items", "N", MkPyNumber(CurrentItems));
-   setattr(callbackInst, "total_items", "N", MkPyNumber(TotalItems));
+   if (! setattr(callbackInst, "last_bytes", "N", MkPyNumber(LastBytes)))
+       return false;
+   if (! setattr(callbackInst, "current_cps", "N", MkPyNumber(CurrentCPS)))
+       return false;
+   if (! setattr(callbackInst, "current_bytes", "N", MkPyNumber(CurrentBytes)))
+       return false;
+   if (! setattr(callbackInst, "total_bytes", "N", MkPyNumber(TotalBytes)))
+       return false;
+   if (! setattr(callbackInst, "fetched_bytes", "N", MkPyNumber(FetchedBytes)))
+       return false;
+   if (! setattr(callbackInst, "elapsed_time", "N", MkPyNumber(ElapsedTime)))
+       return false;
+   if (! setattr(callbackInst, "current_items", "N", MkPyNumber(CurrentItems)))
+       return false;
+   if (! setattr(callbackInst, "total_items", "N", MkPyNumber(TotalItems)))
+       return false;
 
    // New style
    if (!PyObject_HasAttrString(callbackInst, "updateStatus")) {
@@ -313,12 +323,19 @@
      return true;
    }
 #ifdef COMPAT_0_7
-   setattr(callbackInst, "currentCPS", "N", MkPyNumber(CurrentCPS));
-   setattr(callbackInst, "currentBytes", "N", MkPyNumber(CurrentBytes));
-   setattr(callbackInst, "totalBytes", "N", MkPyNumber(TotalBytes));
-   setattr(callbackInst, "fetchedBytes", "N", MkPyNumber(FetchedBytes));
-   setattr(callbackInst, "currentItems", "N", MkPyNumber(CurrentItems));
-   setattr(callbackInst, "totalItems", "N", MkPyNumber(TotalItems));
+   if (! setattr(callbackInst, "currentCPS", "N", MkPyNumber(CurrentCPS)))
+       return false;
+   if (! setattr(callbackInst, "currentBytes", "N", MkPyNumber(CurrentBytes)))
+       return false;
+   if (! setattr(callbackInst, "totalBytes", "N", MkPyNumber(TotalBytes)))
+       return false;
+   if (! setattr(callbackInst, "fetchedBytes", "N", MkPyNumber(FetchedBytes)))
+       return false;
+   if (! setattr(callbackInst, "currentItems", "N", MkPyNumber(CurrentItems)))
+       return false;
+   if (! setattr(callbackInst, "totalItems", "N", MkPyNumber(TotalItems)))
+       return false;
+
    // Go through the list of items and add active items to the
    // activeItems vector.
    map<pkgAcquire::Worker *, pkgAcquire::ItemDesc *> activeItemMap;

=== added file 'tests/test_apt_pkg.py'
--- tests/test_apt_pkg.py	1970-01-01 00:00:00 +0000
+++ tests/test_apt_pkg.py	2012-10-02 08:29:26 +0000
@@ -0,0 +1,111 @@
+#!/usr/bin/env python
+
+import sys
+import unittest
+
+import apt_pkg
+
+data = {
+        # XXX: note the trailing spaces for some of these entries!
+    10 ** 1                  : "10 ",
+    10 ** 2                  : "100 ",
+    10 ** 3                  : "1000 ",
+    10 ** 4                  : "10.0 k",
+    10 ** 5                  : "100 k",
+    10 ** 6                  : "1000 k",
+    10 ** 7                  : "10.0 M",
+    10 ** 8                  : "100 M",
+    10 ** 9                  : "1000 M",
+    10 ** 10                 : "10.0 G",
+    10 ** 11                 : "100 G",
+    10 ** 12                 : "1000 G",
+    10 ** 13                 : "10.0 T",
+    10 ** 14                 : "100 T",
+    10 ** 15                 : "1000 T",
+    10 ** 16                 : "10.0 P",
+    10 ** 17                 : "100 P",
+    10 ** 18                 : "1000 P",
+    10 ** 19                 : "10.0 E",
+    10 ** 20                 : "100 E",
+    10 ** 21                 : "1000 E",
+    10 ** 22                 : "10.0 Z",
+    10 ** 23                 : "100.0 Z",
+    10 ** 24                 : "1000 Z",
+    10 ** 25                 : "10.0 Y",
+    10 ** 26                 : "100 Y",
+    10 ** 27                 : "1000 Y",
+
+    # That's our limit :)
+    10 ** 28                 : "10000 Y",
+
+    0                        : "0 ",
+    1                        : "1 ",
+    1024                     : "1024 ",
+    10240                    : "10.2 k",
+    102400                   : "102 k",
+    1024000                  : "1024 k",
+    10240000                 : "10.2 M",
+    102400000                : "102 M",
+    2147483647               : "2147 M",
+    2147483648               : "2147 M",
+    1024000000               : "1024 M",
+    10240000000              : "10.2 G",
+
+    9                        : "9 ",
+    99                       : "99 ",
+    999                      : "999 ",
+    9999                     : "9999 ",
+    99999                    : "100.0 k",
+    999999                   : "1000 k",
+    9999999                  : "10000 k",
+    99999999                 : "100.0 M",
+    999999999                : "1000 M",
+    9999999999               : "10000 M",
+    99999999999              : "100.0 G",
+    999999999999             : "1000 G",
+    9999999999999            : "10000 G",
+    99999999999999           : "100.0 T",
+    999999999999999          : "1000 T",
+    9999999999999999         : "10.0 P",
+    99999999999999999        : "100 P",
+    999999999999999999       : "1000 P",
+    9999999999999999999      : "10.0 E",
+    99999999999999999999     : "100 E",
+    999999999999999999999    : "1000 E",
+    9999999999999999999999   : "10.0 Z",
+    999999999999999999999999 : "1000 Z",
+}
+
+class TestAptPkg(unittest.TestCase):
+    """Test apt_pkg."""
+
+    def setUp(self):
+        pass
+
+    def test_size_to_str(self):
+        try:
+            for k, v in data.items():
+                size = apt_pkg.size_to_str(k)
+                msg = "size_to_str(%s) returned '%s', expected '%s'" % (k, size, v)
+                self.assertEqual(size, v, msg)
+        except:
+            self.fail(sys.exc_info())
+
+        with self.assertRaises(TypeError):
+            apt_pkg.size_to_str("hello")
+
+        with self.assertRaises(TypeError):
+            apt_pkg.size_to_str(None)
+
+        with self.assertRaises(TypeError):
+            apt_pkg.size_to_str({})
+
+        with self.assertRaises(TypeError):
+            apt_pkg.size_to_str([])
+
+        with self.assertRaises(TypeError):
+            apt_pkg.size_to_str(())
+
+
+if __name__ == "__main__":
+    unittest.main()