1:46 p.m.
Hello Chris!
You added a callback parameter to addResource() (formerly known as
enqueue()) and related functions. There's no documentation for that in
server.h/cpp. What is it meant to be used for, what kind of constraints
does it have, etc.? In other words, please document it for me... ;-)
--
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.
9:34 a.m.
2012/3/14 Patrick Ohly <patrick.ohly(a)intel.com>:
Hello Chris!
You added a callback parameter to addResource() (formerly known as
enqueue()) and related functions. There's no documentation for that in
server.h/cpp. What is it meant to be used for, what kind of constraints
does it have, etc.? In other words, please document it for me... ;-)
That is my doing again. Will document it. The callback was added,
because addResource calls checkQueue() which calls SetActive, which is
asynchronous. The callback is called when after all SetActive calls
are finished or, if there was no call to SetActive, at the end of
checkQueue(). In short - addResource, removeResource are asynchronous
too, so given callback is called when they are finished.
--
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
SyncEvolution(a)syncevolution.org
http://lists.syncevolution.org/listinfo/syncevolution
10:26 a.m.
On Thu, 2012-03-15 at 10:34 +0100, Krzesimir Nowak wrote:
2012/3/14 Patrick Ohly <patrick.ohly(a)intel.com>:
> Hello Chris!
>
> You added a callback parameter to addResource() (formerly known as
> enqueue()) and related functions. There's no documentation for that in
> server.h/cpp. What is it meant to be used for, what kind of constraints
> does it have, etc.? In other words, please document it for me... ;-)
>
That is my doing again. Will document it. The callback was added,
because addResource calls checkQueue() which calls SetActive, which is
asynchronous. The callback is called when after all SetActive calls
are finished or, if there was no call to SetActive, at the end of
checkQueue(). In short - addResource, removeResource are asynchronous
too, so given callback is called when they are finished.
Instead of just calling the parameter "callback" it would be good to use
a name which is related to the time when the callback is invoked.
The goal of the callback is to return the final result of a D-Bus method
invocation to the D-Bus client, right?
Why does that have to wait for the completion of SetActive? Can't we
tell the D-Bus client that its StartSession() was successful, then later
emit the right "status changed" signal?
The code is here:
// If not we activate the resource and place it in the active resource list.
if (canRun) {
m_activeResources.push_back(waitingResource);
// If this is a session, we set active and emit sessionChanged to
clients.
boost::shared_ptr<SessionResource> session =
boost::dynamic_pointer_cast<SessionResource>(wq_iter->lock());
if (session) {
++(*counter);
session->setActiveAsync(true, boost::bind(&Server::setActiveCb,
this, session, counter, callback));
}
m_waitingResources.erase(wq_iter++);
}
That seems unnecessarily complex to me, but perhaps I am missing something.
--
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.
1:43 p.m.
On Thu, 2012-03-15 at 11:26 +0100, Patrick Ohly wrote:
On Thu, 2012-03-15 at 10:34 +0100, Krzesimir Nowak wrote:
> 2012/3/14 Patrick Ohly <patrick.ohly(a)intel.com>:
> > Hello Chris!
> >
> > You added a callback parameter to addResource() (formerly known as
> > enqueue()) and related functions. There's no documentation for that in
> > server.h/cpp. What is it meant to be used for, what kind of constraints
> > does it have, etc.? In other words, please document it for me... ;-)
> >
>
> That is my doing again. Will document it. The callback was added,
> because addResource calls checkQueue() which calls SetActive, which is
> asynchronous. The callback is called when after all SetActive calls
> are finished or, if there was no call to SetActive, at the end of
> checkQueue(). In short - addResource, removeResource are asynchronous
> too, so given callback is called when they are finished.
Instead of just calling the parameter "callback" it would be good to use
a name which is related to the time when the callback is invoked.
The goal of the callback is to return the final result of a D-Bus method
invocation to the D-Bus client, right?
Why does that have to wait for the completion of SetActive? Can't we
tell the D-Bus client that its StartSession() was successful, then later
emit the right "status changed" signal?
The code is here:
// If not we activate the resource and place it in the active resource list.
if (canRun) {
m_activeResources.push_back(waitingResource);
// If this is a session, we set active and emit sessionChanged to
clients.
boost::shared_ptr<SessionResource> session =
boost::dynamic_pointer_cast<SessionResource>(wq_iter->lock());
if (session) {
++(*counter);
session->setActiveAsync(true,
boost::bind(&Server::setActiveCb, this, session, counter, callback));
}
m_waitingResources.erase(wq_iter++);
}
That seems unnecessarily complex to me, but perhaps I am missing something.
More observations/questions.
ConnectionResource and SessionResource share a lot of code: init(),
onQuit(), onFailure(), ... - basically the whole startup of the helper
seems to have been copied. Shouldn't that common logic be in the
Resource base class?
I also wonder about object ownership while the resource is still in the
process of getting constructed.
void SessionResource::createSessionResource(const Callback_t &callback,
Server &server,
const std::string &peerDeviceID,
const std::string &configName,
const std::string &session,
const std::vector<std::string>
&flags)
{
std::auto_ptr<SessionResource> resource(new SessionResource(server,
peerDeviceID,
configName,
session,
flags));
resource->init(callback);
// init did not throw an exception, so we guess that child was
// spawned successfully. thus we release the auto_ptr, so it will
// not delete the resource.
resource.release();
}
"we guess" is not encouraging when it comes to memory ownership ;-} Do
we know for sure? If createSessionResource() transfers owership to
init(), then it should do so explicitly.
Next, SessionResource::init(). It connects the resource to ForkExec
signals, using a plain pointer to the resource, without the
boost::signal2 track mechanism that I had suggested.
Who owns the SessionResource at that point? Who is going to free it when
something goes wrong before (eventually) addResource() is called? As far
as I can tell, the instance will simply leak.
Another situation is when the D-Bus client quits while its requested
session still gets constructed. The Client instance of the client will
get destructed. But the SessionResource was not yet connected to it, so
the construction cannot be aborted by Server.
I still think resources should be added to m_waitingResources before
construction is complete. Activating the object and notifying the caller
can wait until the resource is really usable (= helper ready).
--
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.
10:33 a.m.
On Thu, 2012-03-15 at 14:43 +0100, Patrick Ohly wrote:
I still think resources should be added to m_waitingResources before
construction is complete. Activating the object and notifying the
caller
can wait until the resource is really usable (= helper ready).
And one more observation: running the helper should get delayed until it
is really needed, which probably means until the resource is ready to
become active.
Imagine having 100 pending session requests. An entry in
m_waitingResources is cheap enough to handle that (admittedly unlikely)
case, but combine that with 100 syncevo-dbus-helper processes and we
have a problem.
--
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.
10:54 a.m.
On Fri, 2012-03-16 at 11:33 +0100, Patrick Ohly wrote:
On Thu, 2012-03-15 at 14:43 +0100, Patrick Ohly wrote:
> I still think resources should be added to m_waitingResources before
> construction is complete. Activating the object and notifying the
> caller
> can wait until the resource is really usable (= helper ready).
And one more observation: running the helper should get delayed until it
is really needed, which probably means until the resource is ready to
become active.
Imagine having 100 pending session requests. An entry in
m_waitingResources is cheap enough to handle that (admittedly unlikely)
case, but combine that with 100 syncevo-dbus-helper processes and we
have a problem.
Speaking of more than one entry in m_waitingResources: does the current
PriorityCompare really implement a strict weak ordering, as required by
std::set, the storage used for m_waitingResource?
I don't think it does, because two different resources with the same
priority will return false regardless which way you compare them, and
that despite not being equivalent. I bet that as a result, you currently
cannot store two resources with the same priority in m_waitingResource.
Even if you could, what would be the relative order? Everything else
being equal, we want first-come-first-serve.
Another concern is that the outcome of PriorityCompare depends on being
able to lock shared pointers. If a resource goes away, then the
invariants for the set might get violated (comparison no longer returns
the same results as before). I don't want to find out how std::set deals
with that...
In a nutshell, I think we need to go back to the original std::list and
its brute-force insertion sort. Agreed?
--
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.
noon
On Fri, Mar 16, 2012 at 11:54 AM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Fri, 2012-03-16 at 11:33 +0100, Patrick Ohly wrote:
> On Thu, 2012-03-15 at 14:43 +0100, Patrick Ohly wrote:
> > I still think resources should be added to m_waitingResources before
> > construction is complete. Activating the object and notifying the
> > caller
> > can wait until the resource is really usable (= helper ready).
>
> And one more observation: running the helper should get delayed until it
> is really needed, which probably means until the resource is ready to
> become active.
>
> Imagine having 100 pending session requests. An entry in
> m_waitingResources is cheap enough to handle that (admittedly unlikely)
> case, but combine that with 100 syncevo-dbus-helper processes and we
> have a problem.
Speaking of more than one entry in m_waitingResources: does the current
PriorityCompare really implement a strict weak ordering, as required by
std::set, the storage used for m_waitingResource?
I don't think it does, because two different resources with the same
priority will return false regardless which way you compare them, and
that despite not being equivalent. I bet that as a result, you currently
cannot store two resources with the same priority in m_waitingResource.
Even if you could, what would be the relative order? Everything else
being equal, we want first-come-first-serve.
oops, you're right. That should have been a multiset.
Here is a small test I'd written to make sure the ordering was correct.
// constructing multisets
#include <iostream>
#include <set>
using namespace std;
struct classComp {
bool operator() (const string& lhs, const string& rhs) const
{return lhs[0]>rhs[0];}
};
int main ()
{
multiset<string, classComp> setComp;
multiset<string>::iterator it;
setComp.insert("ace");
setComp.insert("bud");
setComp.insert("bad");
setComp.insert("bla");
setComp.insert("cup");
setComp.insert("boo");
// show content:
for ( it=setComp.begin() ; it != setComp.end(); it++ )
cout << (*it) << endl;
return 0;
}
The ordering works but for some reason, the "multi" prefix didn't make
it into the server.
Another concern is that the outcome of PriorityCompare depends on
being
able to lock shared pointers. If a resource goes away, then the
invariants for the set might get violated (comparison no longer returns
the same results as before). I don't want to find out how std::set deals
with that...
I believe I'm handling the case where the weak pointer goes away
already. If the lock is not obtained for the already-in-queue Resource
AND the to-insert Resource is locked the we move on to the next
comparison. If the to-insert Resource is gone then we stop right
there. I does get inserted but it'll be cleaned up in short time.
In a nutshell, I think we need to go back to the original std::list
and
its brute-force insertion sort. Agreed?
I found it a bit nicer to use the comparison class but if you're not
convinced feel free to revert that.
Cheers,
Chris
12:52 p.m.
On Fri, 2012-03-16 at 13:00 +0100, Chris Kühl wrote:
On Fri, Mar 16, 2012 at 11:54 AM, Patrick Ohly
<patrick.ohly(a)intel.com> wrote:
> On Fri, 2012-03-16 at 11:33 +0100, Patrick Ohly wrote:
>> On Thu, 2012-03-15 at 14:43 +0100, Patrick Ohly wrote:
>> > I still think resources should be added to m_waitingResources before
>> > construction is complete. Activating the object and notifying the
>> > caller
>> > can wait until the resource is really usable (= helper ready).
>>
>> And one more observation: running the helper should get delayed until it
>> is really needed, which probably means until the resource is ready to
>> become active.
>>
>> Imagine having 100 pending session requests. An entry in
>> m_waitingResources is cheap enough to handle that (admittedly unlikely)
>> case, but combine that with 100 syncevo-dbus-helper processes and we
>> have a problem.
>
> Speaking of more than one entry in m_waitingResources: does the current
> PriorityCompare really implement a strict weak ordering, as required by
> std::set, the storage used for m_waitingResource?
>
> I don't think it does, because two different resources with the same
> priority will return false regardless which way you compare them, and
> that despite not being equivalent. I bet that as a result, you currently
> cannot store two resources with the same priority in m_waitingResource.
> Even if you could, what would be the relative order? Everything else
> being equal, we want first-come-first-serve.
>
oops, you're right. That should have been a multiset.
Here is a small test I'd written to make sure the ordering was correct.
... but is it guaranteed to be correct, or does it just happen to work
as intended for your current implementation of std::multiset? I don't
see anything in the SGI STL documentation that says that a multiset
preserves the order in which elements are added.
The Linux man page says "Iteration is done in ascending order according
to the keys." That leaves it undefined what the order is for equal keys.
> Another concern is that the outcome of PriorityCompare depends
on being
> able to lock shared pointers. If a resource goes away, then the
> invariants for the set might get violated (comparison no longer returns
> the same results as before). I don't want to find out how std::set deals
> with that...
>
I believe I'm handling the case where the weak pointer goes away
already.
Your code does, but does std::multiset? It's data structure is based on
the result of the comparison between two elements. If that comparison
suddenly returns different results, some invariants inside std::multiset
get violated. All hell might break loose.
> In a nutshell, I think we need to go back to the original
std::list and
> its brute-force insertion sort. Agreed?
>
I found it a bit nicer to use the comparison class but if you're not
convinced feel free to revert that.
It's nicer, but I am not convinced that it is correct.
Another aspect is that m_waitingResources now mixes connections and
sessions. That wasn't the case before. A connection created a session
which then got scheduled, instead of being scheduled like one. There is
a reason for that: suppose there is a running connection for client A.
The client gives up on that connection and starts anew. Is that new
connection allowed to start? If not, it will be blocked by the stale
connection that it is meant to replace.
So we have a pretty complex logic now: allow multiple
ConnectionResources to be active, but prevent running multiple syncs.
Speaking of running a sync with a ConnectionResource: where is the
SessionResource that exposes the sync to D-Bus clients? The old
Connection class had
boost::shared_ptr<Session> m_session
That pointer still exists in the helper, but the counterpart in
syncevo-dbus-server is missing. Have you thought about this?
I'm wondering whether the old approach would be easier:
* keep the Connection logic inside syncevo-dbus-server
* schedule a normal SessionResource there
* use it once it is ready
One downside of this is that Connection needs to parse the incoming
message, for which it must instantiate libsynthesis. It's linked to
anyway via libsyncevolution, but not used elsewhere, so long term we
could get rid of it in syncevo-dbus-server by refactoring the libs if it
wasn't for this one aspect.
--
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.
2:35 p.m.
On Fri, 2012-03-16 at 13:52 +0100, Patrick Ohly wrote:
On Fri, 2012-03-16 at 13:00 +0100, Chris Kühl wrote:
> On Fri, Mar 16, 2012 at 11:54 AM, Patrick Ohly <patrick.ohly(a)intel.com>
wrote:
> > In a nutshell, I think we need to go back to the original std::list and
> > its brute-force insertion sort. Agreed?
> >
>
> I found it a bit nicer to use the comparison class but if you're not
> convinced feel free to revert that.
It's nicer, but I am not convinced that it is correct.
[...]
I'm wondering whether the old approach would be easier:
* keep the Connection logic inside syncevo-dbus-server
* schedule a normal SessionResource there
* use it once it is ready
One downside of this is that Connection needs to parse the incoming
message, for which it must instantiate libsynthesis. It's linked to
anyway via libsyncevolution, but not used elsewhere, so long term we
could get rid of it in syncevo-dbus-server by refactoring the libs if it
wasn't for this one aspect.
I'm going to experiment with the following changes on a branch:
* restore old work queue (list of sessions)
* run Connection entirely in syncevo-dbus-server, with blocking
sync done in a normal helper session
Krzesimir, you can document and rename the callbacks in parallel. Please
base it on the concurrent-sync-pohly-delayed-dbus branch.
--
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.
10:33 a.m.
2012/3/16 Patrick Ohly <patrick.ohly(a)intel.com>:
On Fri, 2012-03-16 at 13:52 +0100, Patrick Ohly wrote:
Krzesimir, you can document and rename the callbacks in parallel. Please
base it on the concurrent-sync-pohly-delayed-dbus branch.
It is in
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
I also started refactoring Resources to have:
- clean ownership situation when creating resources,
- resources tracking in signals
- additional callback for case when creating resource failed after
spawning a sync-helper.
The branch is
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
but I recommend not merging that, because I have to investigate why
some session test are now failing.
I was also thinking that we should use boost::slot instead of
boost::function for callback for tracking purposes. But if that is
needed then it may be done after I am done with the things above.
--
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.
10:39 a.m.
On Mon, 2012-03-19 at 11:33 +0100, Krzesimir Nowak wrote:
2012/3/16 Patrick Ohly <patrick.ohly(a)intel.com>:
> On Fri, 2012-03-16 at 13:52 +0100, Patrick Ohly wrote:
>
> Krzesimir, you can document and rename the callbacks in parallel. Please
> base it on the concurrent-sync-pohly-delayed-dbus branch.
>
It is in
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
I also started refactoring Resources to have:
- clean ownership situation when creating resources,
- resources tracking in signals
- additional callback for case when creating resource failed after
spawning a sync-helper.
The branch is
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
but I recommend not merging that, because I have to investigate why
some session test are now failing.
I was also thinking that we should use boost::slot instead of
boost::function for callback for tracking purposes. But if that is
needed then it may be done after I am done with the things above.
Thanks for the changes. I'll have a look at them as part of my own code
changes. Don't worry about the failing tests for now. The problem might
resolve itself as part of the work that I am doing.
--
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.
10:07 a.m.
2012/3/19 Patrick Ohly <patrick.ohly(a)intel.com>:
On Mon, 2012-03-19 at 11:33 +0100, Krzesimir Nowak wrote:
> 2012/3/16 Patrick Ohly <patrick.ohly(a)intel.com>:
> > On Fri, 2012-03-16 at 13:52 +0100, Patrick Ohly wrote:
> >
> > Krzesimir, you can document and rename the callbacks in parallel. Please
> > base it on the concurrent-sync-pohly-delayed-dbus branch.
> >
>
> It is in
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
>
> I also started refactoring Resources to have:
> - clean ownership situation when creating resources,
> - resources tracking in signals
> - additional callback for case when creating resource failed after
> spawning a sync-helper.
>
> The branch is
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
> but I recommend not merging that, because I have to investigate why
> some session test are now failing.
>
> I was also thinking that we should use boost::slot instead of
> boost::function for callback for tracking purposes. But if that is
> needed then it may be done after I am done with the things above.
Thanks for the changes. I'll have a look at them as part of my own code
changes. Don't worry about the failing tests for now. The problem might
resolve itself as part of the work that I am doing.
Hi, I just did forced push to my cssr1 branch with some fixes in
refactoring I did - apparently boost::signals does not immediately
clean up disconnected slots, which ended in keeping reference to
session resource. Thus client was not calling some code on detach,
because shared pointer was not unique. For now the test below fails
(but they I remember they passed before):
TestLocalSync.testConcurrency
TestLocalSync.testPasswordRequest
TestLocalSync.testSync
TestSessionAPIsDummy.testInteractivePassword
I don't know if these failures are related to my refactoring.
--
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.
10:46 a.m.
2012/3/20 Krzesimir Nowak <qdlacz(a)gmail.com>:
2012/3/19 Patrick Ohly <patrick.ohly(a)intel.com>:
> On Mon, 2012-03-19 at 11:33 +0100, Krzesimir Nowak wrote:
>> 2012/3/16 Patrick Ohly <patrick.ohly(a)intel.com>:
>> > On Fri, 2012-03-16 at 13:52 +0100, Patrick Ohly wrote:
>> >
>> > Krzesimir, you can document and rename the callbacks in parallel. Please
>> > base it on the concurrent-sync-pohly-delayed-dbus branch.
>> >
>>
>> It is in
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
>>
>> I also started refactoring Resources to have:
>> - clean ownership situation when creating resources,
>> - resources tracking in signals
>> - additional callback for case when creating resource failed after
>> spawning a sync-helper.
>>
>> The branch is
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
>> but I recommend not merging that, because I have to investigate why
>> some session test are now failing.
>>
>> I was also thinking that we should use boost::slot instead of
>> boost::function for callback for tracking purposes. But if that is
>> needed then it may be done after I am done with the things above.
>
> Thanks for the changes. I'll have a look at them as part of my own code
> changes. Don't worry about the failing tests for now. The problem might
> resolve itself as part of the work that I am doing.
Hi, I just did forced push to my cssr1 branch with some fixes in
refactoring I did - apparently boost::signals does not immediately
clean up disconnected slots, which ended in keeping reference to
session resource. Thus client was not calling some code on detach,
because shared pointer was not unique. For now the test below fails
(but they I remember they passed before):
TestLocalSync.testConcurrency
TestLocalSync.testPasswordRequest
TestLocalSync.testSync
TestSessionAPIsDummy.testInteractivePassword
I don't know if these failures are related to my refactoring.
I just updated libsynthesis to recent master and now those tests
mostly pass, but not reliably:
[ERROR syncevo-dbus-helper 00:00:00] sending message to child failed:
org.freedesktop.DBus.Error.UnknownMethod: No such interface
`org.syncevolution.localtransport.child' on object at path /
Looks like something is racy.
> --
> 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.
>
>
11:06 a.m.
On Tue, 2012-03-20 at 11:46 +0100, Krzesimir Nowak wrote:
[ERROR syncevo-dbus-helper 00:00:00] sending message to child
failed:
org.freedesktop.DBus.Error.UnknownMethod: No such interface
`org.syncevolution.localtransport.child' on object at path /
Looks like something is racy.
Probably the same issue as for
syncevo-dbus-server<->syncevo-dbus-helper, but now between
syncevo-dbus-helper<->syncevo-local-sync. See my email about not always
using delayed message processing in ForkExec - syncevo-local-sync still
starts processing right away, because its call to ForkExec was not
updated yet.
In my current work branch I have merged your ForkExec patch without the
"delayed = false" parameter.
--
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.
11:21 a.m.
2012/3/20 Patrick Ohly <patrick.ohly(a)intel.com>:
On Tue, 2012-03-20 at 11:46 +0100, Krzesimir Nowak wrote:
> [ERROR syncevo-dbus-helper 00:00:00] sending message to child failed:
> org.freedesktop.DBus.Error.UnknownMethod: No such interface
> `org.syncevolution.localtransport.child' on object at path /
>
> Looks like something is racy.
Probably the same issue as for
syncevo-dbus-server<->syncevo-dbus-helper, but now between
syncevo-dbus-helper<->syncevo-local-sync. See my email about not always
using delayed message processing in ForkExec - syncevo-local-sync still
starts processing right away, because its call to ForkExec was not
updated yet.
In my current work branch I have merged your ForkExec patch without the
"delayed = false" parameter.
Ah, right. I thought that you already did that for
concurrent-sync-pohly-delayed-dbus upon which my work is based.
I am now wondering if I should now try moving some common code from
session resource and connection resource to base class. I read that
you are trying to move connection stuff to server and that would end
with not using sync-helper for it, that is - connection resource
itself would disappear I think.
--
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.
11:27 a.m.
On Tue, 2012-03-20 at 12:21 +0100, Krzesimir Nowak wrote:
2012/3/20 Patrick Ohly <patrick.ohly(a)intel.com>:
> On Tue, 2012-03-20 at 11:46 +0100, Krzesimir Nowak wrote:
>> [ERROR syncevo-dbus-helper 00:00:00] sending message to child failed:
>> org.freedesktop.DBus.Error.UnknownMethod: No such interface
>> `org.syncevolution.localtransport.child' on object at path /
>>
>> Looks like something is racy.
>
> Probably the same issue as for
> syncevo-dbus-server<->syncevo-dbus-helper, but now between
> syncevo-dbus-helper<->syncevo-local-sync. See my email about not always
> using delayed message processing in ForkExec - syncevo-local-sync still
> starts processing right away, because its call to ForkExec was not
> updated yet.
>
> In my current work branch I have merged your ForkExec patch without the
> "delayed = false" parameter.
Ah, right. I thought that you already did that for
concurrent-sync-pohly-delayed-dbus upon which my work is based.
I am now wondering if I should now try moving some common code from
session resource and connection resource to base class.
Please stop for now. You'll end up duplicating work.
I read that
you are trying to move connection stuff to server and that would end
with not using sync-helper for it, that is - connection resource
itself would disappear I think.
Exactly. Connection and AutoSyncManager depend on common state
information (presence, session history). That state information should
stay in the syncevo-dbus-server together with the main logic in these
classes, to avoid constantly calling out to some helper.
--
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.
11:35 a.m.
2012/3/20 Patrick Ohly <patrick.ohly(a)intel.com>:
On Tue, 2012-03-20 at 12:21 +0100, Krzesimir Nowak wrote:
> 2012/3/20 Patrick Ohly <patrick.ohly(a)intel.com>:
> > On Tue, 2012-03-20 at 11:46 +0100, Krzesimir Nowak wrote:
> >> [ERROR syncevo-dbus-helper 00:00:00] sending message to child failed:
> >> org.freedesktop.DBus.Error.UnknownMethod: No such interface
> >> `org.syncevolution.localtransport.child' on object at path /
> >>
> >> Looks like something is racy.
> >
> > Probably the same issue as for
> > syncevo-dbus-server<->syncevo-dbus-helper, but now between
> > syncevo-dbus-helper<->syncevo-local-sync. See my email about not always
> > using delayed message processing in ForkExec - syncevo-local-sync still
> > starts processing right away, because its call to ForkExec was not
> > updated yet.
> >
> > In my current work branch I have merged your ForkExec patch without the
> > "delayed = false" parameter.
>
> Ah, right. I thought that you already did that for
> concurrent-sync-pohly-delayed-dbus upon which my work is based.
>
> I am now wondering if I should now try moving some common code from
> session resource and connection resource to base class.
Please stop for now. You'll end up duplicating work.
I have not begin that even. :) Is there something else you want to be
done by me in this area?
> I read that
> you are trying to move connection stuff to server and that would end
> with not using sync-helper for it, that is - connection resource
> itself would disappear I think.
Exactly. Connection and AutoSyncManager depend on common state
information (presence, session history). That state information should
stay in the syncevo-dbus-server together with the main logic in these
classes, to avoid constantly calling out to some helper.
--
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.
3:29 p.m.
On Fri, 2012-03-16 at 15:35 +0100, Patrick Ohly wrote:
On Fri, 2012-03-16 at 13:52 +0100, Patrick Ohly wrote:
> On Fri, 2012-03-16 at 13:00 +0100, Chris Kühl wrote:
> > On Fri, Mar 16, 2012 at 11:54 AM, Patrick Ohly <patrick.ohly(a)intel.com>
wrote:
> > > In a nutshell, I think we need to go back to the original std::list and
> > > its brute-force insertion sort. Agreed?
> > >
> >
> > I found it a bit nicer to use the comparison class but if you're not
> > convinced feel free to revert that.
>
> It's nicer, but I am not convinced that it is correct.
[...]
> I'm wondering whether the old approach would be easier:
> * keep the Connection logic inside syncevo-dbus-server
> * schedule a normal SessionResource there
> * use it once it is ready
>
> One downside of this is that Connection needs to parse the incoming
> message, for which it must instantiate libsynthesis. It's linked to
> anyway via libsyncevolution, but not used elsewhere, so long term we
> could get rid of it in syncevo-dbus-server by refactoring the libs if it
> wasn't for this one aspect.
I'm going to experiment with the following changes on a branch:
* restore old work queue (list of sessions)
* run Connection entirely in syncevo-dbus-server, with blocking
sync done in a normal helper session
I've finished the initial server rewrite, see ccs-pohly. I have also
done the auto sync manager rewrite that I had planned. I think the key
difference (only run a subset of Session in the helper, no dead code for
running multiple sessions in parallel) makes the code easier and avoided
changes elsewhere in the server (like an entirely different internal
Connection API, asynchronous status queries, etc.).
The new design is less ambitious in some ways. For example, backends are
still loaded and activated in the main syncevo-dbus-server, and there is
no support for running multiple sync sessions in parallel. But I believe
that this can be added later without design changes.
All tests passed on my desktop, now I am testing on the nightly test
machine.
However, while working on this I noticed quite a few areas for which
there are no tests.
For example, running a command line operation is not tested. Support for
it is implemented, but only without the output handling. Before adding
that, the tests from the Cmdline unit tests should be incorporated into
test-dbus.py. That way it can be verified that the tests work (because
they fail initially with the new branch) and developing the feature
becomes easier (because testing doesn't have to be manual).
The main commit message has the details (pri1 is something that I'll
work on immediately, pri2 is necessary before 1.3, pri3 is optional).
Help for anything in pri2 or pri3 is welcome.
commit 2250ea0ac8adb060eaacd73d7562643509cdb06e
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Mon Mar 26 17:19:25 2012 +0200
D-Bus server: run operations in syncevo-dbus-helper
This commit moves running the blocking syncing, database restore and
command line execution into the syncevo-dbus-helper process. The
advantage is that the main syncevo-dbus-server remains responsive
under all circumstances (fully asynchronous now) and suffers less from
memory leaks and/or crashes during a sync.
All test-dbus.py tests pass.
The core idea behind the new architecture is that Session remains the
D-Bus facing side of a session. It continues to run inside
syncevo-dbus-server and uses the syncevo-dbus-helper transparently via
a custom D-Bus interface between the two processes. State changes of
the helper are mirrored in the server.
Later the helper might also be used multiple times in a Session. For
example, anything related to loading backends should be moved into the
helper (currently the "is config usable" check still runs in the
syncevo-dbus-server and needs to load/initialize backends). The
startup code of the helper already handles that (see boolean result of
operation callback), but it is not used yet in practice.
At the moment, only the helper provides a D-Bus API. It sends out
signals when it needs information from the server. The server watches
those and replies when ready. It has the disadvantage that the helper
is not automatically notified with a "connection lost" error when
waiting for information from the server. This is not currently
handled (see below). Might revise this approach as part of resolving
the issue.
The problem of "helper died unexpectedly" is already handled, by not
returning a D-Bus method reply until the requested operation is
completed.
The Connection class continues to use such a Session, as before. It's
now fully asynchronous and exchanges messages with the helper via the
Session class.
Inside syncevo-dbus-server, boost::signals2 and the dbus-callbacks
infrastructure for asynchronous methods execution are used heavily
now. The glib event loop is entered exactly once and only left to shut
down.
The AutoSyncManager was completely rewritten. The data structure is a
lot simpler now (basically just a cache of transient information about
a sync config and the relevant config properties that define auto
syncing). The main work happens inside the schedule() call, which
verifies whether a session can run and, if not possible for some
reasons, ensures that it gets invoked again when that blocker is
gone (timeout over, server idle, etc.). The new code also uses
signals/slots instead of explicit coupling between the different
classes.
pri3:
syncevo-dbus-helper and syncevo-local-sync are conceptually very
similar. Should investigate whether a single executable can serve both
functions.
pri1:
All code still lives inside the src/dbus/server directory. This
simplifies checking differences in partly modified files like
dbus-sync.cpp. The next commit will move the helper files.
pri1:
Redirecting output from a running sync or the command line operation
in the helper to the client of syncevo-dbus-server is not implemented.
Need to write tests for the old implementation first, then implement
the missing functionality.
pri2:
A sync is meant to finish even if the syncevo-dbus-server gets
killed. On the other hand, in some cases (pending password request,
waiting for next message from Connection) the helper needs to detect
that syncevo-dbus-server process died, because the helper will never
get a response. Need a test for these aspects.
pri2:
When syncevo-dbus-server is invoked without SYNCEVOLUTION_DEBUG, it
logs to the syslog instead of stdout. Need a test.
The syncevo-dbus-server now sends the final "Session.StatusChanged
done" signal immediately. The old implementation accidentally delayed
sending that for 100 seconds. The revised test-dbus.py checks for
more "session done" quit events to cover this fix.
--
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.
8:44 a.m.
2012/3/27 Patrick Ohly <patrick.ohly(a)intel.com>:
For example, running a command line operation is not tested. Support
for
it is implemented, but only without the output handling. Before adding
that, the tests from the Cmdline unit tests should be incorporated into
test-dbus.py. That way it can be verified that the tests work (because
they fail initially with the new branch) and developing the feature
becomes easier (because testing doesn't have to be manual).
I started working on it yesterday. Just to be sure - do all of the
tests from syncevo/Cmdline.cpp have to be ported?
Also, I guess that I can pass any value to be under 0th index of args
parameter for session.Execute()? I have seen "client-test" to be
passed in TestCmdline class, but it seems to be used nowhere in
Cmdline anyway.
--
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.
9:02 a.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
On Thu, 2012-03-29 at 10:44 +0200, Krzesimir Nowak wrote:
2012/3/27 Patrick Ohly <patrick.ohly(a)intel.com>:
> For example, running a command line operation is not tested. Support for
> it is implemented, but only without the output handling. Before adding
> that, the tests from the Cmdline unit tests should be incorporated into
> test-dbus.py. That way it can be verified that the tests work (because
> they fail initially with the new branch) and developing the feature
> becomes easier (because testing doesn't have to be manual).
I started working on it yesterday. Just to be sure - do all of the
tests from syncevo/Cmdline.cpp have to be ported?
Some might be genuine unit tests (exercise the Cmdline class and not so
much the implementation behind it), those can stay in Cmdline.cpp.
Everything which does real, functional testing of more than just the
Cmdline class (and I think most of the tests fall into that category)
should be extracted so that it can run with --disable-unittests and
exercises the entire system, like a normal user would.
Also, I guess that I can pass any value to be under 0th index of
args
parameter for session.Execute()? I have seen "client-test" to be
passed in TestCmdline class, but it seems to be used nowhere in
Cmdline anyway.
Careful here. The main goal is to have real tests involving
syncevolution as frontend and syncevo-dbus-server as backend. That means
the test driver needs to invoke the syncevolution binary. Unit tests are
useful to check for specific corner cases quickly, but in general I
prefer to have realistic integration tests which include the corner
cases.
Putting this into test-dbus.py has the advantage that there is quite a
bit of infrastructure for setting up and monitoring syncevo-dbus-server
and for creating the right environment (XDG env variables, content of
these directories - see @property("snapshot")).
Invoking and monitoring an executable instead of making D-Bus calls will
require some plumbing.
--
Best Regards
Patrick Ohly
Senior Software Engineer
Intel GmbH
Open Source Technology Center
Pützstr. 5 Phone: +49-228-2493652
53129 Bonn
Germany
10:56 a.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
2012/3/29 Patrick Ohly <patrick.ohly(a)intel.com>:
On Thu, 2012-03-29 at 10:44 +0200, Krzesimir Nowak wrote:
> 2012/3/27 Patrick Ohly <patrick.ohly(a)intel.com>:
> > For example, running a command line operation is not tested. Support for
> > it is implemented, but only without the output handling. Before adding
> > that, the tests from the Cmdline unit tests should be incorporated into
> > test-dbus.py. That way it can be verified that the tests work (because
> > they fail initially with the new branch) and developing the feature
> > becomes easier (because testing doesn't have to be manual).
>
> I started working on it yesterday. Just to be sure - do all of the
> tests from syncevo/Cmdline.cpp have to be ported?
Some might be genuine unit tests (exercise the Cmdline class and not so
much the implementation behind it), those can stay in Cmdline.cpp.
Everything which does real, functional testing of more than just the
Cmdline class (and I think most of the tests fall into that category)
should be extracted so that it can run with --disable-unittests and
exercises the entire system, like a normal user would.
Ok, before I port more tests I would like you to just have a look on
one I have ported (testSetupScheduleWorld).
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
I guess that you will want it to be done differently.
> Also, I guess that I can pass any value to be under 0th index of
args
> parameter for session.Execute()? I have seen "client-test" to be
> passed in TestCmdline class, but it seems to be used nowhere in
> Cmdline anyway.
Careful here. The main goal is to have real tests involving
syncevolution as frontend and syncevo-dbus-server as backend. That means
the test driver needs to invoke the syncevolution binary. Unit tests are
useful to check for specific corner cases quickly, but in general I
prefer to have realistic integration tests which include the corner
cases.
Oh, I thought that we are going to call Session.Execute() here. That
is what I have done in my port. For now it ends with following errors:
GConf Error: Configuration server couldn't be contacted: D-BUS error:
Method "GetDatabase" with signature "s" on interface
"org.gnome.GConf.Server" doesn't exist
GConf Error: Configuration server couldn't be contacted: D-BUS error:
Method "GetDatabase" with signature "s" on interface
"org.gnome.GConf.Server" doesn't exist
GConf Error: Configuration server couldn't be contacted: D-BUS error:
Method "GetDatabase" with signature "s" on interface
"org.gnome.GConf.Server" doesn't exist
GConf Error: Configuration server couldn't be contacted: D-BUS error:
Method "GetDatabase" with signature "s" on interface
"org.gnome.GConf.Server" doesn't exist
GConf Error: Configuration server couldn't be contacted: D-BUS error:
Method "GetDatabase" with signature "s" on interface
"org.gnome.GConf.Server" doesn't exist
GConf Error: Configuration server couldn't be contacted: D-BUS error:
Method "GetDatabase" with signature "s" on interface
"org.gnome.GConf.Server" doesn't exist
GConf Error: Configuration server couldn't be contacted: D-BUS error:
Method "GetDatabase" with signature "s" on interface
"org.gnome.GConf.Server" doesn't exist
GConf Error: Configuration server couldn't be contacted: D-BUS error:
Method "GetDatabase" with signature "s" on interface
"org.gnome.GConf.Server" doesn't exist
GConf Error: Configuration server couldn't be contacted: D-BUS error:
Method "GetDatabase" with signature "s" on interface
"org.gnome.GConf.Server" doesn't exist
[WARNING syncevo-dbus-helper 00:00:00] e-data-server: Could not update
"/apps/evolution/addressbook/sources": Configuration server couldn't
be contacted: D-BUS error: Method "GetDatabase" with signature "s" on
interface "org.gnome.GConf.Server" doesn't exist
GConf Error: Configuration server couldn't be contacted: D-BUS error:
Method "GetDatabase" with signature "s" on interface
"org.gnome.GConf.Server" doesn't exist
GConf Error: Configuration server couldn't be contacted: D-BUS error:
Method "GetDatabase" with signature "s" on interface
"org.gnome.GConf.Server" doesn't exist
[DEBUG syncevo-dbus-helper 00:00:00] exception thrown at
../../../projekty/cxx/syncevolution/src/syncevo/DevNullConfigNode.h:46
[ERROR syncevo-dbus-helper 00:00:00] : virtual read-only configuration
node, cannot write property sync = two-way
[DEBUG syncevo-dbus-helper 00:00:00] terminating as requested by operation
[INFO syncevo-dbus-helper 00:00:00] terminating normally
[DEBUG 00:00:01] execute() helper call completed,
org.syncevolution.Server: : virtual read-only configuration node,
cannot write property sync = two-way
[ERROR 00:00:01] : virtual read-only configuration node, cannot write
property sync = two-way
[DEBUG 00:00:01] session failed: remember 10500 error
[DEBUG 00:00:01] session
/org/syncevolution/Session/3840802061333016421 done, config , not
modified, result 10500
Putting this into test-dbus.py has the advantage that there is quite
a
bit of infrastructure for setting up and monitoring syncevo-dbus-server
and for creating the right environment (XDG env variables, content of
these directories - see @property("snapshot")).
Invoking and monitoring an executable instead of making D-Bus calls will
require some plumbing.
--
Best Regards
Patrick Ohly
Senior Software Engineer
Intel GmbH
Open Source Technology Center
Pützstr. 5 Phone: +49-228-2493652
53129 Bonn
Germany
12:30 p.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
On Thu, 2012-03-29 at 12:56 +0200, Krzesimir Nowak wrote:
2012/3/29 Patrick Ohly <patrick.ohly(a)intel.com>:
> On Thu, 2012-03-29 at 10:44 +0200, Krzesimir Nowak wrote:
>> 2012/3/27 Patrick Ohly <patrick.ohly(a)intel.com>:
>> > For example, running a command line operation is not tested. Support for
>> > it is implemented, but only without the output handling. Before adding
>> > that, the tests from the Cmdline unit tests should be incorporated into
>> > test-dbus.py. That way it can be verified that the tests work (because
>> > they fail initially with the new branch) and developing the feature
>> > becomes easier (because testing doesn't have to be manual).
>>
>> I started working on it yesterday. Just to be sure - do all of the
>> tests from syncevo/Cmdline.cpp have to be ported?
>
> Some might be genuine unit tests (exercise the Cmdline class and not so
> much the implementation behind it), those can stay in Cmdline.cpp.
> Everything which does real, functional testing of more than just the
> Cmdline class (and I think most of the tests fall into that category)
> should be extracted so that it can run with --disable-unittests and
> exercises the entire system, like a normal user would.
>
Ok, before I port more tests I would like you to just have a look on
one I have ported (testSetupScheduleWorld).
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
I guess that you will want it to be done differently.
Yes, using Session.Execute() directly avoids (mostly in the negative
sense) testing whether the "syncevolution" tool and the "syncevolution
<-> syncevo-dbus-server" communication works. Better figure out how to
invoke syncevolution and capture its output. Bonus points if later some
tests can also be written which give data to syncevolution (interactive
password requests).
I wonder how valuable the inline encoding of config files really is
(createFiles()). test-dbus.py copies plain files instead. That way the
test-dbus.py remains smaller. On the other hand, editing tests now
requires editing multiple files. What do you think?
GConf Error: Configuration server couldn't be contacted: D-BUS
error:
Method "GetDatabase" with signature "s" on interface
"org.gnome.GConf.Server" doesn't exist
Looks like a GNOME setup problem. Neither org.gnome.GConf.Server nor
libgconf are used directly by SyncEvolution.
[DEBUG syncevo-dbus-helper 00:00:00] exception thrown at
../../../projekty/cxx/syncevolution/src/syncevo/DevNullConfigNode.h:46
[ERROR syncevo-dbus-helper 00:00:00] : virtual read-only configuration
node, cannot write property sync = two-way
[DEBUG syncevo-dbus-helper 00:00:00] terminating as requested by operation
[INFO syncevo-dbus-helper 00:00:00] terminating normally
[DEBUG 00:00:01] execute() helper call completed,
org.syncevolution.Server: : virtual read-only configuration node,
cannot write property sync = two-way
Interesting. Attaching to syncevo-dbus-helper and capturing the stack
backtrace would be useful.
But before you rathole there, better focus on writing the tests against
current master. That code should work and can serve as reference for the
expected behavior (with sanity checks, of course - there might be bugs
also in the old implementation).
--
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.
9:22 a.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
On Thu, 2012-03-29 at 14:30 +0200, Patrick Ohly wrote:
But before you rathole there, better focus on writing the tests
against
current master. That code should work and can serve as reference for the
expected behavior (with sanity checks, of course - there might be bugs
also in the old implementation).
Please base your work on the current "for-master/cmdline-output" branch.
This includes the stdout/stderr change that I mentioned in my email
yesterday and also cleans up the output so that it is the same, with or
without syncevo-dbus-server. I'm currently running one final sanity
check on the automated test server and then will merge that branch into
master.
Output checking is hard. Some parts of the output are variable, but
skipping over those parts entirely would miss cases where undesirable
output is included in them.
For example, the current TestCmdline would not catch the extra "[ERROR]
command line execution failure" added by the execution inside the
syncevo-dbus-server when the operation failed.
Probably a stricter regex would do the job: "output must start with
exactly this [ERROR], followed by this [INFO] line and then more [INFO]
lines of this format".
In the meantime I'll merge these changes into the ccs-pohly branch.
--
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.
1:59 p.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
(Removing Chris from CC).
2012/4/12 Patrick Ohly <patrick.ohly(a)intel.com>:
On Thu, 2012-03-29 at 14:30 +0200, Patrick Ohly wrote:
> But before you rathole there, better focus on writing the tests against
> current master. That code should work and can serve as reference for the
> expected behavior (with sanity checks, of course - there might be bugs
> also in the old implementation).
Please base your work on the current "for-master/cmdline-output" branch.
This includes the stdout/stderr change that I mentioned in my email
yesterday and also cleans up the output so that it is the same, with or
without syncevo-dbus-server. I'm currently running one final sanity
check on the automated test server and then will merge that branch into
master.
Output checking is hard. Some parts of the output are variable, but
skipping over those parts entirely would miss cases where undesirable
output is included in them.
For example, the current TestCmdline would not catch the extra "[ERROR]
command line execution failure" added by the execution inside the
syncevo-dbus-server when the operation failed.
Probably a stricter regex would do the job: "output must start with
exactly this [ERROR], followed by this [INFO] line and then more [INFO]
lines of this format".
In the meantime I'll merge these changes into the ccs-pohly branch.
Some of the tests are implemented in following branch:
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
Commits are not so informative - will do it later.
For now testTemplate is failing - I do not know whether this should be
fixed in Cmdline or rather the test should be adapted to current
behaviour. This place is marked as TODO.
--
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.
12:38 p.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
On Do, 2012-04-12 at 15:59 +0200, Krzesimir Nowak wrote:
> In the meantime I'll merge these changes into the ccs-pohly
branch.
Branch updated and rebased, see ccs-pohly-cmdline.
Some of the tests are implemented in following branch:
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
Commits are not so informative - will do it later.
And they are also incomplete, aren't they? stdout and stderr from
running the command line tool are captured, but they are not checked
anywhere. Nor is the return code checked.
+def runSyncEvolution(argsstr, envstr):
+ return subprocess.Popen("env `dbus-launch` " + envstr + " sh -c
'trap \"kill $DBUS_SESSION_BUS_PID\" EXIT; syncevolution " + argsstr +
"'", shell = True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+
It would be better to pass arguments as array to runSyncEvolution and
subprocess.Popen(). Same for the env. That avoids quoting issues:
+ argsstr = "--configure --sync-property \"proxyHost = proxy\"
scheduleworld addressbook"
+ envstr = "SYNCEVOLUTION_TEMPLATE_DIR=" +
os.environ["SYNCEVOLUTION_TEMPLATE_DIR"] + " XDG_CONFIG_HOME=" +
self.testdir + " HOME=" + self.testdir
Regarding the environment, better refactor the code that is used for
starting syncevo-dbus-server and use the same environment for both
processes.
Regarding the dbus-launch above, why is that necessary? test-dbus.py
must be running inside a valid D-Bus session. Quite the opposite,
launching another session probably prevents using the
syncevo-dbus-server which was already started in the outer D-Bus session
by test-dbus.py.
For now testTemplate is failing - I do not know whether this should
be
fixed in Cmdline or rather the test should be adapted to current
behaviour. This place is marked as TODO.
I'll have a look.
--
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.
1:11 p.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
2012/4/13 Patrick Ohly <patrick.ohly(a)intel.com>:
On Do, 2012-04-12 at 15:59 +0200, Krzesimir Nowak wrote:
> > In the meantime I'll merge these changes into the ccs-pohly branch.
Branch updated and rebased, see ccs-pohly-cmdline.
> Some of the tests are implemented in following branch:
>
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
>
> Commits are not so informative - will do it later.
And they are also incomplete, aren't they? stdout and stderr from
running the command line tool are captured, but they are not checked
anywhere. Nor is the return code checked.
Forgot about the return code. stdout and stderr are captured in other
tests (like testPrintConfig - not yet pushed to my branch).
+def runSyncEvolution(argsstr, envstr):
+ return subprocess.Popen("env `dbus-launch` " + envstr + " sh -c
'trap \"kill $DBUS_SESSION_BUS_PID\" EXIT; syncevolution " + argsstr +
"'", shell = True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+
It would be better to pass arguments as array to runSyncEvolution and
subprocess.Popen(). Same for the env. That avoids quoting issues:
+ argsstr = "--configure --sync-property \"proxyHost = proxy\"
scheduleworld addressbook"
+ envstr = "SYNCEVOLUTION_TEMPLATE_DIR=" +
os.environ["SYNCEVOLUTION_TEMPLATE_DIR"] + " XDG_CONFIG_HOME=" +
self.testdir + " HOME=" + self.testdir
Ok.
Regarding the environment, better refactor the code that is used for
starting syncevo-dbus-server and use the same environment for both
processes.
Ok.
Regarding the dbus-launch above, why is that necessary? test-dbus.py
must be running inside a valid D-Bus session. Quite the opposite,
launching another session probably prevents using the
syncevo-dbus-server which was already started in the outer D-Bus session
by test-dbus.py.
I guess it just worked for me. And those strange GConf errors
(probably used by some backend) stopped showing up.
> For now testTemplate is failing - I do not know whether this should be
> fixed in Cmdline or rather the test should be adapted to current
> behaviour. This place is marked as TODO.
I'll have a look.
I got it working in for-master/cmdline-output after forced update.
--
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.
1:45 p.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
On Fr, 2012-04-13 at 15:11 +0200, Krzesimir Nowak wrote:
2012/4/13 Patrick Ohly <patrick.ohly(a)intel.com>:
> On Do, 2012-04-12 at 15:59 +0200, Krzesimir Nowak wrote:
>> > In the meantime I'll merge these changes into the ccs-pohly branch.
>
> Branch updated and rebased, see ccs-pohly-cmdline.
>
>> Some of the tests are implemented in following branch:
>>
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
>>
>> Commits are not so informative - will do it later.
>
> And they are also incomplete, aren't they? stdout and stderr from
> running the command line tool are captured, but they are not checked
> anywhere. Nor is the return code checked.
Forgot about the return code. stdout and stderr are captured in other
tests (like testPrintConfig - not yet pushed to my branch).
My point is that they should *always* be tested, even if it just to
ensure that they are empty.
--
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.
1:28 p.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
On Fr, 2012-04-13 at 14:38 +0200, Patrick Ohly wrote:
On Do, 2012-04-12 at 15:59 +0200, Krzesimir Nowak wrote:
> Some of the tests are implemented in following branch:
>
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
>
> Commits are not so informative - will do it later.
And they are also incomplete, aren't they? stdout and stderr from
running the command line tool are captured, but they are not checked
anywhere.
The way how they are captured doesn't allow checking the right order of
output via stdout and stderr, because they are stored in separate
strings. But I am hard-pressed to suggest a better solution: one could
write a custom communicate() method which uses two pipes and copies into
one string for each stream and one combined string.
But that suffers from race conditions. If output is ready in both
streams when the parents starts to collect it, then the information
about which output is older is already lost.
I'm inclined to forgo testing the stdout/stderr distinction in most
tests: use the same pipe for both (so that we can test the order and
thus how it really looks to users) in most tests and then write a few
tests which check for stdout/stderr usage (and ignore the order).
Actually, any test which expects output only via one pipe can use two
pipes, because order doesn't matter when one pipe never has data.
Below my patch to make the tests work for me:
1. I had to set SYNCEVOLUTION_TEMPLATE_DIR before invoking
test-dbus.py, otherwise os.environ["SYNCEVOLUTION_TEMPLATE_DIR"]
fails. This really should be rethought.
2. As suspected, the wrong, system syncevo-dbus-server was used
because of the dbus-launch.
3. Output checking only gives "normal" results when
syncevo-dbus-server is run without SYNCEVOLUTION_DEBUG.
@property('debug', False) achieves that (better name welcome).
This is just a quick-and-dirty solution, not committed anywhere. Shall I
do the base work for a few tests and then let you continue with adding
the rest?
diff --git a/test/test-dbus.py b/test/test-dbus.py
index 262cf16..1c38ed8 100755
--- a/test/test-dbus.py
+++ b/test/test-dbus.py
@@ -420,7 +420,8 @@ class DBusUtil(Timeout):
# always print all debug output directly (no output redirection),
# and increase log level
- env["SYNCEVOLUTION_DEBUG"] = "1"
+ if self.getTestProperty("debug", True):
+ env["SYNCEVOLUTION_DEBUG"] = "1"
# can be set by a test to run additional tests on the content
# of the D-Bus log
@@ -3890,7 +3891,7 @@ def sortConfig(config):
return newconfig
def runSyncEvolution(argsstr, envstr):
- return subprocess.Popen("env `dbus-launch` " + envstr + " sh -c
'trap \"kill $DBUS_SESSION_BUS_PID\" EXIT; syncevolution " + argsstr +
"'", shell = True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+ return subprocess.Popen("env " + envstr + " syncevolution " +
argsstr, shell = True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
def lastLine(string):
return string.splitlines(True)[-1]
@@ -4009,7 +4010,11 @@ class TestCmdline(unittest.TestCase, DBusUtil):
envstr = "SYNCEVOLUTION_TEMPLATE_DIR=" +
os.environ["SYNCEVOLUTION_TEMPLATE_DIR"] + " XDG_CONFIG_HOME=" +
self.testdir + " HOME=" + self.testdir
syncevo = runSyncEvolution(argsstr, envstr)
- syncevo.communicate()
+ result = syncevo.communicate()
+ print "syncevolution result:", result, syncevo
+ self.assertEqual(0, syncevo.returncode, result[0] + result[1])
+ self.assertEqual("", result[0])
+ self.assertEqual("", result[1])
res = sortConfig(scanFiles(root))
res = self.removeRandomUUID(res)
res = self.removeSSLCertsPaths(res)
@@ -4029,13 +4034,14 @@ class TestCmdline(unittest.TestCase, DBusUtil):
shutil.rmtree(peer, True)
argsstr = "--configure --sync-property \"deviceId = fixed-devid\"
scheduleworld"
syncevo = runSyncEvolution(argsstr, envstr)
- syncevo.communicate()
+ result = syncevo.communicate()
res = scanFiles(root)
res = self.removeSSLCertsPaths(res)
expected = ScheduleWorldConfig()
expected = sortConfig(expected)
self.diffStrings(expected, res)
+ @property('debug', False)
def testSetupScheduleWorld(self):
"""TestCmdline.testSetupScheduleWorld - TODO:
description"""
self.doSetupScheduleWorld(False)
--
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.
3:27 p.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
2012/4/13 Patrick Ohly <patrick.ohly(a)intel.com>:
On Fr, 2012-04-13 at 14:38 +0200, Patrick Ohly wrote:
> On Do, 2012-04-12 at 15:59 +0200, Krzesimir Nowak wrote:
> > Some of the tests are implemented in following branch:
> >
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
> >
> > Commits are not so informative - will do it later.
>
> And they are also incomplete, aren't they? stdout and stderr from
> running the command line tool are captured, but they are not checked
> anywhere.
The way how they are captured doesn't allow checking the right order of
output via stdout and stderr, because they are stored in separate
strings. But I am hard-pressed to suggest a better solution: one could
write a custom communicate() method which uses two pipes and copies into
one string for each stream and one combined string.
But that suffers from race conditions. If output is ready in both
streams when the parents starts to collect it, then the information
about which output is older is already lost.
I'm inclined to forgo testing the stdout/stderr distinction in most
tests: use the same pipe for both (so that we can test the order and
thus how it really looks to users) in most tests and then write a few
tests which check for stdout/stderr usage (and ignore the order).
I wonder if there is some crazy platform where stdout is buffered so
long, that unbuffered stderr shows up first even if something earlier
was printed into stdout. I guess that such tests are hardly reliable
as an order of messages is guaranteed for one stream, but not between
two streams.
My idea would be to print usage messages to different streams
depending on situation. That is - if I call "syncevolution --help"
then it would print usage to stdout. But if I call "syncevolution
--template" then it would print usage to stderr with some "[ERROR]
missing parameter for 'template' option" following it.
Are there any other cases when we want to have messages in stdout and
stderr in some order?
Actually, any test which expects output only via one pipe can use
two
pipes, because order doesn't matter when one pipe never has data.
Below my patch to make the tests work for me:
1. I had to set SYNCEVOLUTION_TEMPLATE_DIR before invoking
test-dbus.py, otherwise os.environ["SYNCEVOLUTION_TEMPLATE_DIR"]
fails. This really should be rethought.
Yes, because there is a test (e.g. testMatchTemplate) which wants
different directory ("$(srcdir)/test/testcases/templates"). Maybe
copying some files/directories during build to $(builddir)/src would
suffice. That way we can specify SYNCEVOLUTION_TEMPLATE_DIR with
relative directories instead (like C++ tests are doing it).
2. As suspected, the wrong, system syncevo-dbus-server was used
because of the dbus-launch.
Yeah, well... Tests were working fine so far for me. :)
3. Output checking only gives "normal" results when
syncevo-dbus-server is run without SYNCEVOLUTION_DEBUG.
@property('debug', False) achieves that (better name welcome).
I just added a function stripping all lines having "[DEBUG" prefix.
The original output with debugs might be useful (maybe print it when
some test fails).
This is just a quick-and-dirty solution, not committed anywhere.
Shall I
do the base work for a few tests and then let you continue with adding
the rest?
Ok. I have some tests already written, but I will adapt them to your
changes. I still have to rebase them to your ccs-pohly-cmdline branch,
because my branch is based on todays for-master/cmdline-output one.
diff --git a/test/test-dbus.py b/test/test-dbus.py
index 262cf16..1c38ed8 100755
--- a/test/test-dbus.py
+++ b/test/test-dbus.py
@@ -420,7 +420,8 @@ class DBusUtil(Timeout):
# always print all debug output directly (no output redirection),
# and increase log level
- env["SYNCEVOLUTION_DEBUG"] = "1"
+ if self.getTestProperty("debug", True):
+ env["SYNCEVOLUTION_DEBUG"] = "1"
# can be set by a test to run additional tests on the content
# of the D-Bus log
@@ -3890,7 +3891,7 @@ def sortConfig(config):
return newconfig
def runSyncEvolution(argsstr, envstr):
- return subprocess.Popen("env `dbus-launch` " + envstr + " sh -c
'trap \"kill $DBUS_SESSION_BUS_PID\" EXIT; syncevolution " + argsstr +
"'", shell = True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+ return subprocess.Popen("env " + envstr + " syncevolution " +
argsstr, shell = True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
I guess we would need to store an environment used to launch
syncevo-dbus-server and reuse it here after moving runSyncEvolution to
DBusUtil class.
def lastLine(string):
return string.splitlines(True)[-1]
@@ -4009,7 +4010,11 @@ class TestCmdline(unittest.TestCase, DBusUtil):
envstr = "SYNCEVOLUTION_TEMPLATE_DIR=" +
os.environ["SYNCEVOLUTION_TEMPLATE_DIR"] + " XDG_CONFIG_HOME=" +
self.testdir + " HOME=" + self.testdir
syncevo = runSyncEvolution(argsstr, envstr)
- syncevo.communicate()
+ result = syncevo.communicate()
+ print "syncevolution result:", result, syncevo
+ self.assertEqual(0, syncevo.returncode, result[0] + result[1])
+ self.assertEqual("", result[0])
+ self.assertEqual("", result[1])
res = sortConfig(scanFiles(root))
res = self.removeRandomUUID(res)
res = self.removeSSLCertsPaths(res)
@@ -4029,13 +4034,14 @@ class TestCmdline(unittest.TestCase, DBusUtil):
shutil.rmtree(peer, True)
argsstr = "--configure --sync-property \"deviceId = fixed-devid\"
scheduleworld"
syncevo = runSyncEvolution(argsstr, envstr)
- syncevo.communicate()
+ result = syncevo.communicate()
res = scanFiles(root)
res = self.removeSSLCertsPaths(res)
expected = ScheduleWorldConfig()
expected = sortConfig(expected)
self.diffStrings(expected, res)
+ @property('debug', False)
def testSetupScheduleWorld(self):
"""TestCmdline.testSetupScheduleWorld - TODO:
description"""
self.doSetupScheduleWorld(False)
--
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.
3:40 p.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
On Fr, 2012-04-13 at 17:27 +0200, Krzesimir Nowak wrote:
2012/4/13 Patrick Ohly <patrick.ohly(a)intel.com>:
> On Fr, 2012-04-13 at 14:38 +0200, Patrick Ohly wrote:
>> On Do, 2012-04-12 at 15:59 +0200, Krzesimir Nowak wrote:
>> > Some of the tests are implemented in following branch:
>> >
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
>> >
>> > Commits are not so informative - will do it later.
>>
>> And they are also incomplete, aren't they? stdout and stderr from
>> running the command line tool are captured, but they are not checked
>> anywhere.
>
> The way how they are captured doesn't allow checking the right order of
> output via stdout and stderr, because they are stored in separate
> strings. But I am hard-pressed to suggest a better solution: one could
> write a custom communicate() method which uses two pipes and copies into
> one string for each stream and one combined string.
>
> But that suffers from race conditions. If output is ready in both
> streams when the parents starts to collect it, then the information
> about which output is older is already lost.
>
> I'm inclined to forgo testing the stdout/stderr distinction in most
> tests: use the same pipe for both (so that we can test the order and
> thus how it really looks to users) in most tests and then write a few
> tests which check for stdout/stderr usage (and ignore the order).
I wonder if there is some crazy platform where stdout is buffered so
long, that unbuffered stderr shows up first even if something earlier
was printed into stdout. I guess that such tests are hardly reliable
as an order of messages is guaranteed for one stream, but not between
two streams.
My idea would be to print usage messages to different streams
depending on situation. That is - if I call "syncevolution --help"
then it would print usage to stdout. But if I call "syncevolution
--template" then it would print usage to stderr with some "[ERROR]
missing parameter for 'template' option" following it.
Are there any other cases when we want to have messages in stdout and
stderr in some order?
Yes, running a sync. At least the way it is implemented now, "normal"
output is via stdout and all meta information is via stderr. In fact, it
might be the only example where it really matters, so my proposal to
test with two streams most of the time and then check one of them for
empty should work okay.
> Actually, any test which expects output only via one pipe can
use two
> pipes, because order doesn't matter when one pipe never has data.
>
> Below my patch to make the tests work for me:
> 1. I had to set SYNCEVOLUTION_TEMPLATE_DIR before invoking
> test-dbus.py, otherwise os.environ["SYNCEVOLUTION_TEMPLATE_DIR"]
> fails. This really should be rethought.
Yes, because there is a test (e.g. testMatchTemplate) which wants
different directory ("$(srcdir)/test/testcases/templates"). Maybe
copying some files/directories during build to $(builddir)/src would
suffice. That way we can specify SYNCEVOLUTION_TEMPLATE_DIR with
relative directories instead (like C++ tests are doing it).
test-dbus.py already expects certain files to be available. Adding
certain templates to that would be easy. But then
SYNCEVOLUTION_TEMPLATE_DIR would only have to be set for that test and
by that test itself, not by the user of test-dbus.py.
> 3. Output checking only gives "normal" results
when
> syncevo-dbus-server is run without SYNCEVOLUTION_DEBUG.
> @property('debug', False) achieves that (better name welcome).
I just added a function stripping all lines having "[DEBUG" prefix.
The original output with debugs might be useful (maybe print it when
some test fails).
I'd rather run the command line and the daemon exactly as the user does.
> This is just a quick-and-dirty solution, not committed anywhere.
Shall I
> do the base work for a few tests and then let you continue with adding
> the rest?
Ok. I have some tests already written, but I will adapt them to your
changes. I still have to rebase them to your ccs-pohly-cmdline branch,
No, let's finish the tests against master. That way we have a sound
baseline before starting to test the re-written server.
--
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.
4:27 p.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
I feel compelled to pitch in:
Krzesimir Nowak <qdlacz(a)gmail.com> wrote on Fri, 13 Apr 2012 17:27:50
+0200:
I wonder if there is some crazy platform where stdout is buffered so
long, that unbuffered stderr shows up first even if something earlier
was printed into stdout.
stdout is generally buffered, whereas stderr is not. The "crazy"
situation you describe happens all the time.
I guess that such tests are hardly reliable as an order of messages
is
guaranteed for one stream, but not between two streams.
Precisely. You should not rely on synchronicity between different
output streams.
Justus
6:38 p.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
On Fr, 2012-04-13 at 18:27 +0200, Justus-bulk(a)Piater.name wrote:
I feel compelled to pitch in:
Krzesimir Nowak <qdlacz(a)gmail.com> wrote on Fri, 13 Apr 2012 17:27:50
+0200:
> I wonder if there is some crazy platform where stdout is buffered so
> long, that unbuffered stderr shows up first even if something earlier
> was printed into stdout.
stdout is generally buffered, whereas stderr is not. The "crazy"
situation you describe happens all the time.
True in general, but not in the SyncEvolution binaries. They disable
buffering.
> I guess that such tests are hardly reliable as an order of
messages is
> guaranteed for one stream, but not between two streams.
Precisely. You should not rely on synchronicity between different
output streams.
That is indeed the problem.
--
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.
3:38 p.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
On Fri, 2012-04-13 at 15:28 +0200, Patrick Ohly wrote:
This is just a quick-and-dirty solution, not committed anywhere.
Shall I
do the base work for a few tests and then let you continue with adding
the rest?
Below are the commits, already merged into master after a quick test
run:
http://syncev.meego.com/2012-04-16-12-37_lucid-amd64_prebuilt-amd64_testi...
I started with your initial commit and then included the changes that we
were discussing, plus a fix for a failure still visible in that test
run.
The reason is that your scanFiles() implementation is slightly different
from the C++ version. In contrast to the C++ version it includes lines
like:
sources/addressbook/config.ini:# ActiveSync Address Book = eas-contacts
sources/addressbook/config.ini:# ActiveSync Events = eas-events
sources/addressbook/config.ini:# ActiveSync Todos = eas-todos
sources/addressbook/config.ini:# ActiveSync Memos = eas-memos
These need to be excluded by the comparison because these comments can
change, depending on how the binaries were compiled or run. Your
reference text in the ScheduleWorld config includes one particular
variation and therefore it worked for me when testing manually, but the
nightly testing differs.
A good test for the assertEqualDiff() :-) See:
http://syncev.meego.com/2012-04-16-12-37_lucid-amd64_prebuilt-amd64_testi...
With everything in place, do you still want to convert some more tests?
I think one commit per converted test would be good, with C++ removed
and Python added in the same test. I expect that most tests can and
should be moved. The only reason for keeping a test in the C++ code
would be if it explicitly tests some aspect that cannot be tested when
going through D-Bus - not sure whether there is any such case.
commit 2925b97bac45ae4dc4dac92b04b91683cad29ddc
Author: Krzesimir Nowak <krnowak(a)openismus.com>
Date: Thu Apr 12 15:45:02 2012 +0200
D-Bus Testing: add Cmdline tests framework
This is a translation of the corresponding C++ TestCmdline from
Cmdline.cpp into Python.
commit aa562e5ef97eba78b7c45ab095df5071134840dc
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Mon Apr 16 09:56:50 2012 +0200
D-Bus Testing: added assertEqualDiff()
Replaces the diffStrings() from the previous commit. Renamed it
because it really is specifying an assertion and moved it to
DBusUtil because it may be useful outside of the command line
testing.
Also made sure that the diff is part of the assertion because then it
will be repeated when listing the failure.
commit 5389b8eeeddc328e29a86c91aec8606ac3fb9741
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Mon Apr 16 10:54:52 2012 +0200
D-Bus Testing: ensure that we have assertRegexpMatches()
unittest.assertRegexpMatches() is very useful for command line
output testing. But it is not yet available in Ubuntu Lucid,
the current base line for SyncEvolution releases. Therefore
provide a fallback implementation.
commit 0e63bc8e59b53a8821beff9f29d061cae48a66fc
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Mon Apr 16 17:26:12 2012 +0200
D-Bus Testing: fixed scanFiles()
The function must return all property assignments, including those
which are commented out. The Python version accidentally included some
comment lines which looked like assignments, partly because it ignored
leading spaces, partly because it looked anywhere in the line for an
equal sign.
Fixed by using a regex for the matching.
commit 67ce74b57fcb51f57b8cf29e353e13da16d60b8b
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Mon Apr 16 12:13:14 2012 +0200
D-Bus Testing: eliminate TestCmdline.testdir
All D-Bus tests already have their private xdg_root = 'temp-test-dbus'
which is cleared before running each test. Use it also for TestCmdline
instead of creating our own temporary directory, for the sake of
consistency.
Another advantage is that the syncevo-dbus-server is running with that
same XDG root and reading/writing files in it automatically. The
command line client currently *does* send its XDG env variables and
the server uses them, but perhaps that feature will have to be
removed, because it has a design mistake (*), so don't depend on it.
(*) If different clients and the auto-syncing code in the server are
allowed to use different config roots, the 'config name' alone no
longer is a unique identifier for configs.
commit 7c2de4ed5199f9c971d1474595f5a04988c1eac3
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Mon Apr 16 12:58:52 2012 +0200
D-Bus Testing: added running of command line tool
The 'syncevolution' command line tool is started inside the same D-Bus
session as the 'syncevo-dbus-server' that is started before running
each test. They use the same XDG env variables.
The test-dbus.py script is able to watch and report all D-Bus traffic
between tool and server.
Because tests are meant to be realistic, SYNCEVOLUTION_DEBUG has to be
optional and must be turned off for all tests which test for real
output as seen by users.
Every single invocation of runCmdline() should check stdout and
stderr, to detect unexpected output. The return code is already tested
by runCmdline() itself, unless explicitly turned off (may be useful for
tests which expect certain kind of failures).
Ideally the entire string gets tested, either via an exact string
comparison or a full regex match. Note that assertRegexpMatches() only
does a search, so use ^ and $ to ensure full match.
If (and only if) the command is expected to produce output on both
stdout and stderr where the order matters (for example, syncing), then
stdout and stderr should be combined into one stream. Otherwise the
order cannot be tested.
commit 470f0e7d7a56b96adff21e5e918c58ee0a9e3420
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Mon Apr 16 13:02:23 2012 +0200
D-Bus Testing: added testSetupScheduleWorld
The test is a straight copy from Cmdline.cpp. Because the hard-coded
default SSL certificate path is not known in test-dbus.py, it has to
extract that string by printing a template once with the command line
tool (only the command line exposes the comments, normal D-Bus API
doesn't) and finding it in the output.
This allows copying the C++ tests without making them less strict by
filtering out the SSL certificate path before the result comparisons.
commit a27f2af580a00d81107f69bc5802c079cef23766
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Mon Apr 16 14:38:34 2012 +0200
D-Bus Testing: work around Python 2.6 subprocess bug
Explicitly pass an environment when running the command line
tool. 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).
--
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.
8:25 a.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
2012/4/16 Patrick Ohly <patrick.ohly(a)intel.com>:
On Fri, 2012-04-13 at 15:28 +0200, Patrick Ohly wrote:
> This is just a quick-and-dirty solution, not committed anywhere. Shall I
> do the base work for a few tests and then let you continue with adding
> the rest?
Below are the commits, already merged into master after a quick test
run:
http://syncev.meego.com/2012-04-16-12-37_lucid-amd64_prebuilt-amd64_testi...
I started with your initial commit and then included the changes that we
were discussing, plus a fix for a failure still visible in that test
run.
The reason is that your scanFiles() implementation is slightly different
from the C++ version. In contrast to the C++ version it includes lines
like:
sources/addressbook/config.ini:# ActiveSync Address Book = eas-contacts
sources/addressbook/config.ini:# ActiveSync Events = eas-events
sources/addressbook/config.ini:# ActiveSync Todos = eas-todos
sources/addressbook/config.ini:# ActiveSync Memos = eas-memos
Yeah, I noticed that yesterday - it was mostly fault of
isPropAssignment - I was stripping all whitespace at the beginning of
the passed line. That was bad idea. :)
These need to be excluded by the comparison because these comments
can
change, depending on how the binaries were compiled or run. Your
reference text in the ScheduleWorld config includes one particular
variation and therefore it worked for me when testing manually, but the
nightly testing differs.
A good test for the assertEqualDiff() :-) See:
http://syncev.meego.com/2012-04-16-12-37_lucid-amd64_prebuilt-amd64_testi...
With everything in place, do you still want to convert some more tests?
It should be a breeze, so I will try to be quick with it.
I think one commit per converted test would be good, with C++
removed
and Python added in the same test. I expect that most tests can and
should be moved. The only reason for keeping a test in the C++ code
would be if it explicitly tests some aspect that cannot be tested when
going through D-Bus - not sure whether there is any such case.
I guess there are possibly some:
testPeerConfigMigration - has some calls to SyncContext::setStableRelease
testFutureConfig - it calls SyncConfig directly in expectTooOld
testWebDAV - how can we detect if we have webdav enabled?
I guess that some of this code can be mimicked by calling
syncevolution with some options. Will look at it later.
commit 2925b97bac45ae4dc4dac92b04b91683cad29ddc
Author: Krzesimir Nowak <krnowak(a)openismus.com>
Date: Thu Apr 12 15:45:02 2012 +0200
D-Bus Testing: add Cmdline tests framework
This is a translation of the corresponding C++ TestCmdline from
Cmdline.cpp into Python.
commit aa562e5ef97eba78b7c45ab095df5071134840dc
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Mon Apr 16 09:56:50 2012 +0200
D-Bus Testing: added assertEqualDiff()
Replaces the diffStrings() from the previous commit. Renamed it
because it really is specifying an assertion and moved it to
DBusUtil because it may be useful outside of the command line
testing.
Also made sure that the diff is part of the assertion because then it
will be repeated when listing the failure.
commit 5389b8eeeddc328e29a86c91aec8606ac3fb9741
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Mon Apr 16 10:54:52 2012 +0200
D-Bus Testing: ensure that we have assertRegexpMatches()
unittest.assertRegexpMatches() is very useful for command line
output testing. But it is not yet available in Ubuntu Lucid,
the current base line for SyncEvolution releases. Therefore
provide a fallback implementation.
commit 0e63bc8e59b53a8821beff9f29d061cae48a66fc
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Mon Apr 16 17:26:12 2012 +0200
D-Bus Testing: fixed scanFiles()
The function must return all property assignments, including those
which are commented out. The Python version accidentally included some
comment lines which looked like assignments, partly because it ignored
leading spaces, partly because it looked anywhere in the line for an
equal sign.
Fixed by using a regex for the matching.
commit 67ce74b57fcb51f57b8cf29e353e13da16d60b8b
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Mon Apr 16 12:13:14 2012 +0200
D-Bus Testing: eliminate TestCmdline.testdir
All D-Bus tests already have their private xdg_root = 'temp-test-dbus'
which is cleared before running each test. Use it also for TestCmdline
instead of creating our own temporary directory, for the sake of
consistency.
Another advantage is that the syncevo-dbus-server is running with that
same XDG root and reading/writing files in it automatically. The
command line client currently *does* send its XDG env variables and
the server uses them, but perhaps that feature will have to be
removed, because it has a design mistake (*), so don't depend on it.
(*) If different clients and the auto-syncing code in the server are
allowed to use different config roots, the 'config name' alone no
longer is a unique identifier for configs.
commit 7c2de4ed5199f9c971d1474595f5a04988c1eac3
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Mon Apr 16 12:58:52 2012 +0200
D-Bus Testing: added running of command line tool
The 'syncevolution' command line tool is started inside the same D-Bus
session as the 'syncevo-dbus-server' that is started before running
each test. They use the same XDG env variables.
The test-dbus.py script is able to watch and report all D-Bus traffic
between tool and server.
Because tests are meant to be realistic, SYNCEVOLUTION_DEBUG has to be
optional and must be turned off for all tests which test for real
output as seen by users.
Every single invocation of runCmdline() should check stdout and
stderr, to detect unexpected output. The return code is already tested
by runCmdline() itself, unless explicitly turned off (may be useful for
tests which expect certain kind of failures).
Ideally the entire string gets tested, either via an exact string
comparison or a full regex match. Note that assertRegexpMatches() only
does a search, so use ^ and $ to ensure full match.
If (and only if) the command is expected to produce output on both
stdout and stderr where the order matters (for example, syncing), then
stdout and stderr should be combined into one stream. Otherwise the
order cannot be tested.
commit 470f0e7d7a56b96adff21e5e918c58ee0a9e3420
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Mon Apr 16 13:02:23 2012 +0200
D-Bus Testing: added testSetupScheduleWorld
The test is a straight copy from Cmdline.cpp. Because the hard-coded
default SSL certificate path is not known in test-dbus.py, it has to
extract that string by printing a template once with the command line
tool (only the command line exposes the comments, normal D-Bus API
doesn't) and finding it in the output.
This allows copying the C++ tests without making them less strict by
filtering out the SSL certificate path before the result comparisons.
commit a27f2af580a00d81107f69bc5802c079cef23766
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Mon Apr 16 14:38:34 2012 +0200
D-Bus Testing: work around Python 2.6 subprocess bug
Explicitly pass an environment when running the command line
tool. 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).
--
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.
11:57 a.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
2012/4/16 Patrick Ohly <patrick.ohly(a)intel.com>:
On Fri, 2012-04-13 at 15:28 +0200, Patrick Ohly wrote:
> This is just a quick-and-dirty solution, not committed anywhere. Shall I
> do the base work for a few tests and then let you continue with adding
> the rest?
Below are the commits, already merged into master after a quick test
run:
http://syncev.meego.com/2012-04-16-12-37_lucid-amd64_prebuilt-amd64_testi...
I started with your initial commit and then included the changes that we
were discussing, plus a fix for a failure still visible in that test
run.
The reason is that your scanFiles() implementation is slightly different
from the C++ version. In contrast to the C++ version it includes lines
like:
sources/addressbook/config.ini:# ActiveSync Address Book = eas-contacts
sources/addressbook/config.ini:# ActiveSync Events = eas-events
sources/addressbook/config.ini:# ActiveSync Todos = eas-todos
sources/addressbook/config.ini:# ActiveSync Memos = eas-memos
These need to be excluded by the comparison because these comments can
change, depending on how the binaries were compiled or run. Your
reference text in the ScheduleWorld config includes one particular
variation and therefore it worked for me when testing manually, but the
nightly testing differs.
A good test for the assertEqualDiff() :-) See:
http://syncev.meego.com/2012-04-16-12-37_lucid-amd64_prebuilt-amd64_testi...
With everything in place, do you still want to convert some more tests?
I think one commit per converted test would be good, with C++ removed
and Python added in the same test. I expect that most tests can and
should be moved. The only reason for keeping a test in the C++ code
would be if it explicitly tests some aspect that cannot be tested when
going through D-Bus - not sure whether there is any such case.
commit 2925b97bac45ae4dc4dac92b04b91683cad29ddc
Author: Krzesimir Nowak <krnowak(a)openismus.com>
Date: Thu Apr 12 15:45:02 2012 +0200
D-Bus Testing: add Cmdline tests framework
This is a translation of the corresponding C++ TestCmdline from
Cmdline.cpp into Python.
commit aa562e5ef97eba78b7c45ab095df5071134840dc
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Mon Apr 16 09:56:50 2012 +0200
D-Bus Testing: added assertEqualDiff()
Replaces the diffStrings() from the previous commit. Renamed it
because it really is specifying an assertion and moved it to
DBusUtil because it may be useful outside of the command line
testing.
Also made sure that the diff is part of the assertion because then it
will be repeated when listing the failure.
commit 5389b8eeeddc328e29a86c91aec8606ac3fb9741
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Mon Apr 16 10:54:52 2012 +0200
D-Bus Testing: ensure that we have assertRegexpMatches()
unittest.assertRegexpMatches() is very useful for command line
output testing. But it is not yet available in Ubuntu Lucid,
the current base line for SyncEvolution releases. Therefore
provide a fallback implementation.
commit 0e63bc8e59b53a8821beff9f29d061cae48a66fc
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Mon Apr 16 17:26:12 2012 +0200
D-Bus Testing: fixed scanFiles()
The function must return all property assignments, including those
which are commented out. The Python version accidentally included some
comment lines which looked like assignments, partly because it ignored
leading spaces, partly because it looked anywhere in the line for an
equal sign.
Fixed by using a regex for the matching.
commit 67ce74b57fcb51f57b8cf29e353e13da16d60b8b
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Mon Apr 16 12:13:14 2012 +0200
D-Bus Testing: eliminate TestCmdline.testdir
All D-Bus tests already have their private xdg_root = 'temp-test-dbus'
which is cleared before running each test. Use it also for TestCmdline
instead of creating our own temporary directory, for the sake of
consistency.
Another advantage is that the syncevo-dbus-server is running with that
same XDG root and reading/writing files in it automatically. The
command line client currently *does* send its XDG env variables and
the server uses them, but perhaps that feature will have to be
removed, because it has a design mistake (*), so don't depend on it.
(*) If different clients and the auto-syncing code in the server are
allowed to use different config roots, the 'config name' alone no
longer is a unique identifier for configs.
commit 7c2de4ed5199f9c971d1474595f5a04988c1eac3
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Mon Apr 16 12:58:52 2012 +0200
D-Bus Testing: added running of command line tool
The 'syncevolution' command line tool is started inside the same D-Bus
session as the 'syncevo-dbus-server' that is started before running
each test. They use the same XDG env variables.
The test-dbus.py script is able to watch and report all D-Bus traffic
between tool and server.
Because tests are meant to be realistic, SYNCEVOLUTION_DEBUG has to be
optional and must be turned off for all tests which test for real
output as seen by users.
Every single invocation of runCmdline() should check stdout and
stderr, to detect unexpected output. The return code is already tested
by runCmdline() itself, unless explicitly turned off (may be useful for
tests which expect certain kind of failures).
Ideally the entire string gets tested, either via an exact string
comparison or a full regex match. Note that assertRegexpMatches() only
does a search, so use ^ and $ to ensure full match.
If (and only if) the command is expected to produce output on both
stdout and stderr where the order matters (for example, syncing), then
stdout and stderr should be combined into one stream. Otherwise the
order cannot be tested.
commit 470f0e7d7a56b96adff21e5e918c58ee0a9e3420
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Mon Apr 16 13:02:23 2012 +0200
D-Bus Testing: added testSetupScheduleWorld
The test is a straight copy from Cmdline.cpp. Because the hard-coded
default SSL certificate path is not known in test-dbus.py, it has to
extract that string by printing a template once with the command line
tool (only the command line exposes the comments, normal D-Bus API
doesn't) and finding it in the output.
This allows copying the C++ tests without making them less strict by
filtering out the SSL certificate path before the result comparisons.
commit a27f2af580a00d81107f69bc5802c079cef23766
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Mon Apr 16 14:38:34 2012 +0200
D-Bus Testing: work around Python 2.6 subprocess bug
Explicitly pass an environment when running the command line
tool. 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).
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.
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.
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?
--- 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)
--
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.
12:03 p.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
2012/4/17 Krzesimir Nowak <qdlacz(a)gmail.com>:
2012/4/16 Patrick Ohly <patrick.ohly(a)intel.com>:
> On Fri, 2012-04-13 at 15:28 +0200, Patrick Ohly wrote:
>> This is just a quick-and-dirty solution, not committed anywhere. Shall I
>> do the base work for a few tests and then let you continue with adding
>> the rest?
>
> Below are the commits, already merged into master after a quick test
> run:
> http://syncev.meego.com/2012-04-16-12-37_lucid-amd64_prebuilt-amd64_testi...
>
> I started with your initial commit and then included the changes that we
> were discussing, plus a fix for a failure still visible in that test
> run.
>
> The reason is that your scanFiles() implementation is slightly different
> from the C++ version. In contrast to the C++ version it includes lines
> like:
>
> sources/addressbook/config.ini:# ActiveSync Address Book = eas-contacts
> sources/addressbook/config.ini:# ActiveSync Events = eas-events
> sources/addressbook/config.ini:# ActiveSync Todos = eas-todos
> sources/addressbook/config.ini:# ActiveSync Memos = eas-memos
>
> These need to be excluded by the comparison because these comments can
> change, depending on how the binaries were compiled or run. Your
> reference text in the ScheduleWorld config includes one particular
> variation and therefore it worked for me when testing manually, but the
> nightly testing differs.
>
> A good test for the assertEqualDiff() :-) See:
>
>
http://syncev.meego.com/2012-04-16-12-37_lucid-amd64_prebuilt-amd64_testi...
>
> With everything in place, do you still want to convert some more tests?
>
> I think one commit per converted test would be good, with C++ removed
> and Python added in the same test. I expect that most tests can and
> should be moved. The only reason for keeping a test in the C++ code
> would be if it explicitly tests some aspect that cannot be tested when
> going through D-Bus - not sure whether there is any such case.
>
>
> commit 2925b97bac45ae4dc4dac92b04b91683cad29ddc
> Author: Krzesimir Nowak <krnowak(a)openismus.com>
> Date: Thu Apr 12 15:45:02 2012 +0200
>
> D-Bus Testing: add Cmdline tests framework
>
> This is a translation of the corresponding C++ TestCmdline from
> Cmdline.cpp into Python.
>
> commit aa562e5ef97eba78b7c45ab095df5071134840dc
> Author: Patrick Ohly <patrick.ohly(a)intel.com>
> Date: Mon Apr 16 09:56:50 2012 +0200
>
> D-Bus Testing: added assertEqualDiff()
>
> Replaces the diffStrings() from the previous commit. Renamed it
> because it really is specifying an assertion and moved it to
> DBusUtil because it may be useful outside of the command line
> testing.
>
> Also made sure that the diff is part of the assertion because then it
> will be repeated when listing the failure.
>
> commit 5389b8eeeddc328e29a86c91aec8606ac3fb9741
> Author: Patrick Ohly <patrick.ohly(a)intel.com>
> Date: Mon Apr 16 10:54:52 2012 +0200
>
> D-Bus Testing: ensure that we have assertRegexpMatches()
>
> unittest.assertRegexpMatches() is very useful for command line
> output testing. But it is not yet available in Ubuntu Lucid,
> the current base line for SyncEvolution releases. Therefore
> provide a fallback implementation.
>
> commit 0e63bc8e59b53a8821beff9f29d061cae48a66fc
> Author: Patrick Ohly <patrick.ohly(a)intel.com>
> Date: Mon Apr 16 17:26:12 2012 +0200
>
> D-Bus Testing: fixed scanFiles()
>
> The function must return all property assignments, including those
> which are commented out. The Python version accidentally included some
> comment lines which looked like assignments, partly because it ignored
> leading spaces, partly because it looked anywhere in the line for an
> equal sign.
>
> Fixed by using a regex for the matching.
>
> commit 67ce74b57fcb51f57b8cf29e353e13da16d60b8b
> Author: Patrick Ohly <patrick.ohly(a)intel.com>
> Date: Mon Apr 16 12:13:14 2012 +0200
>
> D-Bus Testing: eliminate TestCmdline.testdir
>
> All D-Bus tests already have their private xdg_root = 'temp-test-dbus'
> which is cleared before running each test. Use it also for TestCmdline
> instead of creating our own temporary directory, for the sake of
> consistency.
>
> Another advantage is that the syncevo-dbus-server is running with that
> same XDG root and reading/writing files in it automatically. The
> command line client currently *does* send its XDG env variables and
> the server uses them, but perhaps that feature will have to be
> removed, because it has a design mistake (*), so don't depend on it.
>
> (*) If different clients and the auto-syncing code in the server are
> allowed to use different config roots, the 'config name' alone no
> longer is a unique identifier for configs.
>
> commit 7c2de4ed5199f9c971d1474595f5a04988c1eac3
> Author: Patrick Ohly <patrick.ohly(a)intel.com>
> Date: Mon Apr 16 12:58:52 2012 +0200
>
> D-Bus Testing: added running of command line tool
>
> The 'syncevolution' command line tool is started inside the same D-Bus
> session as the 'syncevo-dbus-server' that is started before running
> each test. They use the same XDG env variables.
>
> The test-dbus.py script is able to watch and report all D-Bus traffic
> between tool and server.
>
> Because tests are meant to be realistic, SYNCEVOLUTION_DEBUG has to be
> optional and must be turned off for all tests which test for real
> output as seen by users.
>
> Every single invocation of runCmdline() should check stdout and
> stderr, to detect unexpected output. The return code is already tested
> by runCmdline() itself, unless explicitly turned off (may be useful for
> tests which expect certain kind of failures).
>
> Ideally the entire string gets tested, either via an exact string
> comparison or a full regex match. Note that assertRegexpMatches() only
> does a search, so use ^ and $ to ensure full match.
>
> If (and only if) the command is expected to produce output on both
> stdout and stderr where the order matters (for example, syncing), then
> stdout and stderr should be combined into one stream. Otherwise the
> order cannot be tested.
>
> commit 470f0e7d7a56b96adff21e5e918c58ee0a9e3420
> Author: Patrick Ohly <patrick.ohly(a)intel.com>
> Date: Mon Apr 16 13:02:23 2012 +0200
>
> D-Bus Testing: added testSetupScheduleWorld
>
> The test is a straight copy from Cmdline.cpp. Because the hard-coded
> default SSL certificate path is not known in test-dbus.py, it has to
> extract that string by printing a template once with the command line
> tool (only the command line exposes the comments, normal D-Bus API
> doesn't) and finding it in the output.
>
> This allows copying the C++ tests without making them less strict by
> filtering out the SSL certificate path before the result comparisons.
>
> commit a27f2af580a00d81107f69bc5802c079cef23766
> Author: Patrick Ohly <patrick.ohly(a)intel.com>
> Date: Mon Apr 16 14:38:34 2012 +0200
>
> D-Bus Testing: work around Python 2.6 subprocess bug
>
> Explicitly pass an environment when running the command line
> tool. 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).
>
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.
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.
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?
--- 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)
Eh, should be:
--- 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,14 +3562,16 @@ 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)
+ env=cmdline_env)
else:
s = subprocess.Popen(a, stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
- env=env)
+ env=cmdline_env)
out, err = s.communicate()
if expectSuccess and s.returncode != 0:
result = 'syncevolution command failed.\nOutput:\n%s' % out
> --
> 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.
>
>
12:45 p.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
On Tue, 2012-04-17 at 13:57 +0200, Krzesimir Nowak wrote:
2012/4/16 Patrick Ohly <patrick.ohly(a)intel.com>:
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.
1:24 p.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
2012/4/17 Patrick Ohly <patrick.ohly(a)intel.com>:
On Tue, 2012-04-17 at 13:57 +0200, Krzesimir Nowak wrote:
> 2012/4/16 Patrick Ohly <patrick.ohly(a)intel.com>:
> 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-syncevolut...
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.
3:13 p.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
On Tue, 2012-04-17 at 15:24 +0200, Krzesimir Nowak wrote:
In the meantime I pushed some tests to my branch:
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
Some comment on that.
D-Bus Testing: added testMatchTemplate
------------------------------ test/test-dbus.py ------------------------------
index 21f69f3..d9abf2d 100755
@@ -4083,6 +4083,40 @@ sources/todo/config.ini:# databasePassword =
'''.format(
out, err, code = self.runCmdline(["--template", "? "])
self.assertEqualDiff(expected, out)
self.assertEqual(err, '')
+ # TODO: better directory name? reuse some copied&patched testcases? see src.am
+ @property("ENV",
"SYNCEVOLUTION_TEMPLATE_DIR=templates_from_test_testcases")
+ @property("debug", False)
+ def testMatchTemplate(self):
test/testcases/templates are the templates used for the Cmdline.cpp
tests. Copy the files to test/test-dbus/templates so that there is a
templates dir inside it. Use "SYNCEVOLUTION_TEMPLATE_DIR=" + xdg_root +
"/templates". Then in the test itself, call
self.setUpFiles('templates'). That should make the D-Bus server use the
test templates instead of the installed ones - completely untested,
though.
--
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.
10:51 a.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
2012/4/17 Patrick Ohly <patrick.ohly(a)intel.com>:
On Tue, 2012-04-17 at 15:24 +0200, Krzesimir Nowak wrote:
> In the meantime I pushed some tests to my branch:
>
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
Some comment on that.
D-Bus Testing: added testMatchTemplate
------------------------------ test/test-dbus.py ------------------------------
index 21f69f3..d9abf2d 100755
@@ -4083,6 +4083,40 @@ sources/todo/config.ini:# databasePassword =
'''.format(
out, err, code = self.runCmdline(["--template", "? "])
self.assertEqualDiff(expected, out)
self.assertEqual(err, '')
+ # TODO: better directory name? reuse some copied&patched testcases? see src.am
+ @property("ENV",
"SYNCEVOLUTION_TEMPLATE_DIR=templates_from_test_testcases")
+ @property("debug", False)
+ def testMatchTemplate(self):
test/testcases/templates are the templates used for the Cmdline.cpp
tests. Copy the files to test/test-dbus/templates so that there is a
templates dir inside it. Use "SYNCEVOLUTION_TEMPLATE_DIR=" + xdg_root +
"/templates". Then in the test itself, call
self.setUpFiles('templates'). That should make the D-Bus server use the
test templates instead of the installed ones - completely untested,
though.
That did the trick, thanks. I only had to add new pair to setUpFiles.
Also, runCmdline now uses either passed env or self.storedenv,
depending whether env is None or not.
I added some more tests, they are as usual on my branch:
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
testPrintFileTemplatesConfig is fishy for me and fails with an error
for now. There is a TODO about it. Could you look at it?
Also, there are some TODOs in testSync. Maybe some --print-config +
some parameters could mimick the C++ code there?
--
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.
7:46 a.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
On Wed, 2012-04-18 at 12:51 +0200, Krzesimir Nowak wrote:
I added some more tests, they are as usual on my branch:
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
When you move tests, can you also copy the comments associated with
them? For example:
void testPrintFileTemplates() {
// use local copy of templates in build dir (no need to install)
ScopedEnvChange templates("SYNCEVOLUTION_TEMPLATE_DIR",
"./templates");
The comment in the C++ version explains (briefly) what "./templates" is
meant to be: the "src/templates" that gets created in the build
directory. The (admittedly undocumented) assumption is that test-dbus.py
will always be run there.
The Python version replicates that without the comment:
# TODO: this test works, but I do not know what are the assumptions behind it.
@property("ENV", "SYNCEVOLUTION_TEMPLATE_DIR=./templates")
@property("debug", False)
def testPrintFileTemplates(self):
"""TestCmdline.testPrintFileTemplates - print file
templates"""
self.doPrintFileTemplates()
testPrintFileTemplatesConfig is fishy for me and fails with an error
for now. There is a TODO about it. Could you look at it?
The Python version doesn't quite match the C++ version. self.configdir =
$XDG_CONFIG_HOME/syncevolution != m_testDir = $XDG_CONFIG_HOME.
Here's a patch which makes the test work:
diff --git a/test/test-dbus.py b/test/test-dbus.py
index 5adea8f..fafe974 100755
--- a/test/test-dbus.py
+++ b/test/test-dbus.py
@@ -4379,19 +4379,30 @@ sources/todo/config.ini:# databasePassword =
'''.format(
self.assertEqualDiff(filterConfig(internalToIni(self.FunambolConfig())),
injectValues(filterConfig(out)))
-# TODO: this test works, but I do not know what are the assumptions behind it.
+ # Use local copy of templates in build dir (no need to install);
+ # this assumes that test-dbus.py is run in the build "src"
+ # directory.
@property("ENV", "SYNCEVOLUTION_TEMPLATE_DIR=./templates")
@property("debug", False)
def testPrintFileTemplates(self):
"""TestCmdline.testPrintFileTemplates - print file
templates"""
self.doPrintFileTemplates()
+ # Disable reading default templates, rely on finding the user
+ # ones (set up below via symlink).
@property("ENV", "SYNCEVOLUTION_TEMPLATE_DIR=/dev/null")
@property("debug", False)
def testPrintFileTemplatesConfig(self):
- """TestCmdline.testPrintFileTemplatesConfig - print file templates
config"""
-# TODO: This "../templates" looks fishy. I do not know what are the assumptions
behind it.
- os.symlink("../templates", self.configdir +
"/syncevolution-templates")
+ """TestCmdline.testPrintFileTemplatesConfig - print file templates
from user home directory"""
+ # The user's "home directory" or more precisely, his
+ # XDG_CONFIG_HOME, is set to xdg_root + "config". Make sure
+ # that there is a "syncevolution-templates" with valid
+ # templates in that "config" directory, because "templates"
+ # will not be found otherwise (SYNCEVOLUTION_TEMPLATE_DIR
+ # doesn't point to it).
+ os.makedirs(xdg_root + "/config")
+ # Use same "./templates" as in testPrintFileTemplates().
+ os.symlink("../../templates", xdg_root +
"/config/syncevolution-templates")
self.doPrintFileTemplates()
@property("debug", False)
Also, there are some TODOs in testSync. Maybe some --print-config +
some parameters could mimick the C++ code there?
I think the parts marked with TODO are examples of tests that better
stays in Cmdline.cpp because they are true unit tests (cover internal
implementation aspects).
--
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.
1:26 p.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
2012/4/19 Patrick Ohly <patrick.ohly(a)intel.com>:
On Wed, 2012-04-18 at 12:51 +0200, Krzesimir Nowak wrote:
> I added some more tests, they are as usual on my branch:
>
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
When you move tests, can you also copy the comments associated with
them? For example:
void testPrintFileTemplates() {
// use local copy of templates in build dir (no need to install)
ScopedEnvChange templates("SYNCEVOLUTION_TEMPLATE_DIR",
"./templates");
The comment in the C++ version explains (briefly) what "./templates" is
meant to be: the "src/templates" that gets created in the build
directory. The (admittedly undocumented) assumption is that test-dbus.py
will always be run there.
The Python version replicates that without the comment:
# TODO: this test works, but I do not know what are the assumptions behind it.
@property("ENV", "SYNCEVOLUTION_TEMPLATE_DIR=./templates")
@property("debug", False)
def testPrintFileTemplates(self):
"""TestCmdline.testPrintFileTemplates - print file
templates"""
self.doPrintFileTemplates()
> testPrintFileTemplatesConfig is fishy for me and fails with an error
> for now. There is a TODO about it. Could you look at it?
The Python version doesn't quite match the C++ version. self.configdir =
$XDG_CONFIG_HOME/syncevolution != m_testDir = $XDG_CONFIG_HOME.
Here's a patch which makes the test work:
diff --git a/test/test-dbus.py b/test/test-dbus.py
index 5adea8f..fafe974 100755
--- a/test/test-dbus.py
+++ b/test/test-dbus.py
@@ -4379,19 +4379,30 @@ sources/todo/config.ini:# databasePassword =
'''.format(
self.assertEqualDiff(filterConfig(internalToIni(self.FunambolConfig())),
injectValues(filterConfig(out)))
-# TODO: this test works, but I do not know what are the assumptions behind it.
+ # Use local copy of templates in build dir (no need to install);
+ # this assumes that test-dbus.py is run in the build "src"
+ # directory.
@property("ENV", "SYNCEVOLUTION_TEMPLATE_DIR=./templates")
@property("debug", False)
def testPrintFileTemplates(self):
"""TestCmdline.testPrintFileTemplates - print file
templates"""
self.doPrintFileTemplates()
+ # Disable reading default templates, rely on finding the user
+ # ones (set up below via symlink).
@property("ENV", "SYNCEVOLUTION_TEMPLATE_DIR=/dev/null")
@property("debug", False)
def testPrintFileTemplatesConfig(self):
- """TestCmdline.testPrintFileTemplatesConfig - print file
templates config"""
-# TODO: This "../templates" looks fishy. I do not know what are the
assumptions behind it.
- os.symlink("../templates", self.configdir +
"/syncevolution-templates")
+ """TestCmdline.testPrintFileTemplatesConfig - print file
templates from user home directory"""
+ # The user's "home directory" or more precisely, his
+ # XDG_CONFIG_HOME, is set to xdg_root + "config". Make sure
+ # that there is a "syncevolution-templates" with valid
+ # templates in that "config" directory, because "templates"
+ # will not be found otherwise (SYNCEVOLUTION_TEMPLATE_DIR
+ # doesn't point to it).
+ os.makedirs(xdg_root + "/config")
+ # Use same "./templates" as in testPrintFileTemplates().
+ os.symlink("../../templates", xdg_root +
"/config/syncevolution-templates")
self.doPrintFileTemplates()
@property("debug", False)
> Also, there are some TODOs in testSync. Maybe some --print-config +
> some parameters could mimick the C++ code there?
I think the parts marked with TODO are examples of tests that better
stays in Cmdline.cpp because they are true unit tests (cover internal
implementation aspects).
Small status update:
I got a sort of hickup, because suddenly tests stopped working -
running syncevolution via process subprocess module caused it to hang,
so tests were ending with errors after timeout. After googling a bit
about it I thought that using pipes to gather output from
syncevolution is the culprit - a deadlock may happen when output is so
large that it fills the pipe completely, so subprocess is blocked
during writing, because it waits for the pipe to be empty. Pipes are
emptied by reading from them, but that happen only after subprocess
finishes. But apparently that was not that - I changed runCmdline to
use just files, but hanging still happened. I tried running tests
after setting minimal environment needed for running the tests (PATH
and SYNCEVOLUTION_TEMPLATE_DIR) and the tests worked. It turned out
that G_SLICE env var set to "always-malloc" was messing something up.
What? I do not know and I was not investigating it - I have lost too
much time on it already.
Currently tests are in cmdline-tests-master branch which is rebased
against today's master:
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
(you will most probably get "503 Service Unavailable", I wonder when
word "gitorious" will hit the common directory as a synonym of being
crappy, slow or other negative adjectives)
Tests not ported:
testFutureConfig
testPeerConfigMigration
testContextConfigMigration
testOldConfigure
Tests failing on my computer:
testConfigureSource:
AssertionError: differences between expected and actual text
.internal.ini:contextMinVersion = 1
.internal.ini:contextCurVersion = 1
config.ini:# logdir =
config.ini:# maxlogdirs = 10
config.ini:deviceId = fixed-devid
sources/addressbook/config.ini:backend = file
sources/addressbook/config.ini:database = file://tmp/test
- sources/addressbook/config.ini:databaseFormat = text/x-vcard
? ----
+ sources/addressbook/config.ini:databaseFormat = /x-vcard
sources/addressbook/config.ini:# databaseUser =
sources/addressbook/config.ini:# databasePassword =
========
testItemOperations:
AssertionError: differences between expected and actual text
- BEGIN:VCARD
+ BEGIN
- VERSION:3.0
? ----
+ VERSION
- FN:John Doe
- N:Doe;John;;;
- END:VCARD
+ FN
+ N
+ END
- BEGIN:VCARD
+ BEGIN
- VERSION:3.0
? ----
+ VERSION
- FN:Joan Doe
- N:Doe;Joan;;;
- END:VCARD
+ FN
+ N
+ END
========
testMigrate and testMigrateAutoSync:
AssertionError: syncevolution command failed.
Output:
Separate stderr:
[ERROR] Server 'scheduleworld@default' has not been configured yet.
========
testMigrateContext:
AssertionError: 'sources/addressbook/config.ini:databaseFormat =
text/vcard' not found in 'long long text, will not paste it as a whole
here'
Relevant line of this ''long long text...' is:
'sources/addressbook/config.ini:# databaseFormat = '
========
Also, testConfigure is failing without workarounds there - stripping
the last newline from expected output. It is marked as 'WORKAROUND',
so it is easy to grep.
I have not yet removed the C++ tests. I guess that would be the next
step, together with "proofcomparing" C++ and Python tests.
--
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.
1:35 p.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
On Wed, 2012-04-25 at 15:26 +0200, Krzesimir Nowak wrote:
I got a sort of hickup, because suddenly tests stopped working -
running syncevolution via process subprocess module caused it to hang,
so tests were ending with errors after timeout. After googling a bit
about it I thought that using pipes to gather output from
syncevolution is the culprit - a deadlock may happen when output is so
large that it fills the pipe completely, so subprocess is blocked
during writing, because it waits for the pipe to be empty. Pipes are
emptied by reading from them, but that happen only after subprocess
finishes.
Using "communicate()" explicitly avoids that problem.
But apparently that was not that - I changed runCmdline to
use just files, but hanging still happened. I tried running tests
after setting minimal environment needed for running the tests (PATH
and SYNCEVOLUTION_TEMPLATE_DIR) and the tests worked. It turned out
that G_SLICE env var set to "always-malloc" was messing something up.
What? I do not know and I was not investigating it - I have lost too
much time on it already.
Okay, thanks for your time. I'll have a look at your branch as soon as I
can. Right now I am running full-scale tests of the "master" branch and
chase memory leaks in the GIO GDBus bindings + stack. Yes, debugging
takes time ;-}
--
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.
7:21 a.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
On Wed, 2012-04-25 at 15:26 +0200, Krzesimir Nowak wrote:
Small status update:
I got a sort of hickup, because suddenly tests stopped working -
running syncevolution via process subprocess module caused it to hang,
so tests were ending with errors after timeout. After googling a bit
about it I thought that using pipes to gather output from
syncevolution is the culprit - a deadlock may happen when output is so
large that it fills the pipe completely, so subprocess is blocked
during writing, because it waits for the pipe to be empty. Pipes are
emptied by reading from them, but that happen only after subprocess
finishes. But apparently that was not that - I changed runCmdline to
use just files, but hanging still happened. I tried running tests
after setting minimal environment needed for running the tests (PATH
and SYNCEVOLUTION_TEMPLATE_DIR) and the tests worked. It turned out
that G_SLICE env var set to "always-malloc" was messing something up.
What? I do not know and I was not investigating it - I have lost too
much time on it already.
I'll try to reproduce that in the nightly test platforms.
Currently tests are in cmdline-tests-master branch which is rebased
against today's master:
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
(you will most probably get "503 Service Unavailable", I wonder when
word "gitorious" will hit the common directory as a synonym of being
crappy, slow or other negative adjectives)
Migration to freedesktop.org is going to happen soonish.
Tests not ported:
testFutureConfig
testPeerConfigMigration
testContextConfigMigration
testOldConfigure
Tests failing on my computer:
testConfigureSource:
AssertionError: differences between expected and actual text
.internal.ini:contextMinVersion = 1
.internal.ini:contextCurVersion = 1
config.ini:# logdir =
config.ini:# maxlogdirs = 10
config.ini:deviceId = fixed-devid
sources/addressbook/config.ini:backend = file
sources/addressbook/config.ini:database = file://tmp/test
- sources/addressbook/config.ini:databaseFormat = text/x-vcard
? ----
+ sources/addressbook/config.ini:databaseFormat = /x-vcard
sources/addressbook/config.ini:# databaseUser =
sources/addressbook/config.ini:# databasePassword =
Cut-and-paste error from C++.
testItemOperations:
AssertionError: differences between expected and actual text
- BEGIN:VCARD
+ BEGIN
- VERSION:3.0
? ----
+ VERSION
- FN:John Doe
- N:Doe;John;;;
- END:VCARD
+ FN
+ N
+ END
A bug in the Python version of createFiles() - str.split() splits into
many components and threw away the parts after the second or higher
colon.
testMigrate and testMigrateAutoSync:
AssertionError: syncevolution command failed.
Output:
Separate stderr:
[ERROR] Server 'scheduleworld@default' has not been configured yet.
$HOME had to be set to find the really, really old ~/.sync4j config
files.
testMigrateContext:
AssertionError: 'sources/addressbook/config.ini:databaseFormat =
text/vcard' not found in 'long long text, will not paste it as a whole
here'
Relevant line of this ''long long text...' is:
'sources/addressbook/config.ini:# databaseFormat = '
Have not seen this myself after fixing the other things.
Also, testConfigure is failing without workarounds there - stripping
the last newline from expected output. It is marked as 'WORKAROUND',
so it is easy to grep.
Not looked at yet.
On current master I ran into issues with running a lot of syncs inside
client-test: at some point fork() starts to fail with "out of memory".
In runs which complete, valgrind doesn't show any major leaks. I don't
know yet whether that is because the process really starts allocating
lots of memory without freeing it at the end or an artifact of running
under valgrind.
Either way, running a sync inside a forked process will solve resp. work
around the issue. Maximum memory consumption is still something which
needs to be investigated, but first I want to get the "sync in forked
process" code merged.
I've pushed my fixes into a cmdline-tests-master branch and will rebase
another branch in preparation for testing and merging.
--
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.
7:25 a.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
On Fri, 2012-04-27 at 09:21 +0200, Patrick Ohly wrote:
I've pushed my fixes into a cmdline-tests-master branch and will
rebase
another branch in preparation for testing and merging.
I forgot to mention that one of the ported tests revealed a genuine bug:
--luids argument passing to the D-Bus server was broken. For me that
makes the entire effort worthwhile, besides making me a lot more
confident about not introducing regressions when merging the fork/exec
sync.
--
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.
10:09 a.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
2012/4/27 Patrick Ohly <patrick.ohly(a)intel.com>:
On Wed, 2012-04-25 at 15:26 +0200, Krzesimir Nowak wrote:
> Small status update:
>
> I got a sort of hickup, because suddenly tests stopped working -
> running syncevolution via process subprocess module caused it to hang,
> so tests were ending with errors after timeout. After googling a bit
> about it I thought that using pipes to gather output from
> syncevolution is the culprit - a deadlock may happen when output is so
> large that it fills the pipe completely, so subprocess is blocked
> during writing, because it waits for the pipe to be empty. Pipes are
> emptied by reading from them, but that happen only after subprocess
> finishes. But apparently that was not that - I changed runCmdline to
> use just files, but hanging still happened. I tried running tests
> after setting minimal environment needed for running the tests (PATH
> and SYNCEVOLUTION_TEMPLATE_DIR) and the tests worked. It turned out
> that G_SLICE env var set to "always-malloc" was messing something up.
> What? I do not know and I was not investigating it - I have lost too
> much time on it already.
I'll try to reproduce that in the nightly test platforms.
> Currently tests are in cmdline-tests-master branch which is rebased
> against today's master:
>
https://meego.gitorious.org/~krnowak/meego-middleware/krnowaks-syncevolut...
> (you will most probably get "503 Service Unavailable", I wonder when
> word "gitorious" will hit the common directory as a synonym of being
> crappy, slow or other negative adjectives)
Migration to freedesktop.org is going to happen soonish.
> Tests not ported:
> testFutureConfig
> testPeerConfigMigration
> testContextConfigMigration
> testOldConfigure
>
> Tests failing on my computer:
> testConfigureSource:
> AssertionError: differences between expected and actual text
>
> .internal.ini:contextMinVersion = 1
> .internal.ini:contextCurVersion = 1
> config.ini:# logdir =
> config.ini:# maxlogdirs = 10
> config.ini:deviceId = fixed-devid
> sources/addressbook/config.ini:backend = file
> sources/addressbook/config.ini:database = file://tmp/test
> - sources/addressbook/config.ini:databaseFormat = text/x-vcard
> ? ----
> + sources/addressbook/config.ini:databaseFormat = /x-vcard
> sources/addressbook/config.ini:# databaseUser =
> sources/addressbook/config.ini:# databasePassword =
Cut-and-paste error from C++.
> testItemOperations:
> AssertionError: differences between expected and actual text
>
> - BEGIN:VCARD
> + BEGIN
> - VERSION:3.0
> ? ----
> + VERSION
> - FN:John Doe
> - N:Doe;John;;;
> - END:VCARD
> + FN
> + N
> + END
A bug in the Python version of createFiles() - str.split() splits into
many components and threw away the parts after the second or higher
colon.
> testMigrate and testMigrateAutoSync:
> AssertionError: syncevolution command failed.
> Output:
>
> Separate stderr:
> [ERROR] Server 'scheduleworld@default' has not been configured yet.
$HOME had to be set to find the really, really old ~/.sync4j config
files.
> testMigrateContext:
> AssertionError: 'sources/addressbook/config.ini:databaseFormat =
> text/vcard' not found in 'long long text, will not paste it as a whole
> here'
> Relevant line of this ''long long text...' is:
> 'sources/addressbook/config.ini:# databaseFormat = '
Have not seen this myself after fixing the other things.
> Also, testConfigure is failing without workarounds there - stripping
> the last newline from expected output. It is marked as 'WORKAROUND',
> so it is easy to grep.
Not looked at yet.
On current master I ran into issues with running a lot of syncs inside
client-test: at some point fork() starts to fail with "out of memory".
In runs which complete, valgrind doesn't show any major leaks. I don't
know yet whether that is because the process really starts allocating
lots of memory without freeing it at the end or an artifact of running
under valgrind.
Either way, running a sync inside a forked process will solve resp. work
around the issue. Maximum memory consumption is still something which
needs to be investigated, but first I want to get the "sync in forked
process" code merged.
I've pushed my fixes into a cmdline-tests-master branch and will rebase
another branch in preparation for testing and merging.
--
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.
Thanks for finding and fixing the issues. I also have pushed one last
commit to my branch removing most of the C++ tests from Cmdline.cpp.
11:58 a.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
Hello!
All code is now in the master branch, including the fork/exec rewrite of
syncevo-dbus-server. The full test that ran over the weekend showed that
D-Bus worked without issues across the board (Ubuntu Lucid -> Debian
Testing, with GIO D-Bus and libdbus). Thanks everyone for making this
possible.
fork/exec in syncevo-dbus-server does not help with the "fork:
out-of-memory" errors seen when running client-test under valgrind,
because the latter doesn't use D-Bus at all... I might change that, to
make the testing more realistic.
--
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.
1:03 p.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
On Thu, 2012-03-29 at 12:56 +0200, Krzesimir Nowak wrote:
[DEBUG syncevo-dbus-helper 00:00:00] exception thrown at
../../../projekty/cxx/syncevolution/src/syncevo/DevNullConfigNode.h:46
[ERROR syncevo-dbus-helper 00:00:00] : virtual read-only configuration
node, cannot write property sync = two-way
Fixed, I think:
commit 287cf501936fe06c3e2940bc22f16e3aae5fabd4
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Fri Mar 30 14:30:21 2012 +0200
D-Bus server: fixed running a sync inside a command line session
CmdlineWrapper::createSyncClient() must pass a valid config name,
taken from the base Cmdline, to the DBusSync. Otherwise the sync runs
without access to a writeable sync config, leading to errors about
"virtual read-only configuration node, cannot write ..."
I've also experimented with output handling. Not done yet:
commit 40e29611476b6d7c048382f27eb5e7c5a134f72d
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Fri Mar 30 14:58:12 2012 +0200
D-Bus server: handle syncevo-dbus-helper output
This is a first shot at making the user-visible output created during
operations visible to the user again. It's based on the same idea as
output handling in the syncevo-dbus-server:
- Session registers itself as the top-most logger and sends
SyncEvolution logging via D-Bus to the parent, which re-sends
it with the right D-Bus object path as output of the session.
- Output redirection catches all other output and feeds it back
to the Session log handler, from where it goes via D-Bus
to the parent.
The advantage of this approach is that level information is made
available directly to the parent and that message boundaries are
preserved properly.
The disadvantage is that the current solution is racy: depending on
the order in which events are processed, the command line client quits
before it has printed all output from the helper. This needs further
work...
A better solution might be to capture stdout and stderr in
ForkExec.cpp, translate it back into messages and relay that to the
command line client. That would be guaranteed to capture everything
happening inside the helper.
I'll be away next week, so I'll put this aside for now.
--
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.
1:20 p.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
2012/3/30 Patrick Ohly <patrick.ohly(a)intel.com>:
On Thu, 2012-03-29 at 12:56 +0200, Krzesimir Nowak wrote:
> [DEBUG syncevo-dbus-helper 00:00:00] exception thrown at
> ../../../projekty/cxx/syncevolution/src/syncevo/DevNullConfigNode.h:46
> [ERROR syncevo-dbus-helper 00:00:00] : virtual read-only configuration
> node, cannot write property sync = two-way
Fixed, I think:
commit 287cf501936fe06c3e2940bc22f16e3aae5fabd4
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Fri Mar 30 14:30:21 2012 +0200
D-Bus server: fixed running a sync inside a command line session
CmdlineWrapper::createSyncClient() must pass a valid config name,
taken from the base Cmdline, to the DBusSync. Otherwise the sync runs
without access to a writeable sync config, leading to errors about
"virtual read-only configuration node, cannot write ..."
I've also experimented with output handling. Not done yet:
commit 40e29611476b6d7c048382f27eb5e7c5a134f72d
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Fri Mar 30 14:58:12 2012 +0200
D-Bus server: handle syncevo-dbus-helper output
This is a first shot at making the user-visible output created during
operations visible to the user again. It's based on the same idea as
output handling in the syncevo-dbus-server:
- Session registers itself as the top-most logger and sends
SyncEvolution logging via D-Bus to the parent, which re-sends
it with the right D-Bus object path as output of the session.
- Output redirection catches all other output and feeds it back
to the Session log handler, from where it goes via D-Bus
to the parent.
The advantage of this approach is that level information is made
available directly to the parent and that message boundaries are
preserved properly.
The disadvantage is that the current solution is racy: depending on
the order in which events are processed, the command line client quits
before it has printed all output from the helper. This needs further
work...
A better solution might be to capture stdout and stderr in
ForkExec.cpp, translate it back into messages and relay that to the
command line client. That would be guaranteed to capture everything
happening inside the helper.
Neat, and found why I had GConf errors - I was compiling syncevolution
under jhbuild, because system's GLib was too old (racy GDBus). I guess
that some parts of jhbuilded libs were not cooperating with system
libraries. For now I switched to bluez wrapper.
I'll be away next week, so I'll put this aside for now.
Yeah, me too. Have a nice holidays then.
--
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.
3:46 p.m.
New subject: [SyncEvolution] server class + fork/exec dbus-server + command line testing
2012/3/30 Krzesimir Nowak <qdlacz(a)gmail.com>:
2012/3/30 Patrick Ohly <patrick.ohly(a)intel.com>:
> On Thu, 2012-03-29 at 12:56 +0200, Krzesimir Nowak wrote:
>> [DEBUG syncevo-dbus-helper 00:00:00] exception thrown at
>> ../../../projekty/cxx/syncevolution/src/syncevo/DevNullConfigNode.h:46
>> [ERROR syncevo-dbus-helper 00:00:00] : virtual read-only configuration
>> node, cannot write property sync = two-way
>
> Fixed, I think:
>
> commit 287cf501936fe06c3e2940bc22f16e3aae5fabd4
> Author: Patrick Ohly <patrick.ohly(a)intel.com>
> Date: Fri Mar 30 14:30:21 2012 +0200
>
> D-Bus server: fixed running a sync inside a command line session
>
> CmdlineWrapper::createSyncClient() must pass a valid config name,
> taken from the base Cmdline, to the DBusSync. Otherwise the sync runs
> without access to a writeable sync config, leading to errors about
> "virtual read-only configuration node, cannot write ..."
>
> I've also experimented with output handling. Not done yet:
>
> commit 40e29611476b6d7c048382f27eb5e7c5a134f72d
> Author: Patrick Ohly <patrick.ohly(a)intel.com>
> Date: Fri Mar 30 14:58:12 2012 +0200
>
> D-Bus server: handle syncevo-dbus-helper output
>
> This is a first shot at making the user-visible output created during
> operations visible to the user again. It's based on the same idea as
> output handling in the syncevo-dbus-server:
> - Session registers itself as the top-most logger and sends
> SyncEvolution logging via D-Bus to the parent, which re-sends
> it with the right D-Bus object path as output of the session.
> - Output redirection catches all other output and feeds it back
> to the Session log handler, from where it goes via D-Bus
> to the parent.
>
> The advantage of this approach is that level information is made
> available directly to the parent and that message boundaries are
> preserved properly.
>
> The disadvantage is that the current solution is racy: depending on
> the order in which events are processed, the command line client quits
> before it has printed all output from the helper. This needs further
> work...
>
> A better solution might be to capture stdout and stderr in
> ForkExec.cpp, translate it back into messages and relay that to the
> command line client. That would be guaranteed to capture everything
> happening inside the helper.
Neat, and found why I had GConf errors - I was compiling syncevolution
under jhbuild, because system's GLib was too old (racy GDBus). I guess
that some parts of jhbuilded libs were not cooperating with system
libraries. For now I switched to bluez wrapper.
Actually, probably culprit is probably elsewhere - I have to run
syncevolution in similar manner like described in link below:
http://www.estamos.de/blog/2009/05/08/running-syncevolution-as-cron-job/
> I'll be away next week, so I'll put this aside for now.
Yeah, me too. Have a nice holidays then.
>
> --
> 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.
>
>
3:21 p.m.
On Fri, Mar 16, 2012 at 1:52 PM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
On Fri, 2012-03-16 at 13:00 +0100, Chris Kühl wrote:
> On Fri, Mar 16, 2012 at 11:54 AM, Patrick Ohly <patrick.ohly(a)intel.com> wrote:
> > On Fri, 2012-03-16 at 11:33 +0100, Patrick Ohly wrote:
> >> On Thu, 2012-03-15 at 14:43 +0100, Patrick Ohly wrote:
> >> > I still think resources should be added to m_waitingResources before
> >> > construction is complete. Activating the object and notifying the
> >> > caller
> >> > can wait until the resource is really usable (= helper ready).
> >>
> >> And one more observation: running the helper should get delayed until it
> >> is really needed, which probably means until the resource is ready to
> >> become active.
> >>
> >> Imagine having 100 pending session requests. An entry in
> >> m_waitingResources is cheap enough to handle that (admittedly unlikely)
> >> case, but combine that with 100 syncevo-dbus-helper processes and we
> >> have a problem.
> >
> > Speaking of more than one entry in m_waitingResources: does the current
> > PriorityCompare really implement a strict weak ordering, as required by
> > std::set, the storage used for m_waitingResource?
> >
> > I don't think it does, because two different resources with the same
> > priority will return false regardless which way you compare them, and
> > that despite not being equivalent. I bet that as a result, you currently
> > cannot store two resources with the same priority in m_waitingResource.
> > Even if you could, what would be the relative order? Everything else
> > being equal, we want first-come-first-serve.
> >
>
> oops, you're right. That should have been a multiset.
>
> Here is a small test I'd written to make sure the ordering was correct.
... but is it guaranteed to be correct, or does it just happen to work
as intended for your current implementation of std::multiset? I don't
see anything in the SGI STL documentation that says that a multiset
preserves the order in which elements are added.
The Linux man page says "Iteration is done in ascending order according
to the keys." That leaves it undefined what the order is for equal keys.
> > Another concern is that the outcome of PriorityCompare depends on being
> > able to lock shared pointers. If a resource goes away, then the
> > invariants for the set might get violated (comparison no longer returns
> > the same results as before). I don't want to find out how std::set deals
> > with that...
> >
>
> I believe I'm handling the case where the weak pointer goes away
> already.
Your code does, but does std::multiset? It's data structure is based on
the result of the comparison between two elements. If that comparison
suddenly returns different results, some invariants inside std::multiset
get violated. All hell might break loose.
You're absolutely right. Item 22 in "Effective STL" explains just this
situation. Thanks for catching and explaining that.
> > In a nutshell, I think we need to go back to the original
std::list and
> > its brute-force insertion sort. Agreed?
> >
>
> I found it a bit nicer to use the comparison class but if you're not
> convinced feel free to revert that.
It's nicer, but I am not convinced that it is correct.
And you're right.
Another aspect is that m_waitingResources now mixes connections and
sessions. That wasn't the case before. A connection created a session
which then got scheduled, instead of being scheduled like one. There is
a reason for that: suppose there is a running connection for client A.
The client gives up on that connection and starts anew. Is that new
connection allowed to start? If not, it will be blocked by the stale
connection that it is meant to replace.
Like I said the Connection code is incomplete and this is because I
wasn't convinced I was taking the right approach. In hindsight I would
have started a connection immediately. Once the session needed to be
created by the Connection the helper would create a Session object,
set up the one-to-one dbus interface and wait for the Server to create
a SessionResource then place it in the queue. At this point we'd be in
the same situation as if only a Session had been requested.
So we have a pretty complex logic now: allow multiple
ConnectionResources to be active, but prevent running multiple syncs.
The above scheme would remove the need to have the wait queue and
active resource list hold anything other than Sessions.
Speaking of running a sync with a ConnectionResource: where is the
SessionResource that exposes the sync to D-Bus clients? The old
Connection class had
boost::shared_ptr<Session> m_session
That pointer still exists in the helper, but the counterpart in
syncevo-dbus-server is missing. Have you thought about this?
Again, the above scheme would lend itself to using m_session easily.
I'm wondering whether the old approach would be easier:
* keep the Connection logic inside syncevo-dbus-server
* schedule a normal SessionResource there
* use it once it is ready
One downside of this is that Connection needs to parse the incoming
message, for which it must instantiate libsynthesis. It's linked to
anyway via libsyncevolution, but not used elsewhere, so long term we
could get rid of it in syncevo-dbus-server by refactoring the libs if it
wasn't for this one aspect.
I believe the scheme I proposed above would lead itself to achieving this goal.
Cheers,
Chris
12:54 p.m.
2012/3/16 Patrick Ohly <patrick.ohly(a)intel.com>:
On Thu, 2012-03-15 at 14:43 +0100, Patrick Ohly wrote:
> I still think resources should be added to m_waitingResources before
> construction is complete. Activating the object and notifying the
> caller
> can wait until the resource is really usable (= helper ready).
And one more observation: running the helper should get delayed until it
is really needed, which probably means until the resource is ready to
become active.
Not really, I am afraid - there are methods that does not need Session
to be active. GetStatus for example. Or maybe I did not understand
you.
Imagine having 100 pending session requests. An entry in
m_waitingResources is cheap enough to handle that (admittedly unlikely)
case, but combine that with 100 syncevo-dbus-helper processes and we
have a problem.
--
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.
1:24 p.m.
On Fri, 2012-03-16 at 13:54 +0100, Krzesimir Nowak wrote:
2012/3/16 Patrick Ohly <patrick.ohly(a)intel.com>:
> On Thu, 2012-03-15 at 14:43 +0100, Patrick Ohly wrote:
>> I still think resources should be added to m_waitingResources before
>> construction is complete. Activating the object and notifying the
>> caller
>> can wait until the resource is really usable (= helper ready).
>
> And one more observation: running the helper should get delayed until it
> is really needed, which probably means until the resource is ready to
> become active.
Not really, I am afraid - there are methods that does not need Session
to be active. GetStatus for example. Or maybe I did not understand
you.
Who says that GetStatus() has to be implemented in the helper? ;-)
Putting its implementation into the syncevo-dbus-server would allow
delaying the helper startup and make the server/helper API smaller. A
downside might be that the server replies with slightly stale data, but
that's not a bug. It'll correct itself as soon as the server gets the
next status update from the helper, which then triggers a statusChanged
signal in the server.
This is the same question that I just raised for connection: where do we
put which part of the implementation?
--
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.
12:46 p.m.
2012/3/15 Patrick Ohly <patrick.ohly(a)intel.com>:
More observations/questions.
ConnectionResource and SessionResource share a lot of code: init(),
onQuit(), onFailure(), ... - basically the whole startup of the helper
seems to have been copied. Shouldn't that common logic be in the
Resource base class?
Maybe some of the code could be common, but not that whole. Proxy
handling is different for Session and Connection. Different
environment variables are used. onQuit and onFailure can be easily
moved. I will do it, but maybe bit later when more pressing issues are
solved.
I also wonder about object ownership while the resource is still in the
process of getting constructed.
void SessionResource::createSessionResource(const Callback_t &callback,
Server &server,
const std::string &peerDeviceID,
const std::string &configName,
const std::string &session,
const std::vector<std::string>
&flags)
{
std::auto_ptr<SessionResource> resource(new SessionResource(server,
peerDeviceID,
configName,
session,
flags));
resource->init(callback);
// init did not throw an exception, so we guess that child was
// spawned successfully. thus we release the auto_ptr, so it will
// not delete the resource.
resource.release();
}
"we guess" is not encouraging when it comes to memory ownership ;-} Do
we know for sure? If createSessionResource() transfers owership to
init(), then it should do so explicitly.
Ah, well, exception was not thrown at this point so I stated that all
went fine. The situation when resource could leak would be when there
was some failure during DBus communication between sync-helper and
server e.g. sync-helper crashed. I know - it is still ugly, so I will
change that it will return a shared_ptr to Session, but it will have
to be documented that such Session is not very usable at this point.
To use such Session (or Connection) server has to wait until callback
is called.
Next, SessionResource::init(). It connects the resource to ForkExec
signals, using a plain pointer to the resource, without the
boost::signal2 track mechanism that I had suggested.
With creation of shared_ptr above it should be easy to solve now.
Who owns the SessionResource at that point? Who is going to free it
when
something goes wrong before (eventually) addResource() is called? As far
as I can tell, the instance will simply leak.
Another situation is when the D-Bus client quits while its requested
session still gets constructed. The Client instance of the client will
get destructed. But the SessionResource was not yet connected to it, so
the construction cannot be aborted by Server.
I still think resources should be added to m_waitingResources before
construction is complete. Activating the object and notifying the caller
can wait until the resource is really usable (= helper ready).
--
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.
12:11 p.m.
2012/3/15 Patrick Ohly <patrick.ohly(a)intel.com>:
On Thu, 2012-03-15 at 10:34 +0100, Krzesimir Nowak wrote:
> 2012/3/14 Patrick Ohly <patrick.ohly(a)intel.com>:
> > Hello Chris!
> >
> > You added a callback parameter to addResource() (formerly known as
> > enqueue()) and related functions. There's no documentation for that in
> > server.h/cpp. What is it meant to be used for, what kind of constraints
> > does it have, etc.? In other words, please document it for me... ;-)
> >
>
> That is my doing again. Will document it. The callback was added,
> because addResource calls checkQueue() which calls SetActive, which is
> asynchronous. The callback is called when after all SetActive calls
> are finished or, if there was no call to SetActive, at the end of
> checkQueue(). In short - addResource, removeResource are asynchronous
> too, so given callback is called when they are finished.
Instead of just calling the parameter "callback" it would be good to use
a name which is related to the time when the callback is invoked.
Well, maybe - the callback is called after in fact adding or removing
resource is finished. I have no idea what other name it should have. I
will have to just document it when it may be called.
The goal of the callback is to return the final result of a D-Bus
method
invocation to the D-Bus client, right?
Not really - is more general. It may be your main use, but not
necessarily. I mean - SetActive returns nothing, but some code might
need waiting for those calls to be finished.
Why does that have to wait for the completion of SetActive? Can't
we
tell the D-Bus client that its StartSession() was successful, then later
emit the right "status changed" signal?
If you don't care about when SetActive finishes and want to proceed
then passing nullCb should be enough.
The code is here:
// If not we activate the resource and place it in the active resource list.
if (canRun) {
m_activeResources.push_back(waitingResource);
// If this is a session, we set active and emit sessionChanged to
clients.
boost::shared_ptr<SessionResource> session =
boost::dynamic_pointer_cast<SessionResource>(wq_iter->lock());
if (session) {
++(*counter);
session->setActiveAsync(true, boost::bind(&Server::setActiveCb,
this, session, counter, callback));
}
m_waitingResources.erase(wq_iter++);
}
That seems unnecessarily complex to me, but perhaps I am missing something.
That was the case that if two sessions are going to be active then we
don't want to have our callback called twice.
Of course, if you feel that all of this is not necessary and
overcomplicated, then I will just remove it.
Previously calling SetActive had immediate effect, so we could call
e.g. SetNamedConfig right after it. Now it involves DBus, so maybe
some code will have to wait until the session is really active. I
probably have written this code when I was tracking that race
condition in GIO GDBus.
--
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.
1:16 p.m.
On Fri, 2012-03-16 at 13:11 +0100, Krzesimir Nowak wrote:
2012/3/15 Patrick Ohly <patrick.ohly(a)intel.com>:
> On Thu, 2012-03-15 at 10:34 +0100, Krzesimir Nowak wrote:
>> 2012/3/14 Patrick Ohly <patrick.ohly(a)intel.com>:
>> > Hello Chris!
>> >
>> > You added a callback parameter to addResource() (formerly known as
>> > enqueue()) and related functions. There's no documentation for that in
>> > server.h/cpp. What is it meant to be used for, what kind of constraints
>> > does it have, etc.? In other words, please document it for me... ;-)
>> >
>>
>> That is my doing again. Will document it. The callback was added,
>> because addResource calls checkQueue() which calls SetActive, which is
>> asynchronous. The callback is called when after all SetActive calls
>> are finished or, if there was no call to SetActive, at the end of
>> checkQueue(). In short - addResource, removeResource are asynchronous
>> too, so given callback is called when they are finished.
>
> Instead of just calling the parameter "callback" it would be good to use
> a name which is related to the time when the callback is invoked.
Well, maybe - the callback is called after in fact adding or removing
resource is finished. I have no idea what other name it should have. I
will have to just document it when it may be called.
In SessionResource for createSessionResource(), init(), and
onSessionConnect() a better name would be "constructionCompleted". Note
my previous comments about the lack of a "constructionFailed".
In Server, in checkQueue() and addResource() use the same kind of
callback, but I don't understand what kind of guarantee is made about
it. Something like "activatedOrFailed"?
It's not the same as SessionResource::setActiveAsyncCb's "callback",
although it has the same name and signature.
> The code is here:
> // If not we activate the resource and place it in the active resource
list.
> if (canRun) {
> m_activeResources.push_back(waitingResource);
> // If this is a session, we set active and emit sessionChanged to
clients.
> boost::shared_ptr<SessionResource> session =
>
boost::dynamic_pointer_cast<SessionResource>(wq_iter->lock());
> if (session) {
> ++(*counter);
> session->setActiveAsync(true,
boost::bind(&Server::setActiveCb, this, session, counter, callback));
> }
> m_waitingResources.erase(wq_iter++);
> }
>
> That seems unnecessarily complex to me, but perhaps I am missing something.
That was the case that if two sessions are going to be active then we
don't want to have our callback called twice.
If I knew what the callback stands for, then perhaps I would understand
the code :-/
--
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.
10:08 a.m.
On Thu, 2012-03-15 at 10:34 +0100, Krzesimir Nowak wrote:
2012/3/14 Patrick Ohly <patrick.ohly(a)intel.com>:
> Hello Chris!
>
> You added a callback parameter to addResource() (formerly known as
> enqueue()) and related functions. There's no documentation for that in
> server.h/cpp. What is it meant to be used for, what kind of constraints
> does it have, etc.? In other words, please document it for me... ;-)
>
That is my doing again. Will document it. The callback was added,
because addResource calls checkQueue() which calls SetActive, which is
asynchronous. The callback is called when after all SetActive calls
are finished or, if there was no call to SetActive, at the end of
checkQueue(). In short - addResource, removeResource are asynchronous
too, so given callback is called when they are finished.
Any progress on this, and the related questions that I raised in this
thread?
I'm currently working on auto-sync, but it needs to create a sync
session and for that I need to know how we are dealing with the
addResource() call.
--
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.
3452
days inactive
3513
days old
syncevolution@synevolution.org
56 comments
4 participants
participants (4)
-
Chris Kühl -
Justus-bulk＠Piater.name -
Krzesimir Nowak -
Patrick Ohly