Merge lp:~wesley-wiedenmeier/curtin/fix-unittests into lp:~curtin-dev/curtin/trunk

[Wesley Wiedenmeier]

Fix the unittests for test_apt_source.
Right now the tests have calls to '.assert_called()' which isn't a valid function in a mock object (although it should be)
Interestingly, this only causes the unittests to fail when running 'make check', but not when running tox.
I think that tox may be skipping these tests completely.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:416
https://jenkins.ubuntu.com/server/job/curtin-ci/227/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=amd64/227
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=ppc64el/227

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/227/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

Agreed. I mentioned this to smoser but probably wasn't clear.

review: Approve

[Christian Ehrhardt  (paelzer)]

Hi,
I came to about the same conclusion regarding the build error at

https://launchpadlibrarian.net/277356117/buildlog_ubuntu-xenial-amd64.curtin_0.1.0~bzr415-0ubuntu1~ubuntu16.04.1_BUILDING.txt.gz

What I found interesting is, that it doesn't always show up.
On my normal Laptop a make check just runs fine, but I can still reproduce with

./tools/build-deb -S
DEB_BUILD_OPTIONS=parallel=4 sbuild -Adxenial-amd64 curtin_0.1.0~bzr415-0ubuntu1.dsc

While I wonder why it works in a local make check I still agree to the solution as assert_called isn't documented for the mockobject.

review: Approve

[Scott Moser (smoser)]

Mock is a weird beast. I suspect this is related to one or both of
- http://engineeringblog.yelp.com/2015/02/assert_called_once-threat-or-menace.html
- something is not getting correctly un-patched. I suspect the random failures are order based. Run with -v and compare the other of pass and fail.

On August 8, 2016 2:53:57 AM MDT, ChristianEhrhardt <email address hidden> wrote:
>Review: Approve
>
>Hi,
>I came to about the same conclusion regarding the build error at
>
>https://launchpadlibrarian.net/277356117/buildlog_ubuntu-xenial-amd64.curtin_0.1.0~bzr415-0ubuntu1~ubuntu16.04.1_BUILDING.txt.gz
>
>What I found interesting is, that it doesn't always show up.
>On my normal Laptop a make check just runs fine, but I can still
>reproduce with
>
>./tools/build-deb -S
>DEB_BUILD_OPTIONS=parallel=4 sbuild -Adxenial-amd64
>curtin_0.1.0~bzr415-0ubuntu1.dsc
>
>While I wonder why it works in a local make check I still agree to the
>solution as assert_called isn't documented for the mockobject.
>
>--
>https://code.launchpad.net/~wesley-wiedenmeier/curtin/fix-unittests/+merge/302200
>Your team curtin developers is subscribed to branch lp:curtin.

[Scott Moser (smoser)]

ok...
so https://bugs.python.org/issue26323 is why you dont see this in tox.
Your (and my) tox environment has mock 2.0. yakkety has 2.0, xenial has 1.3.0.
Thats why I was able to develop this, and even build it locally.

That all said, i'm pulling the change.

Its possible that we should add a tox environment to run unit tests in that sets the versions for each release (similar to what 'trusty-check' does).

[Scott Moser (smoser)]

the one last thing, is that is concerning is Christian's comment about transient-ness. That would likely indicate there is some issue where one of the tests is not cleaning up correctly and that order of execution of tests matters. I fixed a similar of these issues when a test was modifying one of the globals in apt_source.

=== modified file 'tests/unittests/test_apt_source.py'
--- tests/unittests/test_apt_source.py	2016-08-04 21:05:09 +0000
+++ tests/unittests/test_apt_source.py	2016-08-05 23:37:55 +0000
@@ -932,7 +932,7 @@
         m_set_sel.return_value = None
 
         apt_config.apply_debconf_selections({'debconf_selections': data})
-        m_get_inst.assert_called()
+        self.assertTrue(m_get_inst.called)
         self.assertEqual(m_set_sel.call_count, 1)
 
         # assumes called with *args value.
@@ -979,7 +979,7 @@
 
         apt_config.apply_debconf_selections({'debconf_selections': data})
 
-        m_get_inst.assert_called()
+        self.assertTrue(m_get_inst.called)
         self.assertEqual(m_dpkg_r.call_count, 0)
 
     @mock.patch("curtin.commands.apt_config.util.subp")
@@ -996,7 +996,7 @@
                                         target=target)
         # cloud-init is actually the only package we have a cleaner for
         # so for now, its the only one that should reconfigured
-        m_subp.assert_called()
+        self.assertTrue(m_subp.called)
         ci_cleaner.assert_called_with(target)
         self.assertEqual(m_subp.call_count, 1)
         found = m_subp.call_args_list[0][0][0]