Match whole path components in DevicePathToDriveLetterPath(). Add tests.

Match whole path components in DevicePathToDriveLetterPath().  Add tests.

+rvargas, who understands the code
+brettw, who is in OWNERS

BUG=109577
TEST=FileUtilTest.DevicePathToDriveLetter


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

[rvargas (doing something else), 2012-01-11 22:26:26]

Thanks for the effort of writing unit tests!.

I'm not so sure about this way to do it, though. It basically means that in the
future we'd want a good part of the OS interface funneled through xx_interface,
for the purpose of enabling mocking, so I don't think this is a decision to take
lightly.

That, and the technical detail of actually passing the array around. (we could
work around that)

Maybe it is better to have classes a little more abstracted from the OS, and
mock them instead?

Let's wait for Brett's opinion.

http://codereview.chromium.org/9167004/diff/10001/base/file_util.h
File base/file_util.h (right):

http://codereview.chromium.org/9167004/diff/10001/base/file_util.h#newcode56
base/file_util.h:56: bool
DevicePathToDriveLetterPath(base::win::WinApiInterface& win_api,
If we put it here so that it has to be accessed by the tests, it needs
documentation, and it has to be exported.

http://codereview.chromium.org/9167004/diff/10001/base/file_util_win.cc
File base/file_util_win.cc (right):

http://codereview.chromium.org/9167004/diff/10001/base/file_util_win.cc#newco...
base/file_util_win.cc:42: bool
DevicePathToDriveLetterPath(base::win::WinApiInterface& win_api,
I don't think that adding an argument with fn pointers to every function we want
to test is something that scales.

I'm also wondering if it's not better to test the caller of this code, given
that so far this is just a "private helper" that is used from a single location.

http://codereview.chromium.org/9167004/diff/10001/base/file_util_win.cc#newco...
base/file_util_win.cc:68: FilePath device_path_to_drive(device_path_as_string);
This name is confusing. How about just device_path ?

http://codereview.chromium.org/9167004/diff/10001/base/win/win_api_interface.h
File base/win/win_api_interface.h (right):

http://codereview.chromium.org/9167004/diff/10001/base/win/win_api_interface....
base/win/win_api_interface.h:37: static WinApiInterface* GetInstance();
This has to be exported.

[Sam Kerner (Chrome), 2012-01-11 23:26:27]

http://codereview.chromium.org/9167004/diff/10001/base/file_util_win.cc
File base/file_util_win.cc (right):

http://codereview.chromium.org/9167004/diff/10001/base/file_util_win.cc#newco...
base/file_util_win.cc:42: bool
DevicePathToDriveLetterPath(base::win::WinApiInterface& win_api,
On 2012/01/11 22:26:26, rvargas wrote:
> I'm also wondering if it's not better to test the caller of this code, given
> that so far this is just a "private helper" that is used from a single
location.

If I tested the caller by adding a parameter, users of base would need to get
the new parameter.  I wanted to avoid changing the exported interface.  I
realize that I ended up exporting DevicePathToDriveLetterPath().  My intention
is to only expose it for tests.

[Sam Kerner (Chrome), 2012-01-11 23:27:27]

On 2012/01/11 22:26:26, rvargas wrote:
> Thanks for the effort of writing unit tests!.
> 
> I'm not so sure about this way to do it, though. It basically means that in
the
> future we'd want a good part of the OS interface funneled through
xx_interface,
> for the purpose of enabling mocking, so I don't think this is a decision to
take
> lightly.
> 
> That, and the technical detail of actually passing the array around. (we could
> work around that)
> 
> Maybe it is better to have classes a little more abstracted from the OS, and
> mock them instead?
> 
> Let's wait for Brett's opinion.

I struggled for a while to find a sane way to write tests for this.  I am open
to ideas for a better way.  Some ideas I considered:

* Hook API calls.  This will break if the test environment uses the same calls.

* Replace windows API calls with calls to a function set by a template
parameter.  Tests can instantiate a testable version of a function by changing
the template parameter.

template<class QueryDosDeviceTemplateVal>
void DoSomethingImpl(int arg) {
  ...
  QueryDosDeviceTemplateVal(...);
  ...
}

void DoSomething(int arg) {
  return DoSomethingImpl<::QueryDosDevice>(arg);
}

Seems ugly and adds lots of boiler-plate code.

* Don't write tests.






> 
> http://codereview.chromium.org/9167004/diff/10001/base/file_util.h
> File base/file_util.h (right):
> 
> http://codereview.chromium.org/9167004/diff/10001/base/file_util.h#newcode56
> base/file_util.h:56: bool
> DevicePathToDriveLetterPath(base::win::WinApiInterface& win_api,
> If we put it here so that it has to be accessed by the tests, it needs
> documentation, and it has to be exported.
> 
> http://codereview.chromium.org/9167004/diff/10001/base/file_util_win.cc
> File base/file_util_win.cc (right):
> 
>
http://codereview.chromium.org/9167004/diff/10001/base/file_util_win.cc#newco...
> base/file_util_win.cc:42: bool
> DevicePathToDriveLetterPath(base::win::WinApiInterface& win_api,
> I don't think that adding an argument with fn pointers to every function we
want
> to test is something that scales.
> 
> I'm also wondering if it's not better to test the caller of this code, given
> that so far this is just a "private helper" that is used from a single
location.
> 
>
http://codereview.chromium.org/9167004/diff/10001/base/file_util_win.cc#newco...
> base/file_util_win.cc:68: FilePath
device_path_to_drive(device_path_as_string);
> This name is confusing. How about just device_path ?
> 
> http://codereview.chromium.org/9167004/diff/10001/base/win/win_api_interface.h
> File base/win/win_api_interface.h (right):
> 
>
http://codereview.chromium.org/9167004/diff/10001/base/win/win_api_interface....
> base/win/win_api_interface.h:37: static WinApiInterface* GetInstance();
> This has to be exported.

[rvargas (doing something else), 2012-01-11 23:53:33]

In general, I would say that making our interfaces be aware all the time about
the mocking is not desirable.

What I have seen in some other places is just a static pointer private to the
module under test, and an explicit method to alter that for tests:

foo.cc

MyType* my_mocckable_object = NULL;

foo.h

void SetSomethingForTest(MyType* obj);

Yes, static object are not cool, and this may lead to tests that bleed into
other tests (depending on how the test is written), but it is simple and doesn't
disturb the regular code too much.

I would not go as far as to attempt to centralize everything into a single
file/type/interface and just live with a bunch of unconnected things.

http://codereview.chromium.org/9167004/diff/10001/base/file_util_win.cc
File base/file_util_win.cc (right):

http://codereview.chromium.org/9167004/diff/10001/base/file_util_win.cc#newco...
base/file_util_win.cc:42: bool
DevicePathToDriveLetterPath(base::win::WinApiInterface& win_api,
On 2012/01/11 23:26:27, Sam Kerner (Chrome) wrote:
> On 2012/01/11 22:26:26, rvargas wrote:
> > I'm also wondering if it's not better to test the caller of this code, given
> > that so far this is just a "private helper" that is used from a single
> location.
> 
> If I tested the caller by adding a parameter, users of base would need to get
> the new parameter.  I wanted to avoid changing the exported interface.  I

And that's one of the arguments against testing passing an extra argument to
every fn we want to test.


> realize that I ended up exporting DevicePathToDriveLetterPath().  My intention
> is to only expose it for tests.

[skerner, 2012-01-12 01:13:23]

On Wed, Jan 11, 2012 at 6:53 PM,  <rvargas@chromium.org> wrote:
> In general, I would say that making our interfaces be aware all the time
> about
> the mocking is not desirable.
>
> What I have seen in some other places is just a static pointer private to
> the
> module under test, and an explicit method to alter that for tests:
>
> foo.cc
>
> MyType* my_mocckable_object = NULL;
>
> foo.h
>
> void SetSomethingForTest(MyType* obj);
>
> Yes, static object are not cool, and this may lead to tests that bleed into
> other tests (depending on how the test is written), but it is simple and
> doesn't
> disturb the regular code too much.
>
> I would not go as far as to attempt to centralize everything into a single
> file/type/interface and just live with a bunch of unconnected things.

Okay, I could do that.  By creating a static pointer that allows an
alternate version of QueryDosDevice, I would be able to test the
interesting case.

Brett, would you approve of such a change?

Sam

>
>
>
> http://codereview.chromium.org/9167004/diff/10001/base/file_util_win.cc
> File base/file_util_win.cc (right):
>
>
http://codereview.chromium.org/9167004/diff/10001/base/file_util_win.cc#newco...
> base/file_util_win.cc:42: bool
> DevicePathToDriveLetterPath(base::win::WinApiInterface& win_api,
> On 2012/01/11 23:26:27, Sam Kerner (Chrome) wrote:
>>
>> On 2012/01/11 22:26:26, rvargas wrote:
>> > I'm also wondering if it's not better to test the caller of this
>
> code, given
>>
>> > that so far this is just a "private helper" that is used from a
>
> single
>>
>> location.
>
>
>> If I tested the caller by adding a parameter, users of base would need
>
> to get
>>
>> the new parameter.  I wanted to avoid changing the exported interface.
>
>  I
>
> And that's one of the arguments against testing passing an extra
> argument to every fn we want to test.
>
>
>
>> realize that I ended up exporting DevicePathToDriveLetterPath().  My
>
> intention
>>
>> is to only expose it for tests.
>
>
> http://codereview.chromium.org/9167004/

[Sam Kerner (Chrome), 2012-01-12 18:12:28]

I found a way to write tests without mocking QueryDosDevice() and friends. 
PTAL.

I do need to export DevicePathToDriveLetterPath(), because the method that uses
it always operates on a real path.  The test hits corner cases by running on
paths that are altered versions of real paths.

[rvargas (doing something else), 2012-01-12 21:01:04]

The new tests seem OK for the code that is being added to the API (an exposed
function should have tests)... but adding that code to the API in order to add
the tests is not so good. I mean, it is not bad to expose that function, but the
new tests are not testing much more than what is being tested today with 
NormalizeFilePath*.

I guess the only new thing being tested is that we can decode just a drive
letter, without extra directories, and this doesn't seem that valuable.

More to the point, the new tests are not able to detect the bug that is being
fixed, are they?. I just wanted to point that out; my intention is not to force
you to write more comprehensive tests.

http://codereview.chromium.org/9167004/diff/15001/base/file_util_unittest.cc
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/9167004/diff/15001/base/file_util_unittest.cc#...
base/file_util_unittest.cc:601: actual_device_path,
nit: it looks like this fits in the previous line (and the second argument
aligned under the first one)

http://codereview.chromium.org/9167004/diff/15001/base/file_util_unittest.cc#...
base/file_util_unittest.cc:607: FilePath
kRelitivePath(FPL("dir1\\dir2\\file.txt"));
nit: kRelative

[Sam Kerner (Chrome), 2012-01-12 21:19:12]

http://codereview.chromium.org/9167004/diff/15001/base/file_util_unittest.cc
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/9167004/diff/15001/base/file_util_unittest.cc#...
base/file_util_unittest.cc:601: actual_device_path,
On 2012/01/12 21:01:04, rvargas wrote:
> nit: it looks like this fits in the previous line (and the second argument
> aligned under the first one)

Done.

http://codereview.chromium.org/9167004/diff/15001/base/file_util_unittest.cc#...
base/file_util_unittest.cc:607: FilePath
kRelitivePath(FPL("dir1\\dir2\\file.txt"));
On 2012/01/12 21:01:04, rvargas wrote:
> nit: kRelative

Done.

http://codereview.chromium.org/9167004/diff/15001/base/file_util_unittest.cc#...
base/file_util_unittest.cc:645:
ASSERT_FALSE(file_util::DevicePathToDriveLetterPath(
This assert will fail without the fix in file_util_win.cc.

http://codereview.chromium.org/9167004/diff/15001/base/file_util_unittest.cc#...
base/file_util_unittest.cc:649:
ASSERT_FALSE(file_util::DevicePathToDriveLetterPath(
This one will also fail.

[Sam Kerner (Chrome), 2012-01-12 21:26:36]

On 2012/01/12 21:01:04, rvargas wrote:
> The new tests seem OK for the code that is being added to the API (an exposed
> function should have tests)... but adding that code to the API in order to add
> the tests is not so good. I mean, it is not bad to expose that function, but
the
> new tests are not testing much more than what is being tested today with 
> NormalizeFilePath*.
> 
> I guess the only new thing being tested is that we can decode just a drive
> letter, without extra directories, and this doesn't seem that valuable.
> 
> More to the point, the new tests are not able to detect the bug that is being
> fixed, are they?. I just wanted to point that out; my intention is not to
force
> you to write more comprehensive tests.

The last two asserts in the test both detect the bug that the CL fixes.  They
are the reason I think the test is worth adding.

The bug being fixed can only be caused if one path returned by QueryDosDevice is
a substring of another path returned by QueryDosDevice(), unless I control the
path being normalized.  I can't control that path when the function is used by
NormalizeFilePath, because the start of the path is a real path to a drive
letter, and a test can't change that path.

Would you prefer I expose the function only to tests with an #ifdef?

> 
> http://codereview.chromium.org/9167004/diff/15001/base/file_util_unittest.cc
> File base/file_util_unittest.cc (right):
> 
>
http://codereview.chromium.org/9167004/diff/15001/base/file_util_unittest.cc#...
> base/file_util_unittest.cc:601: actual_device_path,
> nit: it looks like this fits in the previous line (and the second argument
> aligned under the first one)
> 
>
http://codereview.chromium.org/9167004/diff/15001/base/file_util_unittest.cc#...
> base/file_util_unittest.cc:607: FilePath
> kRelitivePath(FPL("dir1\\dir2\\file.txt"));
> nit: kRelative

[rvargas (doing something else), 2012-01-12 21:39:22]

You're right, I missed that we would find the substring in that last test.

LGTM

[brettw, 2012-01-17 20:41:31]

LGTM rubberstamp

[commit-bot: I haz the power, 2012-01-17 20:45:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skerner@chromium.org/9167004/10004

[commit-bot: I haz the power, 2012-01-17 22:44:33]

Change committed as 117986