Readd missing unittests for owners

Readd missing unittests for owners

TEST=Run tests/owners_unittest.py


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

[Zachary Kuznia, 2012-05-10 03:37:33]

Hi Nicolas,

Could you review this at:
https://chromiumcodereview.appspot.com/10384099

These tests were supposed to land with my last patch, but I somehow lost the
last git commit when uploading it.

Thanks,
-Zach

[Dirk Pranke, 2012-05-10 18:26:11]

https://chromiumcodereview.appspot.com/10384099/diff/3001/owners.py
File owners.py (right):

https://chromiumcodereview.appspot.com/10384099/diff/3001/owners.py#newcode182
owners.py:182: if self._stop_looking(dirname) or dirname == '/':
I'm a bit troubled by the "dirname == '/'" addition; that tells me that
stop_looking isn't working the way it's supposed to (it's supposed to stop at
the root of the checkout). I wonder if we lost a relpath() somewhere at some
point so that we are no longer handing dirnames relative to the root of the
checkout and hence dirname != '' any more.

[Zachary Kuznia, 2012-05-14 04:51:05]

This actually is because "dirname = self.os_path.dirname(dirname)" returns
"/" for the root directory, not "".  This could also be solved by adding
"/" to the initial value of stop_looking.  Would this be preferable?

Thanks,
-Zach

On Fri, May 11, 2012 at 3:26 AM, <dpranke@chromium.org> wrote:

>
>
https://chromiumcodereview.**appspot.com/10384099/diff/**3001/owners.py<https...
> File owners.py (right):
>
> https://chromiumcodereview.**appspot.com/10384099/diff/**
>
3001/owners.py#newcode182<https://chromiumcodereview.appspot.com/10384099/diff/3001/owners.py#newcode182>
> owners.py:182: if self._stop_looking(dirname) or dirname == '/':
> I'm a bit troubled by the "dirname == '/'" addition; that tells me that
> stop_looking isn't working the way it's supposed to (it's supposed to
> stop at the root of the checkout). I wonder if we lost a relpath()
> somewhere at some point so that we are no longer handing dirnames
> relative to the root of the checkout and hence dirname != '' any more.
>
>
https://chromiumcodereview.**appspot.com/10384099/<https://chromiumcodereview...
>

[M-A Ruel, 2012-05-14 16:10:19]

The canonical way to implement it is:

prev_parent = dirname
dirname = self.os_path.dirname(dirname)
if prev_parent == dirname:
  break



2012/5/14 Zach Kuznia <zork@chromium.org>

> This actually is because "dirname = self.os_path.dirname(dirname)" returns
> "/" for the root directory, not "".  This could also be solved by adding
> "/" to the initial value of stop_looking.  Would this be preferable?
>
> Thanks,
> -Zach
>
>
> On Fri, May 11, 2012 at 3:26 AM, <dpranke@chromium.org> wrote:
>
>>
>>
https://chromiumcodereview.**appspot.com/10384099/diff/**3001/owners.py<https...
>> File owners.py (right):
>>
>> https://chromiumcodereview.**appspot.com/10384099/diff/**
>>
3001/owners.py#newcode182<https://chromiumcodereview.appspot.com/10384099/diff/3001/owners.py#newcode182>
>> owners.py:182: if self._stop_looking(dirname) or dirname == '/':
>> I'm a bit troubled by the "dirname == '/'" addition; that tells me that
>> stop_looking isn't working the way it's supposed to (it's supposed to
>> stop at the root of the checkout). I wonder if we lost a relpath()
>> somewhere at some point so that we are no longer handing dirnames
>> relative to the root of the checkout and hence dirname != '' any more.
>>
>>
https://chromiumcodereview.**appspot.com/10384099/<https://chromiumcodereview...
>>
>
>

[Dirk Pranke, 2012-05-14 18:58:35]

There was a reason I set it to be "" and not "/"; unfortunately, I
can't remember what that reason was :(. I need to see if I can
dislodge it from somewhere back in my brain.

-- Dirk

On Sun, May 13, 2012 at 9:51 PM, Zach Kuznia <zork@chromium.org> wrote:
> This actually is because "dirname = self.os_path.dirname(dirname)" returns
> "/" for the root directory, not "".  This could also be solved by adding "/"
> to the initial value of stop_looking.  Would this be preferable?
>
> Thanks,
> -Zach
>
>
> On Fri, May 11, 2012 at 3:26 AM, <dpranke@chromium.org> wrote:
>>
>>
>> https://chromiumcodereview.appspot.com/10384099/diff/3001/owners.py
>> File owners.py (right):
>>
>>
>>
https://chromiumcodereview.appspot.com/10384099/diff/3001/owners.py#newcode182
>> owners.py:182: if self._stop_looking(dirname) or dirname == '/':
>> I'm a bit troubled by the "dirname == '/'" addition; that tells me that
>> stop_looking isn't working the way it's supposed to (it's supposed to
>> stop at the root of the checkout). I wonder if we lost a relpath()
>> somewhere at some point so that we are no longer handing dirnames
>> relative to the root of the checkout and hence dirname != '' any more.
>>
>> https://chromiumcodereview.appspot.com/10384099/
>
>

[Dirk Pranke, 2012-05-14 18:58:53]

lgtm for now, though. I don't want to hold this up.

[commit-bot: I haz the power, 2012-05-15 01:07:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zork@chromium.org/10384099/6003

[commit-bot: I haz the power, 2012-05-15 01:15:45]

Change committed as 137041