Is the _create_instance() function in TestConfigRunner in test/py/ ganeti.config_unittest.py no helper function then? I used the naming and documenting there as an inspiration...
On Fri, Apr 24, 2015 at 11:39 AM, Hrvoje Ribicic <[email protected]> wrote: > > > On Thu, Apr 23, 2015 at 4:49 PM, 'Lisa Velden' via ganeti-devel < > [email protected]> wrote: > >> Make sure that there is an environment variable for each parameter >> with the correct value. >> >> Signed-off-by: Lisa Velden <[email protected]> >> --- >> test/py/ganeti.backend_unittest.py | 40 >> +++++++++++++++++++++++++++++++++++++- >> 1 file changed, 39 insertions(+), 1 deletion(-) >> >> diff --git a/test/py/ganeti.backend_unittest.py b/test/py/ >> ganeti.backend_unittest.py >> index 1f05197..dcb4933 100755 >> --- a/test/py/ganeti.backend_unittest.py >> +++ b/test/py/ganeti.backend_unittest.py >> @@ -44,9 +44,9 @@ from ganeti import hypervisor >> from ganeti import netutils >> from ganeti import objects >> from ganeti import pathutils >> +from ganeti import serializer >> from ganeti import utils >> >> - >> > > To be consistent with other test files, please re-insert this line. > Pedantry, but unnecessary discrepancies should be avoided. > > >> class TestX509Certificates(unittest.TestCase): >> def setUp(self): >> self.tmpdir = tempfile.mkdtemp() >> @@ -948,5 +948,43 @@ class TestSpaceReportingConstants(unittest.TestCase): >> self.assertEqual(None, backend._STORAGE_TYPE_INFO_FN[storage_type]) >> >> >> +class TestOSEnvironment(unittest.TestCase): >> + """Ensures presence of public and private parameters inside >> + os environment variables""" >> > > Please reformat this docstring according to: > http://docs.ganeti.org/ganeti/master/html/dev-codestyle.html#docstrings > > >> + >> + def _create_env(self): >> > > For helper functions, we still use capitalization: e.g. _CreateEnv > > >> + """Creates and returns an environment""" >> > > Same for this docstring. > > >> + inst = objects.Instance(name="test.example.com", >> + uuid="test-uuid", >> + disks=[], nics=[], >> + disks_info="", >> + disk_template=constants.DT_DISKLESS, >> + primary_node="node.example.com", >> + os="debian-image", >> + osparams={"public_param": "public_info"}, >> + osparams_private= >> + serializer.PrivateDict({"private_param": >> + "private_info"}), >> > > I would throw in another private parameter here - it could be that the > conversion is mangled in some way and only processes one entry. > Being paranoid while testing is useful! > > >> + secondary_nodes=[], >> + beparams={}, >> + hvparams={}) >> > > Given that you are replicating a lot of the values provided by the logic > of config_mock, it would probably be better to instantiate a mock config > and create an instance. > This would keep this test more focused on private params. > > >> + >> + # supply required os attributes in a mock object >> + os_mock = mock.Mock() >> + os_mock.api_versions = [15] >> + os_mock.supported_variants = [""] >> + >> + return backend.OSEnvironment(inst, os_mock) >> + >> + def testParamPresence(self): >> + env = self._create_env() >> + self.assertTrue('OSP_PUBLIC_PARAM' in env) >> > > There is an assertIn function which will provide you with better error > reporting. > > >> + self.assertTrue('OSP_PRIVATE_PARAM' in env) >> > > Our style doc mandates double quotes for strings. > > >> + >> + def testParamValues(self): >> + env = self._create_env() >> + self.assertEqual(env['OSP_PUBLIC_PARAM'], "public_info") >> + self.assertEqual(env['OSP_PRIVATE_PARAM'], "private_info") >> > > While I understand where you were going with this, I think these two tests > should be combined. > One will necessarily fail without the other, so they will supply the same > amount of information either way. By using assertIn, the error messages > emitted will be good enough for the distinction to be intuitive. > > > >> + >> > > Two empty lines needed here. > > >> if __name__ == "__main__": >> testutils.GanetiTestProgram() >> -- >> 2.2.0.rc0.207.ga3a616c >> >> > Hrvoje Ribicic > Ganeti Engineering > Google Germany GmbH > Dienerstr. 12, 80331, München > > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg > Geschäftsführer: Graham Law, Christine Elizabeth Flores > Steuernummer: 48/725/00206 > Umsatzsteueridentifikationsnummer: DE813741370 >
