Add pyauto tests for the New Tab page.

Add pyauto tests for the New Tab page.
BUG=63816
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=68268

[kkania, 2010-11-23 22:49:18]

these are from emails; I changed a lot of these to fit with with the slightly
changed API, to fix some flakiness problems, and to make things simpler/cleaner

[Nirnimesh, 2010-11-24 09:58:23]

Minor comments only.

Great to see these going in.
Thanks for the heavylifting.

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.py
File chrome/test/functional/ntp.py (right):

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:38: def _NTPContainsThumbnail(self,
check_thumbnail):
docstring please

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:98: for i in range(len(self.PAGES)):
You don't really need |i|

for page in self.PAGES:
  self.AppendTab(pyauto.GURL(page['url']))
  ...

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:103: for i in range(len(thumbnails)):
ditto

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:104: self.assertEquals(thumbnails[i],
self.GetNTPThumbnails()[0])
this line looks like a tautology
Did you mean to use PAGES?

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:115: self.assertEquals([],
self.GetNTPThumbnails())
assertFalse

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:137: 
please assert that |thumbnails| is not empty

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:138: self.RestartBrowser(False)
use named arg. ie clear_profile=False

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:142: """Tests restoring all removed thumnails"""
thumbnails

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:152: self.RemoveNTPDefaultThumbnails()
maybe this line and line 143 should be removed from this test

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:169: """Testis that once a thumnail is pinned it
never moves"""
Testis?!!

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:176: self.assertEqual(thumbnails,
self.GetNTPThumbnails())
won't self.PAGES[1]['url']) show up in GetNTPThumbnails()?

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:179: """Tests that once a page title changes, the
thumbnail title changes also"""
s/also/too/

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:217: """Tests closing multiple tabs"""
Please elaborate. Should mention that it checks the recently closed list

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:292: self.assertEqual([],
self.GetNTPRecentlyClosed())
assertFalse

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:294: self.assertEqual([],
self.GetNTPRecentlyClosed())
assertFalse

http://codereview.chromium.org/5255005/diff/2001/chrome/test/pyautolib/pyauto...
File chrome/test/pyautolib/pyautolib.h (right):

http://codereview.chromium.org/5255005/diff/2001/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyautolib.h:107: bool CloseBrowserWindow(int
window_index);
Thanks for adding this

[kkania, 2010-11-24 17:40:35]

Fixed almost all comments. A few had questions about. Also, what is our stance
about the comments after the def test...(self):?  Are we forcing them to be one
line long?

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.py
File chrome/test/functional/ntp.py (right):

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:38: def _NTPContainsThumbnail(self,
check_thumbnail):
On 2010/11/24 09:58:23, Nirnimesh wrote:
> docstring please

Done.

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:98: for i in range(len(self.PAGES)):
On 2010/11/24 09:58:23, Nirnimesh wrote:
> You don't really need |i|
> 
> for page in self.PAGES:
>   self.AppendTab(pyauto.GURL(page['url']))
>   ...

Done.

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:103: for i in range(len(thumbnails)):
On 2010/11/24 09:58:23, Nirnimesh wrote:
> ditto

Done.

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:104: self.assertEquals(thumbnails[i],
self.GetNTPThumbnails()[0])
On 2010/11/24 09:58:23, Nirnimesh wrote:
> this line looks like a tautology
> Did you mean to use PAGES?

I wanted to make sure that the thumbnails shift to the front when the first
thumbnail is removed

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:115: self.assertEquals([],
self.GetNTPThumbnails())
On 2010/11/24 09:58:23, Nirnimesh wrote:
> assertFalse

I was wondering about this. assertEquals([], ...) has the nice property that
when it fails it will spit out what the thumbnails were, which is nice for
debugging. I guess it is not worth it though. Done.

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:137: 
On 2010/11/24 09:58:23, Nirnimesh wrote:
> please assert that |thumbnails| is not empty

I am interested in your thoughts about this. I was thinking that this test
should assume that the move function works, since it is tested by
testMoveThumbnailBasic. This makes the test simpler and more focused.
Assumptions like this are made in several places in this and other files.

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:138: self.RestartBrowser(False)
On 2010/11/24 09:58:23, Nirnimesh wrote:
> use named arg. ie clear_profile=False

Done.

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:142: """Tests restoring all removed thumnails"""
On 2010/11/24 09:58:23, Nirnimesh wrote:
> thumbnails

Done.

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:152: self.RemoveNTPDefaultThumbnails()
On 2010/11/24 09:58:23, Nirnimesh wrote:
> maybe this line and line 143 should be removed from this test

That's a good idea, done.

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:169: """Testis that once a thumnail is pinned it
never moves"""
On 2010/11/24 09:58:23, Nirnimesh wrote:
> Testis?!!

Done.

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:176: self.assertEqual(thumbnails,
self.GetNTPThumbnails())
On 2010/11/24 09:58:23, Nirnimesh wrote:
> won't self.PAGES[1]['url']) show up in GetNTPThumbnails()?

yes, but all the pages have already been added to the NTP. Going to page[1]
again triggers it to have a higher rank than page[0]. However, since page[0] is
pinned, it should stay put.

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:179: """Tests that once a page title changes, the
thumbnail title changes also"""
On 2010/11/24 09:58:23, Nirnimesh wrote:
> s/also/too/

Done.

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:217: """Tests closing multiple tabs"""
On 2010/11/24 09:58:23, Nirnimesh wrote:
> Please elaborate. Should mention that it checks the recently closed list

Done.

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:292: self.assertEqual([],
self.GetNTPRecentlyClosed())
On 2010/11/24 09:58:23, Nirnimesh wrote:
> assertFalse

Done.

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:294: self.assertEqual([],
self.GetNTPRecentlyClosed())
On 2010/11/24 09:58:23, Nirnimesh wrote:
> assertFalse

Done.

http://codereview.chromium.org/5255005/diff/2001/chrome/test/pyautolib/pyauto...
File chrome/test/pyautolib/pyautolib.h (right):

http://codereview.chromium.org/5255005/diff/2001/chrome/test/pyautolib/pyauto...
chrome/test/pyautolib/pyautolib.h:107: bool CloseBrowserWindow(int
window_index);
On 2010/11/24 09:58:23, Nirnimesh wrote:
> Thanks for adding this

Done.

[Nirnimesh, 2010-11-24 20:38:11]

LGTM

Ideally every function including test methods' docstring should have a one-line
summary followed by detailed desc if needed.

The one-line summary is especially good for test methods since it gets printed
out when the test is run, and gives an idea of what the test did when looking at
the buildbot output. But I don't enforce it strongly for test methods.
However, I do enforce it for classes and helper functions.

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.py
File chrome/test/functional/ntp.py (right):

http://codereview.chromium.org/5255005/diff/2001/chrome/test/functional/ntp.p...
chrome/test/functional/ntp.py:137: 
On 2010/11/24 17:40:35, kkania wrote:
> On 2010/11/24 09:58:23, Nirnimesh wrote:
> > please assert that |thumbnails| is not empty
> 
> I am interested in your thoughts about this. I was thinking that this test
> should assume that the move function works, since it is tested by
> testMoveThumbnailBasic. This makes the test simpler and more focused.
> Assumptions like this are made in several places in this and other files.

Ok