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

[Kamen Mazdrashki]

Hi Jelmer,

Thanks for your review.

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:
> > Hi Andrew, Jelmer,
> >
> > 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
> >
> > Cheers,
> > Kamen
>
> > 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 :)


> This seems risky as it means we might load the users' system smb.conf when
> they
> are running the testsuite. The testsuite should explicitly not depend on
> the system smb.conf or any settings therein.
>
> Jelmer
>

Cheers,
Kamen