|
|
Created:
8 years, 11 months ago by Sam Kerner (Chrome) Modified:
8 years, 11 months ago CC:
chromium-reviews, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMatch 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
Patch Set 1 : rev #
Total comments: 6
Patch Set 2 : Avoid mocking QueryDosDevice #
Total comments: 6
Patch Set 3 : Rev comments. #
Messages
Total messages: 14 (0 generated)
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.
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.
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.
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.
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/
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.
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
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.
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
You're right, I missed that we would find the substring in that last test. LGTM
LGTM rubberstamp
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skerner@chromium.org/9167004/10004
Change committed as 117986 |