Merge lp://qastaging/~doanac/utah/bug1179531 into lp://qastaging/~utah/utah/0.12

On Wed, May 15, 2013 at 03:41:25AM -0000, Andy Doan wrote:
> Andy Doan has proposed merging lp:~doanac/utah/bug1179531 into lp:~utah/utah/0.12.
>
> Requested reviews:
> UTAH Dev (utah)
> Related bugs:
> Bug #1179531 in UTAH: "utah fails after reboot, but test continues to run on SUT"
> https://bugs.launchpad.net/utah/+bug/1179531
>
> For more details, see:
> https://code.launchpad.net/~doanac/utah/bug1179531/+merge/163828
>
> another regression for 0.12 to fix reboot support
> --
> https://code.launchpad.net/~doanac/utah/bug1179531/+merge/163828
> Your team UTAH Dev is requested to review the proposed merge of lp:~doanac/utah/bug1179531 into lp:~utah/utah/0.12.

> === modified file 'debian/changelog'
> --- debian/changelog 2013-05-13 11:40:41 +0000
> +++ debian/changelog 2013-05-15 03:40:30 +0000
> @@ -1,3 +1,9 @@
> +utah (0.12.4ubuntu1) released; urgency=low
> +
> + * Fix reboot support in runlists (LP: #1179531)
> +
> + -- Andy Doan <doanac@doanac-laptop> Tue, 14 May 2013 22:36:23 -0500
> +
> utah (0.12.3ubuntu2) raring; urgency=low
>
> * Check sftp_client is initialized before closing it (LP: #1178686)
>
> === modified file 'tests/test_run.py'
> --- tests/test_run.py 2013-05-01 18:56:23 +0000
> +++ tests/test_run.py 2013-05-15 03:40:30 +0000
> @@ -36,6 +36,7 @@
> _copy_preseed,
> _get_runlist,
> _write,
> + is_utah_done,
> master_runlist_argument,
> )
>
> @@ -154,3 +155,13 @@
> _copy_preseed(machine, args, logs)
> self.assertEqual(1, len(logs))
> self.assertTrue(copyfile.called)
> +
> + def test_is_utah_done(self):
> + machine = Mock()
> + machine.run.side_effect = UTAHException('blah')
> + try:
> + is_utah_done(machine, 1)
> + raise RuntimeError('is_utah_done should raise UTAHException')
> + except UTAHException as e:
> + self.assertEquals(str(e), 'blah')
> + self.assertTrue(e.retry)

I'm not sure if it'll help here, but you can use:

  self.assertRaises(UTAHException, is_utah_done, machine, 1)

I don't think you can do the exception inspection you are doing though.

>
> === modified file 'utah/run.py'
> --- utah/run.py 2013-05-02 21:30:31 +0000
> +++ utah/run.py 2013-05-15 03:40:30 +0000
> @@ -411,5 +411,5 @@
> else:
> logging.info('UTAH client is finished')
> return exitstatus
> - except socket.error as err:
> + except Exception as err:
> raise UTAHException(str(err), retry=True)
>

On 05/15/2013 08:22 AM, Joe Talbott wrote:
> I'm not sure if it'll help here, but you can use:
>
> self.assertRaises(UTAHException, is_utah_done, machine, 1)
>
> I don't think you can do the exception inspection you are doing though.

yeah - that's why I hacked it this way. The e.retry is critical here so
that we can ensure our retry logic would work properly.

The code looks good.

A few comments:
- socket module is no longer needed in `utah/run.py`.
- A documentation string is missing in `tests/test_run.py` for the test case
  that has been added.
- Instead of `str(e)` you can use `e.message` in the assertion that checks the
  exception message.
- The exception raised by the mock object should be an `Exception` (not a
  `UTAHException`) to ensure that the change captures all exceptions.
- I would recommend to use a context manager to check the exception contents:

       with self.assertRaises(Exception) as cm:
            is_utah_done(machine, 1)

        e = cm.exception
        self.assertEquals(e.message, 'blah')
        self.assertTrue(e.retry)

I think this fixes what Javier wants so I'm going to merge since we need this ASAP and he's gone for the day.

Yes, it does except for the exception class in the mock object, but I guess it's not a big deal.