[PATH] s4-tests: extend LoadParam handling in tests

[Jelmer Vernooij]

On Tue, Dec 02, 2014 at 01:33:44PM +0100, Kamen Mazdrashki wrote:
> On Tue, Dec 2, 2014 at 1:09 PM, Jelmer Vernooij <jelmer at samba.org> wrote:
> > On Tue, Dec 02, 2014 at 05:23:44AM +0100, Kamen Mazdrashki wrote:
> > > Can you please take a look at attached patch.
> > > It extends TestCase.get_loadparm() functionality to:
> > > * throw KeyError in case "SMB_CONF_PATH" is not found in environment
> > > * load LoadParm load_default() in case function client requests default
> > > handling
> > >
> > > Patches in github:
> > >
> > >
> > https://github.com/kamenim/samba/commit/ae2bf19d8b52c72ded4ded424e8fd1180133dc81
> > >
> > >
> > https://github.com/kamenim/samba/commit/8758d8a2f383f70341a1f059a3944654c8d22339
> > >
> > > From ae2bf19d8b52c72ded4ded424e8fd1180133dc81 Mon Sep 17 00:00:00 2001
> > > From: Kamen Mazdrashki <kamenim at samba.org>
> > > Date: Tue, 2 Dec 2014 05:04:40 +0100
> > > Subject: [PATCH 1/2] s4-tests/env_loadparm: Throw KeyError in case
> > >  SMB_CONF_PATH
> > >
> > > A bit more specific for the caller to "know" that env key is missing
> > >
> > > Change-Id: I4d4c2121af868d79f46f865f420336222bc67347
> > > Signed-off-by: Kamen Mazdrashki <kamenim at samba.org>
> > > ---
> > >  python/samba/tests/__init__.py | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/python/samba/tests/__init__.py
> > b/python/samba/tests/__init__.py
> > > index 465a8f1..db09215 100644
> > > --- a/python/samba/tests/__init__.py
> > > +++ b/python/samba/tests/__init__.py
> > > @@ -92,7 +92,7 @@ def env_loadparm():
> > >      try:
> > >          lp.load(os.environ["SMB_CONF_PATH"])
> > >      except KeyError:
> > > -        raise Exception("SMB_CONF_PATH not set")
> > > +        raise KeyError("SMB_CONF_PATH not set")
> > >      return lp
> > >
> > >
> > > --
> > > 1.9.1
> > Reviewed-By: Jelmer Vernooij <jelmer at samba.org>
> >
> > > From 8758d8a2f383f70341a1f059a3944654c8d22339 Mon Sep 17 00:00:00 2001
> > > From: Kamen Mazdrashki <kamenim at samba.org>
> > > Date: Tue, 2 Dec 2014 05:18:39 +0100
> > > Subject: [PATCH 2/2] s4-tests/get_loadparm: Extend the helper to load
> > default
> > >  params if requested
> > >
> > > Change-Id: I7df1c29e573fe9ed24069efc28b94c5fc8c18aa1
> > > Signed-off-by: Kamen Mazdrashki <kamenim at samba.org>
> > > ---
> > >  python/samba/tests/__init__.py | 11 +++++++++--
> > >  1 file changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/python/samba/tests/__init__.py
> > b/python/samba/tests/__init__.py
> > > index db09215..76fe095 100644
> > > --- a/python/samba/tests/__init__.py
> > > +++ b/python/samba/tests/__init__.py
> > > @@ -50,8 +50,15 @@ class TestCase(TesttoolsTestCase):
> > >              samba.set_debug_level(test_debug_level)
> > >              self.addCleanup(samba.set_debug_level, test_debug_level)
> > >
> > > -    def get_loadparm(self):
> > > -        return env_loadparm()
> > > +    def get_loadparm(self, load_default=False):
> > > +        try:
> > > +            return env_loadparm()
> > > +        except KeyError:
> > > +            if not load_default:
> > > +                raise
> > > +        lp = param.LoadParm()
> > > +        lp.load_default()
> > > +        return lp
> > >
> > >      def get_credentials(self):
> > >          return cmdline_credentials
> > What is the reason for this change?
> >
> 
> It was primary  for my convenience - not to make a dummi smb.conf +
> defining another env variable.
> But yeah, your point is quite fair.
> I am using it like this:
> 
> https://github.com/kamenim/samba/commit/75a87863e8be5d23e28e79cab4e3c0d829ba24ee
> So the test author explicitly has to say "I don't car that much about
> default configuration"
> Especially in the above case, where "samba.tests.connect_samdb_env" is
> going to
> populate username/password anyway.
> So in the end, test writter get "i do care" behaviour out of the box.
> But I get you point and if you NAK it is ok (i will just update the comment
> in the begging of the test
> to remind myself how to run it from command line :)
I would prefer to require explicit configuration to prevent test isolation issues.

If the test doesn't really need a configuration, then it shouldn't be calling
get_loadparm() but rather just calling param.LoadParm() without load_default().

What specific test are you looking at?

Cheers,

Jelmer