2012/4/17 Patrick Ohly <[email protected]>:
> 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.
I have a script setting up an environment for running
syncevo-dbus-server and it is exporting SYNCEVOLUTION_DEBUG. And I am
still using it for running dbus-test.py too.
>> 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.
Ah, this actually makes sense - "ENV" property can be used to append
env variables and env parameter of runCmdline to set environment. Will
fix it tomorrow.
In the meantime I pushed some tests to my branch:
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolution/commits/cmdline-tests-master
> 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
