Add unit tests for watchlists

Add unit tests for watchlists


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

[M-A Ruel, 2009-06-17 13:39:55]

Please modify PRESUBMIT.py, add 'test.watchlists_unittest' to the unit tests to
run.

lgtm afterward

http://codereview.chromium.org/126272/diff/1003/4
File tests/watchlists_unittest.py (right):

http://codereview.chromium.org/126272/diff/1003/4#newcode19
Line 19: self.mox.StubOutWithMock(watchlists.Watchlists,
'_ContentsOfWatchlistsFile')
add
self.mox.StubOutWithMock(watchlists.logging, 'error')

it's much simpler than creating a seam function.

http://codereview.chromium.org/126272/diff/1003/4#newcode34
Line 34: self.mox.StubOutWithMock(watchlists, '_Error')
no need for that anymore

http://codereview.chromium.org/126272/diff/1003/5
File watchlists.py (right):

http://codereview.chromium.org/126272/diff/1003/5#newcode26
Line 26: def _Error(text):
No need for that, see the other comment.

[Nirnimesh, 2009-06-17 14:18:58]

http://codereview.chromium.org/126272/diff/1003/4
File tests/watchlists_unittest.py (right):

http://codereview.chromium.org/126272/diff/1003/4#newcode19
Line 19: self.mox.StubOutWithMock(watchlists.Watchlists,
'_ContentsOfWatchlistsFile')
On 2009/06/17 13:39:55, Marc-Antoine Ruel wrote:
> add
> self.mox.StubOutWithMock(watchlists.logging, 'error')
> 
> it's much simpler than creating a seam function.

Done.

http://codereview.chromium.org/126272/diff/1003/4#newcode34
Line 34: self.mox.StubOutWithMock(watchlists, '_Error')
On 2009/06/17 13:39:55, Marc-Antoine Ruel wrote:
> no need for that anymore

Cool. I'm beginning to like pymox.

http://codereview.chromium.org/126272/diff/1003/5
File watchlists.py (right):

http://codereview.chromium.org/126272/diff/1003/5#newcode26
Line 26: def _Error(text):
On 2009/06/17 13:39:55, Marc-Antoine Ruel wrote:
> No need for that, see the other comment.

Done.

[M-A Ruel, 2009-06-17 14:23:51]

lgtm

[John Grabowski, 2009-06-17 18:42:13]

LGTM

http://codereview.chromium.org/126272/diff/1005/11
File PRESUBMIT.py (right):

http://codereview.chromium.org/126272/diff/1005/11#newcode18
Line 18: 'tests.watchlists_unittest',
Hmm.  Do we run all these unit tests on "gcl upload"?  
Perhaps it's fast enough to not care, but I worry about it being excessive.

Could you do something like this:
import timeit
x = timeit.Timer('RunPythonUnitTests ... ')
print x.timeit()

Then add the answer in a comment?

http://codereview.chromium.org/126272/diff/1005/9
File tests/watchlists_unittest.py (right):

http://codereview.chromium.org/126272/diff/1005/9#newcode6
Line 6: """Unit tests for watchlists.py."""
I wuv u

http://codereview.chromium.org/126272/diff/1005/9#newcode9
Line 9: 
extra space

http://codereview.chromium.org/126272/diff/1005/9#newcode10
Line 10: import super_mox
It's not your fault, but the "checkout mox with svn on demand" is a bit awkward.
 :-(  Better to "svn external" add it to the tree I think.

http://codereview.chromium.org/126272/diff/1005/9#newcode32
Line 32: contents = "some garbled and unwanted text"
" --> '   for consistency

http://codereview.chromium.org/126272/diff/1005/9#newcode54
Line 54: watchlists.Watchlists._HasWatchlistsFile().AndReturn(True)
Note python is dynamic, so for simple ops like this you can simply smack it in. 
E.g.
watchlists.Watchlists._HasWatchlistsFile = lambda: True

http://codereview.chromium.org/126272/diff/1005/10
File watchlists.py (right):

http://codereview.chromium.org/126272/diff/1005/10#newcode55
Line 55: watchlists_file = open(self._GetRulesFilePath())
"""Comment for each function please."""
Also, add a try block in case the file doesn't exist which returns an empty
string and prints a warning.  Lack of a watchlist file shouldn't be a fatal
error.

Perhaps then this becomes:
try: 
  return open(self._GetRulesFilePath()).read() # file closed when GC'd
except IOError, e:
  print 'Warning: no watchlist file ... '
  return ''

[M-A Ruel, 2009-06-17 19:20:55]

http://codereview.chromium.org/126272/diff/1005/11
File PRESUBMIT.py (right):

http://codereview.chromium.org/126272/diff/1005/11#newcode18
Line 18: 'tests.watchlists_unittest',
On 2009/06/17 18:42:13, John Grabowski wrote:
> Hmm.  Do we run all these unit tests on "gcl upload"?  
> Perhaps it's fast enough to not care, but I worry about it being excessive.

It's fast enough to not care.

> Could you do something like this:
> import timeit
> x = timeit.Timer('RunPythonUnitTests ... ')
> print x.timeit()
> 
> Then add the answer in a comment? 

Patches are welcome!

http://codereview.chromium.org/126272/diff/1005/9
File tests/watchlists_unittest.py (right):

http://codereview.chromium.org/126272/diff/1005/9#newcode10
Line 10: import super_mox
On 2009/06/17 18:42:13, John Grabowski wrote:
> It's not your fault, but the "checkout mox with svn on demand" is a bit
awkward.
>  :-(  Better to "svn external" add it to the tree I think.

I disagree, 99% percent of the depot_tools users don't want pymox, which
includes the build slaves. Adding a svn:external would add at least 3 seconds on
each 'update scripts' step.

[Nirnimesh, 2009-06-18 03:23:40]

http://codereview.chromium.org/126272/diff/1005/9
File tests/watchlists_unittest.py (right):

http://codereview.chromium.org/126272/diff/1005/9#newcode9
Line 9: 
On 2009/06/17 18:42:13, John Grabowski wrote:
> extra space

Done.

http://codereview.chromium.org/126272/diff/1005/9#newcode10
Line 10: import super_mox
FWIW, mox on demand didn't really work for me. I ended up installing mox as
README.gclient_test said.

http://codereview.chromium.org/126272/diff/1005/9#newcode32
Line 32: contents = "some garbled and unwanted text"
On 2009/06/17 18:42:13, John Grabowski wrote:
> " --> '   for consistency
> 
> 

Done.

http://codereview.chromium.org/126272/diff/1005/9#newcode54
Line 54: watchlists.Watchlists._HasWatchlistsFile().AndReturn(True)
On 2009/06/17 18:42:13, John Grabowski wrote:
> Note python is dynamic, so for simple ops like this you can simply smack it
in. 
> E.g.
> watchlists.Watchlists._HasWatchlistsFile = lambda: True
> 
> 

Interesting. Thanks

http://codereview.chromium.org/126272/diff/1005/10
File watchlists.py (right):

http://codereview.chromium.org/126272/diff/1005/10#newcode55
Line 55: watchlists_file = open(self._GetRulesFilePath())
On 2009/06/17 18:42:13, John Grabowski wrote:
> """Comment for each function please."""
> Also, add a try block in case the file doesn't exist which returns an empty
> string and prints a warning.  Lack of a watchlist file shouldn't be a fatal
> error.
> 
> Perhaps then this becomes:
> try: 
>   return open(self._GetRulesFilePath()).read() # file closed when GC'd
> except IOError, e:
>   print 'Warning: no watchlist file ... '
>   return ''
> 

The only way to reach this fn call is to go through _HasWatchlistsFile() which
would block it.

ANyway, it's good to have a try block, in case of other reasons like file not
readable, etc...

Done