Hmm, these few functions seen to be outliers. I would recommend using the prevailing style.
As is our policy, we will leave the offenders be, but should someone touch that code, they can and should modify the code to fit the overall style. On Fri, Apr 24, 2015 at 11:51 AM, Lisa Velden <[email protected]> wrote: > 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 >> > > 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
