Code review comment for lp:~abentley/juju-ci-tools/industrial-test

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 14-10-17 09:47 AM, Curtis Hovey wrote:
>> + def run_stages(self): + for attempt in
>> self.stage_attempts: + result =
>> attempt.do_stage(self.old_client, self.new_client) + yield result
>> + if False in result: + try: +
>> self.old_client.destroy_environment() + finally:
>
> Sorry. I don't understand this. If one stage fails, we destroy
> both?, not the one we know that failed?

Yes. We want a side-by-side comparison of old_client vs new_client,
to minimize the chance that substrate-side errors are counted as
problems with one client, but not the other.

If we are doing a side-by-side comparison, a failure in either (or
both) clients means we must stop both clients, because it ceases to be
a side-by-side comparison if one client cannot continue.

I haven't implemented the part that re-runs an industrial test until
every stage has been attempted sufficient times, but when I do, a
failure in either client will cause us to re-run the industrial test,
in order to test the stages following the stage that failed. So even
if we weren't doing side-by-side comparisons, there is no advantage in
continuing to run one client after the other fails.

The exception to the rule that failures require a re-run is, of
course, if the failure occurs in the final stage.

> Doesn't this code swallow an error if the old_client errors?

It's a finally block, so it doesn't swallow exceptions by default.
But if new_client.destroy_environment also raises, it will swallow the
old_client.destroy_environment exception.

While the exception for old_client can be suppressed, the juju error
messages for each "juju destroy-environment" invocation will not be
suppressed, and the exception from destroy_environment is not very
interesting (it just tells you which non-zero exit status was used).

>> + def test_destroy_both_even_with_exception(self): +
>> old_client = FakeEnvJujuClient('old') + new_client =
>> FakeEnvJujuClient('new') + industrial =
>> IndustrialTest(old_client, new_client, + [FakeAttempt(False,
>> False), FakeAttempt(True, True)]) + attempt =
>> industrial.run_stages() + with patch.object(old_client,
>> 'destroy_environment', + side_effect=Exception) as oc_mock: +
>> with patch.object(new_client, 'destroy_environment', +
>> side_effect=Exception) as nc_mock: + with
>> self.assertRaises(Exception): + list(attempt)
>
> Sorry, I don't know which exception is raised? Do we expect both?

We expect the new_client exception. I don't know of a way of raising
two exceptions simultaneously, so I don't know how we could get both.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJUQSdHAAoJEK84cMOcf+9hyKAH/2ESEERPpZQeR42SzJUXVWUY
sZw7nDB4Eoe94QG2UgC8YyoZKLpCwy/Ga2IOFKXptlwQdZXaKjztL2Jc36JsTXk1
Bk4PW4QWr/BHxRx1pNroQw/+YBXasTijQPhJO5eE5MgGqtkt5Rg+mIYDhM40d98t
wu4qrNjg7mR42Nno9j8SC7JakXTJzytQCKHG98349Sq/M1Cer0ixuAZM8LV2qY2d
7ZpjnnF4IumJ5EO3ygjwZB3sAO4SV2WU7YslBDPX7qZr3aNuqdYoN4uhvaXmwuyB
/uKjNN+u/lXyHq2Ousow9SEXVe7A2qzHHVl9KRvFHpRwI8U8Np0rhjkqDlbjPb0=
=+BZl
-----END PGP SIGNATURE-----