4:58 a.m.
Hello!
There's currently a problem with the D-Bus API testing when the
TestSessionAPIsReal test suite is run so that both of its tests run real
syncs.
I've looked into this in more detail and found that it is because the
first test sets up a D-Bus signal handle for StatusChanged which calls
loop.quit() when status == "done". Then our server fires that state
*multiple* times for the first sync. The first signal finishes the first
test. The second signal the second, although that sync isn't done yet.
Debugging this was hard because the reasons why loop.quit() were called
were also recorded per test, so they weren't visible in the second test.
I'm changing that.
I'm also still looking at why the "done" signal is sent more than once.
I think that shouldn't happen. Suggestions welcome, in case that I don't
finish this today (not unlikely, it's late and I have a choir rehearsal
for a performance on Sunday...).
--
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:03 a.m.
On Wed, 2009-11-18 at 17:58 +0000, Patrick Ohly wrote:
I'm also still looking at why the "done" signal is sent
more than once.
I think that shouldn't happen. Suggestions welcome, in case that I don't
finish this today (not unlikely, it's late and I have a choir rehearsal
for a performance on Sunday...).
I still haven't looked for the reason for multiple "done" signals. The
test-dbus.py changes which reveal this in many tests are in the "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.
8:11 p.m.
On Wed, 2009-11-18 at 18:58 +0100, Patrick Ohly wrote:
I'm also still looking at why the "done" signal is sent
more than once.
I think that shouldn't happen.
And it is really only sent once. The problem was in the test-dbus.py
script. Here's the fix and a more detailed explanation:
commit ac1c3c032ccae55f8697f75341f7becd2c3adfb0
Author: Patrick Ohly <patrick.ohly(a)intel.com>
Date: Wed Nov 18 22:49:48 2009 +0100
D-Bus testing: only react to D-Bus signals while the test runs
Registered Python D-Bus signal handlers cannot be removed.
This had the undesired effect that when running multiple tests,
old signal handlers where still active and might have undesired
effects, like calling loop.quit() or recording events multiple
times (because both the old and new handler are invoked).
To solve this problem, DBusUtil.runTest() sets self.running = True
while the test is active. All signal handlers must check this
flag and not do anything if it is False.
Debugging such problems was harder than necessary because recording
the "quit_events" inside DBusUtil instances had the effect that the
records of previous signal handlers where not visible in the ensuing,
failing test.
Therefore this patch also turns the DBusUtil instance variables into
class variables which are shared by the tests. As a side effect, it is
no longer necessary to call DBusUtil.__init__. Instead the variables
are reset in runTest().
Yongsheng, I pushed this and a fix for sending SYNC_DONE at the end of
Session::run() into "pohly". Please review.
--
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:11 p.m.
And it is really only sent once. The problem was in the test-dbus.py
script. Here's the fix and a more detailed explanation:
When sync is done,
signals of statusChanged and progressChanged should
be never sent. If has, I think it should be bugs of dbus server.
I doubt why we need this kind of checking.
Yongsheng, I pushed this and a fix for sending SYNC_DONE at the end
of
Session::run() into "pohly". Please review.
It's ok. I'm refining
patches for test-dbus.py according to your comments.
To avoid conflicts, so I'll do below issues:
removing redundant clean configs, setupFiles, add test for GetReports with reference
directory tree.
Cheers,
Yongsheng
> -----Original Message-----
> From: syncevolution-bounces(a)syncevolution.org
> [mailto:syncevolution-bounces@syncevolution.org] On Behalf Of Patrick Ohly
> Sent: Thursday, November 19, 2009 5:11 PM
> To: SyncEvolution
> Subject: Re: [SyncEvolution] D-Bus Testing + TestSessionAPIsReal +
> StatusChanged "done" multiple times
>
> On Wed, 2009-11-18 at 18:58 +0100, Patrick Ohly wrote:
> > I'm also still looking at why the "done" signal is sent more than
once.
> > I think that shouldn't happen.
>
And it is really only sent once. The problem was in the test-dbus.py
script. Here's the fix and a more detailed explanation:
>
> commit ac1c3c032ccae55f8697f75341f7becd2c3adfb0
> Author: Patrick Ohly <patrick.ohly(a)intel.com>
> Date: Wed Nov 18 22:49:48 2009 +0100
>
> D-Bus testing: only react to D-Bus signals while the test runs
>
> Registered Python D-Bus signal handlers cannot be removed.
> This had the undesired effect that when running multiple tests,
> old signal handlers where still active and might have undesired
> effects, like calling loop.quit() or recording events multiple
> times (because both the old and new handler are invoked).
>
> To solve this problem, DBusUtil.runTest() sets self.running = True
> while the test is active. All signal handlers must check this
> flag and not do anything if it is False.
>
> Debugging such problems was harder than necessary because recording
> the "quit_events" inside DBusUtil instances had the effect that the
> records of previous signal handlers where not visible in the ensuing,
> failing test.
>
> Therefore this patch also turns the DBusUtil instance variables into
> class variables which are shared by the tests. As a side effect, it is
> no longer necessary to call DBusUtil.__init__. Instead the variables
> are reset in runTest().
>
Yongsheng, I pushed this and a fix for sending SYNC_DONE at the end
of
Session::run() into "pohly". Please review.
>
>
> --
> 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
7:05 p.m.
On Fri, 2009-11-20 at 03:11 +0000, Zhu, Yongsheng wrote:
> And it is really only sent once. The problem was in the
test-dbus.py
> script. Here's the fix and a more detailed explanation:
When sync is done, signals of statusChanged and progressChanged should
be never sent. If has, I think it should be bugs of dbus server.
I doubt why we need this kind of checking.
I'm not sure I understand. Because signals should not be sent in certain
situations, we don't need to check this? Isn't the conclusion exactly
the opposite? Because the test script needs to work with a potentially
buggy server, it must do as much checking as possible and be prepared to
report server failures gracefully.
> Yongsheng, I pushed this and a fix for sending SYNC_DONE at the
end of
> Session::run() into "pohly". Please review.
It's ok.
I merged everything related to this into master, including the fix for
the StatusChanged "idle" problem. There are more patches pending in the
"pohly" branch, please let me know what you consider ready for 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:43 p.m.
I'm not sure I understand. Because signals should not be sent in
certain
situations, we don't need to check this? Isn't the conclusion exactly
the opposite? Because the test script needs to work with a potentially
buggy server, it must do as much checking as possible and be prepared to
report server failures gracefully.
For a buggy server, it is possible.
But I think a correct server should not. So my suggestion is that we should
add at least one case to test this scenario: these signals should not be sent
out. This could make sure that our server at least has no this kind of error.
I merged everything related to this into master, including the fix
for
the StatusChanged "idle" problem. There are more patches pending in the
"pohly" branch, please let me know what you consider ready for merging.
One question, in the patch 'syncevo-dbus-server: StatusChanged "idle"
was not sent',
+ loop.run()
+ expected = ["session " + self.sessionpath + " done",
+ "session " + sessionpath + " idle",
+ "session " + sessionpath + " ready"]
+ expected.sort()
+ DBusUtil.quit_events.sort()
+ self.failUnlessEqual(DBusUtil.quit_events, expected)
I find expected and quit_events are all sorted. Don't we guarantee the sequence?
'progress' testing needs check the sequence problem.
Cheers,
Yongsheng
> -----Original Message-----
> From: Ohly, Patrick
> Sent: Friday, November 20, 2009 4:06 PM
> To: Zhu, Yongsheng
> Cc: SyncEvolution
> Subject: RE: [SyncEvolution] D-Bus Testing + TestSessionAPIsReal +
> StatusChanged "done" multiple times
>
> On Fri, 2009-11-20 at 03:11 +0000, Zhu, Yongsheng wrote:
> > > And it is really only sent once. The problem was in the test-dbus.py
> > > script. Here's the fix and a more detailed explanation:
> > When sync is done, signals of statusChanged and progressChanged should
> > be never sent. If has, I think it should be bugs of dbus server.
> > I doubt why we need this kind of checking.
>
I'm not sure I understand. Because signals should not be sent in
certain
situations, we don't need to check this? Isn't the conclusion exactly
the opposite? Because the test script needs to work with a potentially
buggy server, it must do as much checking as possible and be prepared to
report server failures gracefully.
>
> > > Yongsheng, I pushed this and a fix for sending SYNC_DONE at the end of
> > > Session::run() into "pohly". Please review.
> > It's ok.
>
I merged everything related to this into master, including the fix
for
the StatusChanged "idle" problem. There are more patches pending in the
"pohly" branch, please let me know what you consider ready for 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.
>
8:24 p.m.
On Fri, 2009-11-20 at 08:43 +0000, Zhu, Yongsheng wrote:
> I'm not sure I understand. Because signals should not be
sent in certain
> situations, we don't need to check this? Isn't the conclusion exactly
> the opposite? Because the test script needs to work with a potentially
> buggy server, it must do as much checking as possible and be prepared to
> report server failures gracefully.
For a buggy server, it is possible.
But I think a correct server should not. So my suggestion is that we should
add at least one case to test this scenario: these signals should not be sent
out. This could make sure that our server at least has no this kind of error.
Agreed. The problem is: how long do we keep checking that these signals
are not sent? Whatever duration we choose, there's always the
possibility that the faulty signal is sent one second after we stop
watching ;-}
> I merged everything related to this into master, including the
fix for
> the StatusChanged "idle" problem. There are more patches pending in the
> "pohly" branch, please let me know what you consider ready for merging.
One question, in the patch 'syncevo-dbus-server: StatusChanged "idle" was
not sent',
+ loop.run()
+ expected = ["session " + self.sessionpath + " done",
+ "session " + sessionpath + " idle",
+ "session " + sessionpath + " ready"]
+ expected.sort()
+ DBusUtil.quit_events.sort()
+ self.failUnlessEqual(DBusUtil.quit_events, expected)
I find expected and quit_events are all sorted. Don't we guarantee the sequence?
Not between Session.StatusChanged and Server.SessionChanged, which is
tested here.
'progress' testing needs check the sequence problem.
In this case, the semantic of the 'progress' events imply a certain
order. There are lots of TODOs in the test program related to
'progress'. They are recorded, but there are no checks that they are
reasonable.
--
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:40 p.m.
Agreed. The problem is: how long do we keep checking that these
signals
are not sent? Whatever duration we choose, there's always the
possibility that the faulty signal is sent one second after we stop
watching ;-}
Yes, it's a real problem. Just with an experience data. I think a duration of a
new sync process
is enough. Due to previous profiling data, 5~6s is an option.
Cheers,
Yongsheng
> -----Original Message-----
> From: Ohly, Patrick
> Sent: Friday, November 20, 2009 5:25 PM
> To: Zhu, Yongsheng
> Cc: SyncEvolution
> Subject: RE: [SyncEvolution] D-Bus Testing + TestSessionAPIsReal +
> StatusChanged "done" multiple times
>
> On Fri, 2009-11-20 at 08:43 +0000, Zhu, Yongsheng wrote:
> > > I'm not sure I understand. Because signals should not be sent in
certain
> > > situations, we don't need to check this? Isn't the conclusion
exactly
> > > the opposite? Because the test script needs to work with a potentially
> > > buggy server, it must do as much checking as possible and be prepared to
> > > report server failures gracefully.
> > For a buggy server, it is possible.
> > But I think a correct server should not. So my suggestion is that we should
> > add at least one case to test this scenario: these signals should not be sent
> > out. This could make sure that our server at least has no this kind of error.
>
Agreed. The problem is: how long do we keep checking that these
signals
are not sent? Whatever duration we choose, there's always the
possibility that the faulty signal is sent one second after we stop
watching ;-}
>
> > > I merged everything related to this into master, including the fix for
> > > the StatusChanged "idle" problem. There are more patches pending
in the
> > > "pohly" branch, please let me know what you consider ready for
merging.
> > One question, in the patch 'syncevo-dbus-server: StatusChanged
"idle" was not
> sent',
> > + loop.run()
> > + expected = ["session " + self.sessionpath + "
done",
> > + "session " + sessionpath + " idle",
> > + "session " + sessionpath + " ready"]
> > + expected.sort()
> > + DBusUtil.quit_events.sort()
> > + self.failUnlessEqual(DBusUtil.quit_events, expected)
> > I find expected and quit_events are all sorted. Don't we guarantee the
> sequence?
>
> Not between Session.StatusChanged and Server.SessionChanged, which is
> tested here.
>
> > 'progress' testing needs check the sequence problem.
>
> In this case, the semantic of the 'progress' events imply a certain
> order. There are lots of TODOs in the test program related to
> 'progress'. They are recorded, but there are no checks that they are
> reasonable.
>
>
>
> --
> 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.
>
4682
days inactive
4687
days old
syncevolution@synevolution.org
7 comments
2 participants
participants (2)
-
Patrick Ohly -
Zhu, Yongsheng