On Tue, 2012-04-17 at 13:57 +0200, Krzesimir Nowak wrote:
> 2012/4/16 Patrick Ohly <[email protected]>:
> Some nitpicks here:
>
> + # Explicitly pass an environment. Otherwise subprocess.Popen()
> + # from Python 2.6 uses not os.environ (which would be okay)
> + # but rather the environment passed to a previous call to
> + # subprocess.Popen() (which will fail if the previous test ran
> + # with an environment which had SYNCEVOLUTION_DEBUG set).
> + if env == None:
> + env=os.environ
>
> 1. @property("debug", False) is not honored in runCmdline. If
> os.environ has SYNCEVOLUTION_DEBUG key then most of tests are going to
> fail, because stderr is not empty - it contains debugging information.
Are you concerned about not *unsetting* the variable if it is in the env
of test-dbus.py? I don't think test-dbus.py should be run like that, but
it won't hurt to remove it.
> 2. Same with "ENV" property - it is not honored. There is a test name
> testMatchTemplate which needs some different template directory
> (actually the one in test/testcases/templates). So for now the only
> way to do it is to deepcopy os.environ and set different
> SYNCEVOLUTION_TEMPLATE_DIR.
I see.
> You wrote at some point in this thread:
> "Regarding the environment, better refactor the code that is used for
> starting syncevo-dbus-server and use the same environment for both
> processes."
>
> Well, this clearly does not happen here. Would not it be better if
> environment used for running syncevo-dbus-server is stored in
> self.storedenv and that would be used for running syncevolution?
Agreed.
> --- a/test/test-dbus.py
> +++ b/test/test-dbus.py
> @@ -371,6 +371,7 @@ class DBusUtil(Timeout):
> quit_events = []
> reply = None
> pserver = None
> + storedenv = {}
>
> def getTestProperty(self, key, default):
> """retrieve values set with @property()"""
> @@ -422,6 +423,16 @@ class DBusUtil(Timeout):
> # and increase log level
> if self.getTestProperty("debug", True):
> env["SYNCEVOLUTION_DEBUG"] = "1"
> + else:
> + # we explicitly specified @property("debug", False")
> + # thus we do not want to have any debugging output.
> + try:
> + del env["SYNCEVOLUTION_DEBUG"]
> + except KeyError:
> + # there was no such key anyway
> + pass
> +
> + self.storedenv = env
>
> # can be set by a test to run additional tests on the content
> # of the D-Bus log
> @@ -3551,8 +3562,10 @@ class TestCmdline(unittest.TestCase, DBusUtil):
> # but rather the environment passed to a previous call to
> # subprocess.Popen() (which will fail if the previous test ran
> # with an environment which had SYNCEVOLUTION_DEBUG set).
> + cmdline_env = copy.deepcopy(self.storedenv)
> if env == None:
> - env=os.environ
> + env={}
> + cmdline_env.update(env)
> if preserveOutputOrder:
> s = subprocess.Popen(a, stdout=subprocess.PIPE,
> stderr=subprocess.STDOUT,
> env=env)
This changes the meaning of the env parameter from "set environment" to
"add to environment". I prefer the former semantic because it is more
powerful. At some point we might want to have tests for "D-Bus server
runs with additional env vars that client doesn't have", which can't be
done if runCmdline() always uses all env vars set for the server.
Other than that I agree with the patch.
--
Best Regards, Patrick Ohly
The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.
_______________________________________________
SyncEvolution mailing list
[email protected]
http://lists.syncevolution.org/listinfo/syncevolution
