add tests for owners database module in depot_tools

Add some unit tests. They don't pass yet.

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

[Dirk Pranke, 2011-03-03 03:58:19]

This change includes a stripped-down version of the "mock filesystem" object I
wrote for WebKit. Let me know if there's some better place to put this.

Also, I don't fully grok your naming conventions yet, so I'm not sure what
should be called a "SmokeTest" or something else. Please advise.

[M-A Ruel, 2011-03-03 16:20:46]

http://codereview.chromium.org/6612011/diff/1/tests/filesystem_mock.py
File tests/filesystem_mock.py (right):

http://codereview.chromium.org/6612011/diff/1/tests/filesystem_mock.py#newcode13
tests/filesystem_mock.py:13: class MockFileSystem(object):
Personally, I think it's waaay simpler (and safer) to just create a temporary
directory on the fly and mocking it.

os.makedirs(os.path.join(base_path, 'test', 'a'))
open(os.path.join(base_path, 'test', 'OWNERS'), 'w').write('*')
open(os.path.join(base_path, 'test', 'a', 'OWNERS'), 'w').write('foo')

If you need a SCM, FakeRepos already does that. It could be interesting to have
FakeRepos to have a mode where it doesn't starts neither svnserve nor create a
git repository. 

But I don't really mind, especially that your code doesn't *write* to the file
system. Feel free to keep this design if you prefer.

http://codereview.chromium.org/6612011/diff/1/tests/owners_unittest.py
File tests/owners_unittest.py (right):

http://codereview.chromium.org/6612011/diff/1/tests/owners_unittest.py#newcode45
tests/owners_unittest.py:45: ben = 'ben@chromium.org'
Use example.com

[Dirk Pranke, 2011-03-03 17:28:26]

On Thu, Mar 3, 2011 at 8:20 AM,  <maruel@chromium.org> wrote:
>
> http://codereview.chromium.org/6612011/diff/1/tests/filesystem_mock.py
> File tests/filesystem_mock.py (right):
>
>
http://codereview.chromium.org/6612011/diff/1/tests/filesystem_mock.py#newcode13
> tests/filesystem_mock.py:13: class MockFileSystem(object):
> Personally, I think it's waaay simpler (and safer) to just create a
> temporary directory on the fly and mocking it.
>

Yeah, I've gotten pretty used to writing tests that don't touch the
filesystem. Faster
and more reliable in my experience.

> os.makedirs(os.path.join(base_path, 'test', 'a'))
> open(os.path.join(base_path, 'test', 'OWNERS'), 'w').write('*')
> open(os.path.join(base_path, 'test', 'a', 'OWNERS'), 'w').write('foo')
>
> If you need a SCM, FakeRepos already does that. It could be interesting
> to have FakeRepos to have a mode where it doesn't starts neither
> svnserve nor create a git repository.
>

This code shouldn't ever need an actual checkout, so I think that'll
be unnecessary.

> But I don't really mind, especially that your code doesn't *write* to
> the file system. Feel free to keep this design if you prefer.
>
> http://codereview.chromium.org/6612011/diff/1/tests/owners_unittest.py
> File tests/owners_unittest.py (right):
>
>
http://codereview.chromium.org/6612011/diff/1/tests/owners_unittest.py#newcode45
> tests/owners_unittest.py:45: ben = 'ben@chromium.org'
> Use example.com
>

Okay. Any particular reason you don't want to use @chromium.org? Anti-spam
harvesting or something?

-- Dirk

> http://codereview.chromium.org/6612011/
>

[Dirk Pranke, 2011-03-03 21:10:38]

update.

[M-A Ruel, 2011-03-03 21:21:56]

lgtm with some nits, pending http://codereview.chromium.org/6609012/

http://codereview.chromium.org/6612011/diff/4001/tests/filesystem_mock.py
File tests/filesystem_mock.py (right):

http://codereview.chromium.org/6612011/diff/4001/tests/filesystem_mock.py#new...
tests/filesystem_mock.py:26: def _get_sep(self):
you could use:
@property
def sep(self):
  return self._sep

you are using the old python 2.3 form

http://codereview.chromium.org/6612011/diff/4001/tests/filesystem_mock.py#new...
tests/filesystem_mock.py:31: def _raise_not_found(self, path):  # pylint:
disable=R0201
you can make it static in fact

http://codereview.chromium.org/6612011/diff/4001/tests/owners_unittest.py
File tests/owners_unittest.py (right):

http://codereview.chromium.org/6612011/diff/4001/tests/owners_unittest.py#new...
tests/owners_unittest.py:59: self.fopen = lambda path:
self.repo.open_for_reading(path)
it's right, just use:
self.fopen = self.repo.open_for_reading

[Dirk Pranke, 2011-03-03 22:51:20]

On 2011/03/03 21:21:56, Marc-Antoine Ruel wrote:
> lgtm with some nits, pending http://codereview.chromium.org/6609012/
> 
> http://codereview.chromium.org/6612011/diff/4001/tests/filesystem_mock.py
> File tests/filesystem_mock.py (right):
> 
>
http://codereview.chromium.org/6612011/diff/4001/tests/filesystem_mock.py#new...
> tests/filesystem_mock.py:26: def _get_sep(self):
> you could use:
> @property
> def sep(self):
>   return self._sep
> 
> you are using the old python 2.3 form
> 

Ah, I didn't realize @property declared a readonly property; I thought it was
read-write. Learned something new.

>
http://codereview.chromium.org/6612011/diff/4001/tests/filesystem_mock.py#new...
> tests/filesystem_mock.py:31: def _raise_not_found(self, path):  # pylint:
> disable=R0201
> you can make it static in fact
>

Yeah, but I don't want to. I have a strong dislike of static methods.
 
> http://codereview.chromium.org/6612011/diff/4001/tests/owners_unittest.py
> File tests/owners_unittest.py (right):
> 
>
http://codereview.chromium.org/6612011/diff/4001/tests/owners_unittest.py#new...
> tests/owners_unittest.py:59: self.fopen = lambda path:
> self.repo.open_for_reading(path)
> it's right, just use:
> self.fopen = self.repo.open_for_reading

Good suggestion. Done.

[M-A Ruel, 2011-03-04 02:04:21]

lgtm