Code review comment for lp://staging/~ursinha/ubuntu-ci-services-itself/bug-1290174-and-1287686

> Excellent !
>
> td;lr: nothing critical to fix but the env var leak... breaks isolation and
> those kind of issue are a pain to debug. Not sure it matters here as I don't
> clearly see the impact on other tests.
>
> Voting approve so you can sort out what you want to fix before landing.
>
> 46 + 'python-gnupg',
>
> Set a version if only to make sure we don't grab a newer one without noticing.

Fixed r367.

>
>
> 71 +KEY_DATA = """
> 72 +-----BEGIN PGP PUBLIC KEY BLOCK-----
> 73 +Version: GnuPG v1
>
> It would be nice to have the test infra to validate this on the fly, not a
> blocker but something to think about post phase-0.
>
> 117 + def _set_gnupg_home(self):
> 118 + os.environ["GNUPGHOME"] = self.gnupg_home
>
> You're leaking the env var, other tests may fail, you want:
>
> from ucitests import fixtures
>
> fixtures.override_env(self, 'GNUPGHOME', self.gnupg_home)
>
> which will take care of the cleanup for you.
>
> 120 + def _unset_gnupg_home(self):
> 121 + os.environ.pop("GNUPGHOME")
>
> so this can be deleted.

Nice :) Fixed r368.

>
> Likewise, using the following in setUp():
>
> fixtures.set_uniq_cwd(self)
> self.gnupg_home = '.'
>
> will take care of:
>
> 123 + def _destroy_gnupg_env(self):
> 124 + self._unset_gnupg_home()
> 125 + shutil.rmtree(self.gnupg_home)

This will take a bit more to figure out how to function properly, there are some tests that fail by setting this because they can't find sample data. Might not be hard to fix but would make this MP even bigger.

>
>
> <lots of fallouts in existing tests related to the gpg key change>
>
> I trust you got that by using that command locally, in the same vein of the
> remark about automating gpg key creation, all this data can be generated wit
> that local command and remove test code making it more readable and easier to
> reuse. Again, nothing to fix for now.

Actually I'm not using gpg directly anywhere with subprocess or similar, but python-dput is. The output you see is library output, I use python-gnupg.import_keys() and python-dput.validate_signatures(), and they give me the loads of messages you see. In near future I'll see what I can do to suppress such messages.

>
> Nit:
>
> 527 + self.assertEqual(mock_process.call_count, 2)
> 528 + self.assertEqual(mock_post.call_count, 1)
> 529 + self.assertEqual(mock_patch.call_count, 1)
>
> pick one of expected, actual or actual, expected but stick to it ;) You know
> my preference ;)

Oops, these are actually correct according to most of other tests. Two (or so) occurrences were backwards because I've changed the assertion but not the logic. Fixed r369.

>
> Nit:
>
> 860 + self.assertRaises(ChangesValidationError, processor.validate)
>
> this is valid but more and more I prefer:
>
> with self.assertRaises(ChangesValidationError) as cm:
> processor.validate()
>
> as it's easier to read and gives greater control to the test writer to add
> assertions on the exception details (if/when needed).