Merge lp:~dobey/ubuntuone-client/delay-syncdaemon into lp:ubuntuone-client

[dobey]

Fix the applet to avoid starting syncdaemon before getting the token

[Eric Casteleijn (thisfred)]

looks good, tests pass.

review: Approve

[Natalia Bidart (nataliabidart)]

Approving. We need this!

review: Approve

[Natalia Bidart (nataliabidart)]

> Approving. We need this!

We do need this, but since the branch doesn't provide tests, I'll have to mark it as Needs Fixing, as per what Lucio talked with Chipaca.

(11:16:39 AM) Chipaca: I want you all to know: UNTESTED CODE IS BROKEN CODE

review: Needs Fixing

[dobey (dobey)]

On Thu, 03 Dec 2009 14:20:52 -0000
Naty Bidart <email address hidden> wrote:

> Review: Needs Fixing
> > Approving. We need this!
>
> We do need this, but since the branch doesn't provide tests, I'll
> have to mark it as Needs Fixing, as per what Lucio talked with
> Chipaca.
>
> (11:16:39 AM) Chipaca: I want you all to know: UNTESTED CODE IS
> BROKEN CODE

There are no unit tests for the applet. We can't easily do unit tests
for the applet. And as it's going away soonish for Lucid, I see no
reason to spend a significant amount of time trying to get unit tests
written and working for it. Integration testing for GUI code is also
more complicated, and should be handled via normal QA processes outside
of unit/regression testing in code.

I believe we should test our code as much as *possible* when possible.
I don't think we should write applications as libraries, just so we can
have unit tests for some of the application code. We should work on
getting Mago/LDTP tests written for our applications, so that they can
be better tested in QA in a VM in a normal use environment. Hopefully
we can get a lot of that done this cycle.

[Natalia Bidart (nataliabidart)]

> On Thu, 03 Dec 2009 14:20:52 -0000
> Naty Bidart <email address hidden> wrote:
>
> > Review: Needs Fixing
> > > Approving. We need this!
> >
> > We do need this, but since the branch doesn't provide tests, I'll
> > have to mark it as Needs Fixing, as per what Lucio talked with
> > Chipaca.
> >
> > (11:16:39 AM) Chipaca: I want you all to know: UNTESTED CODE IS
> > BROKEN CODE
>
>
> There are no unit tests for the applet. We can't easily do unit tests
> for the applet. And as it's going away soonish for Lucid, I see no
> reason to spend a significant amount of time trying to get unit tests
> written and working for it. Integration testing for GUI code is also
> more complicated, and should be handled via normal QA processes outside
> of unit/regression testing in code.
>
> I believe we should test our code as much as *possible* when possible.
> I don't think we should write applications as libraries, just so we can
> have unit tests for some of the application code. We should work on
> getting Mago/LDTP tests written for our applications, so that they can
> be better tested in QA in a VM in a normal use environment. Hopefully
> we can get a lot of that done this cycle.

I personally think that we can not release code to the ~200K users we have without ensuring correct behavior of it. This fix will go to karmic on a SRU, so we need to be really careful.

I'm not asking for a whole test suite of tests, but a single test case where the following scenarios are exercised:

 * if the applet can not get a token, syncdaemon is not started
 * if the applet can get a token, syncdaemon is started

[dobey (dobey)]

On Thu, 03 Dec 2009 17:24:42 -0000
Naty Bidart <email address hidden> wrote:

> > On Thu, 03 Dec 2009 14:20:52 -0000
> > Naty Bidart <email address hidden> wrote:
> >
> > > Review: Needs Fixing
> > > > Approving. We need this!
> > >
> > > We do need this, but since the branch doesn't provide tests, I'll
> > > have to mark it as Needs Fixing, as per what Lucio talked with
> > > Chipaca.
> > >
> > > (11:16:39 AM) Chipaca: I want you all to know: UNTESTED CODE IS
> > > BROKEN CODE
> >
> >
> > There are no unit tests for the applet. We can't easily do unit
> > tests for the applet. And as it's going away soonish for Lucid, I
> > see no reason to spend a significant amount of time trying to get
> > unit tests written and working for it. Integration testing for GUI
> > code is also more complicated, and should be handled via normal QA
> > processes outside of unit/regression testing in code.
> >
> > I believe we should test our code as much as *possible* when
> > possible. I don't think we should write applications as libraries,
> > just so we can have unit tests for some of the application code. We
> > should work on getting Mago/LDTP tests written for our
> > applications, so that they can be better tested in QA in a VM in a
> > normal use environment. Hopefully we can get a lot of that done
> > this cycle.
>
> I personally think that we can not release code to the ~200K users we
> have without ensuring correct behavior of it. This fix will go to
> karmic on a SRU, so we need to be really careful.

I agree we should ideally release perfect code that behaves perfectly.
But we shouldn't block on trying to make everything perfect. This fix
isn't critical. It might not go in an SRU. What it fixes isn't really
problematic.

> I'm not asking for a whole test suite of tests, but a single test
> case where the following scenarios are exercised:
>
> * if the applet can not get a token, syncdaemon is not started
> * if the applet can get a token, syncdaemon is started

A single test case is not trivial. To test the applet with unit tests,
we would have to rewrite the entire applet as mocked objects, and fix
the mocker to do things how we expect. This wouldn't even be testing
the applet, but instead testing the mocked objects. The applet isn't a
module. We can't import it in a test script and monkeypatch the objects
to do things differently (which is bad anyway, because then we're not
actually testing the code so much).

[Natalia Bidart (nataliabidart)]

I would need Chipaca's "go ahead" on this one, since he and Lucio agreed that all code should be submitted with tests. Sorry :-/

[Elliot Murphy (statik)]

Having tests is good. We have several kinds of things that are made out of code and could be tested. Most is library code that can be tested with unit tests. Some is web view/UI code that we've also figured out some ways of testing with unit tests. Some is web UI code that must be manually inspected (CSS) to determine whether it works correctly. Some is GUI code that runs on the desktop that we don't currently have a good way to automatically test, and so someone needs to manually run through installing and manipulating the UI to be sure it's working correctly (think of the applet, Tomboy, evolution-couchdb address book patches, bindwood firefox plugin stuff).

Insisting that we test code is obviously good. Having a single policy that attempts to apply the standard for unit testing library code to a different type of code (thin GUI code where we haven't yet figured out a good way to run automated tests) is not very sophisticated. This discussion is interesting because it highlights how we can't take a single blanket approach, but we do need to be creative and fight for quality by ensuring we find some way of testing code before putting it into Ubuntu or on the production servers.

Rodney, can you write up some directions for Rick McBride just outlining the different use cases that should be tested in the applet once this code is landed and packaged so that we can be 100% confident that it works?

As we build the U1 Control Panel, it will be interesting to try and set up some of the GUI test harness stuff that the Ubuntu Platform team has been using, but it's also important to remember that many times manual testing of GUI code is much more efficient than trying to do automated testing of GUI code.

review: Abstain

[Natalia Bidart (nataliabidart)]

Tests are being added to the QA test suite. Let's go for it!

review: Approve

[dobey (dobey)]

On Mon, 2009-12-07 at 19:08 +0000, Elliot Murphy wrote:
> Rodney, can you write up some directions for Rick McBride just
> outlining the different use cases that should be tested in the applet
> once this code is landed and packaged so that we can be 100% confident
> that it works?

For Rick:

As I mentioned in IRC, this should be tested by checking a couple
seconds after the applet is started, that the syncdaemon is NOT yet
running, when there is no token in the keyring. Easiest is to set the
test environment's config to start the applet without connecting, so
that it doesn't open up the browser bit either.

Thanks.

=== modified file 'bin/ubuntuone-client-applet'
--- bin/ubuntuone-client-applet	2009-10-22 18:56:58 +0000
+++ bin/ubuntuone-client-applet	2009-12-02 22:20:23 +0000
@@ -490,6 +490,11 @@
 
             # DBus signal handling
             self.__bus.add_signal_receiver(
+                  handler_function=self.__auth_finished,
+                  signal_name="NewCredentials",
+                  dbus_interface=DBUS_IFACE_AUTH_NAME)
+
+            self.__bus.add_signal_receiver(
                   handler_function=self.__status_changed,
                   signal_name="StatusChanged",
                   dbus_interface=DBUS_IFACE_STATUS_NAME)
@@ -507,7 +512,7 @@
                   signal_name="DownloadStarted",
                   dbus_interface=DBUS_IFACE_STATUS_NAME)
 
-            gobject.idle_add(self.__get_root)
+            self.set_visible(True)
 
       def set_from_icon_name(self, icon):
             """Handle fallbacks for setting our icon."""
@@ -810,6 +815,10 @@
 
             return False
 
+      def __auth_finished(self, *args, **kwargs):
+            """Got the oauth token, get the root and status."""
+            gobject.idle_add(self.__get_root)
+
       def __build_menus(self):
             """Create the pop-up menu items."""
             # Create the left-click menu