Code review comment for lp:~carla-sella/ubuntu-rssreader-app/ubuntu-rssreader-test-add-view-feeds

I ended up rerunning the test several times. Here are all the runs:
** http://91.189.93.70:8080/job/generic-mediumtests/55/
 - http://91.189.93.70:8080/job/generic-mediumtests/56/
** http://91.189.93.70:8080/job/generic-mediumtests/61/
 - http://91.189.93.70:8080/job/generic-mediumtests/63/
** http://91.189.93.70:8080/job/generic-mediumtests/64/
 - http://91.189.93.70:8080/job/generic-mediumtests/65/

The tests marked with "**" had a failure, not always the same one. While we could say the tests pass, they'll likely end up blocking future MPs when they occasionally fail and the tests themselves will be ignored. (By the way, this is a common problem with the UI tests I've worked with). The tests often behave differently in jenkins because we're using VMs which don't have graphical acceleration. As a result the graphics rendering tends to go slower then autopilot. Also, it's difficult to get the tests to run perfectly, but if we can get them to run better, that should be good.

I did notice an issue with your tests that needs to be corrected and should help. For example:
317 + #click Canonical feed item
318 + canonical_feed = self.main_window.get_canonical_feed()
319 + #does it exist?
320 + lambda: self.assertThat(canonical_feed, NotEquals(None))
321 + #let's click it
322 + self.mouse.move_to_object(canonical_feed)

The "lambda: self.assertThat" isn't doing what you intend. The lambda is not actually running the assert, it's just being defined but never used. What you want to do is this:
317 + #click Canonical feed item
318 + canonical_feed = lambda: self.main_window.get_canonical_feed()
319 + #does it exist?
320 + self.assertThat(canonical_feed, Eventually(NotEquals(None)))
321 + #let's click it
322 + self.mouse.move_to_object(canonical_feed())

Now, 'canonical_feed' is assigned to the lambda, making it a function and not an object. It can be used in the assertThat along with the 'Eventually' matcher to perform a check and wait on the status of the UI element. 'Eventually' requires a callable, which is why 'canonical_feed' is created as a lambda, and will keep retrying the assert until it becomes True or times out (default is 10 seconds). This technique can also be used to eliminate

Also, because 'canonical_feed' is now a function, we have to treat it like a function in the call to 'move_to_object' and add the '()'.

And to resolve the error seen in runs 56 and 61, you could use:
        #type feed in input field
        new_Feed_Url = lambda: self.main_window.get_new_event_name_input_box()
        self.assertThat(new_Feed_Url, Eventually(NotEquals(None)))
        self.pointing_device.click_object(new_Feed_Url())
        self.keyboard.type("http://www.canonical.com/rss.xml")

The Eventually matcher can also be used to eliminate the sleep call in this code:
348 + #check that feed is updated, not empty
349 + canonical_feed_before = self.main_window.get_individual_feed().text
350 + self.assertThat(canonical_feed_before, Eventually(Equals("http://www.canonical.com/rss.xml")))
351 + #had to put this sleep here otherwise test fails
352 + sleep(2)
353 + self.assertThat(canonical_feed_before, NotEquals(self.main_window.get_individual_feed().text))
354 + #verify feed has been added
355 + canonical_feed = self.main_window.get_individual_feed().text
356 + self.assertThat(canonical_feed, Equals("Canonical"))

can be done with:
    def _canonical_feed(self):
        try:
            return self.main_window.get_individual_feed().text
        except StateNotFoundError:
            return None

    def insert_canonical_feed(self):
        ...
        #check that feed is updated, not empty
        canonical_feed_before = "http://www.canonical.com/rss.xml"
        self.assertThat(self._canonical_feed, Eventually(Equals(canonical_feed_before)))
        #verify the feed contents changes
        self.assertThat(self._canonical_feed, Eventually(NotEquals(canonical_feed_before)))
        #verify feed has been added
        self.assertThat(self._canonical_feed, Eventually(Equals("Canonical")))

This requires a different autopilot test 'trick'. Using a lambda to return the feed text was not sufficient. When I was testing, I was getting StateNotFoundError exceptions. This happens when the autopilot Eventually check loop just happens to catch the UI element in a transition state. Possibly the old text had just been removed but not yet replaced with the new text. As a result autopilot can't retrieve the text value because it didn't exist at that specific moment and threw the StateNotFoundError. To solve this, I just created a function, "_canonical_feed" which will return the value of the text field if it is available, or None when it's not. This provides the assert calls a value to test instead of an exception.

This is all just my recommendation, but I hope this helps. Please feel free to ping me on IRC if you have any questions (I'm 'fginther'). If the ubuntu-rssreader-app team would prefer to get these tests in sooner, they are free to approve this MP.