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.

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.

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)

<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.

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 ;)

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).