Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
439873002:
Check for empty paths in path_util (Closed)
DescriptionCheck for empty paths in path_util
BUG=400530
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288187
Patch Set 1 #
Total comments: 2
Patch Set 2 : Tighten #ifdefs #
Total comments: 2
Patch Set 3 : Added tests #Patch Set 4 : Testing comments #
Total comments: 16
Patch Set 5 : Enhanced test #
Total comments: 8
Patch Set 6 : Minor changes #
Total comments: 5
Messages
Total messages: 23 (0 generated)
Unfortunately, the Mac machine we have in the office is not set up for building, so I can't test this on it. I went ahead and checked for empty file paths in both versions of PrettifyPath.
are there no tests for the PrettifyPath function? Maybe we should add a few :)
The functionality was added here: https://chromiumcodereview.appspot.com/10693089 which does include test, an API test, though curiously for windows only. I'm a little scared of testing this because it's so dependent on the context of where the test is running from. I'm not too fussed about just checking for the empty path then moving on - though I'd like to see the #ifdefs tightened up a bit, they're quite broad in scope, as commented. https://codereview.chromium.org/439873002/diff/1/chrome/browser/extensions/pa... File chrome/browser/extensions/path_util.cc (right): https://codereview.chromium.org/439873002/diff/1/chrome/browser/extensions/pa... chrome/browser/extensions/path_util.cc:27: std::string GetDisplayBaseName(const base::FilePath& path) { put specific OS_MACOSX namespace around this function https://codereview.chromium.org/439873002/diff/1/chrome/browser/extensions/pa... chrome/browser/extensions/path_util.cc:44: base::FilePath PrettifyPath(const base::FilePath& source_path) { declare this function then do the empty-path check and the other parts which are common to every implementation (getting the home directory). then ifdef out only the important parts. e.g. base::FilePath PrettifyPath(..) { if (source_path.empty()) return base::FilePath(); base::FilePath home_path; if (!PathService::Get(base::DIR_HOME, &home_path)) return base::FilePath(); #if defined(OS_MACOSX) ... #else // defined(OS_MAXOSX) ... #endif }
On 2014/08/04 23:39:07, kalman wrote: > The functionality was added here: > > https://chromiumcodereview.appspot.com/10693089 > > which does include test, an API test, though curiously for windows only. > > I'm a little scared of testing this because it's so dependent on the context of > where the test is running from. Yeah, it can be a bit tricky to test. But it seems like a few quick sanity checks like: FILE_PATH_LITERAL home_short("~"); FilePath home = GetHomeDir(); FilePath prettified = PrettifyPath(home.AppendASCII("foo")); ASSERT_EQ(FilePath(home_short).AppendASCII("foo").AsUTFUnsafe(), prettified.AsUTFUnsafe()); Would work and at least ensure that nothing breaks down the road, right? Or is it more complicated because of our crazy test bot setup?
On 2014/08/04 23:50:33, Devlin wrote: > On 2014/08/04 23:39:07, kalman wrote: > > The functionality was added here: > > > > https://chromiumcodereview.appspot.com/10693089 > > > > which does include test, an API test, though curiously for windows only. > > > > I'm a little scared of testing this because it's so dependent on the context > of > > where the test is running from. > > Yeah, it can be a bit tricky to test. But it seems like a few quick sanity > checks like: > > FILE_PATH_LITERAL home_short("~"); > FilePath home = GetHomeDir(); > FilePath prettified = PrettifyPath(home.AppendASCII("foo")); > ASSERT_EQ(FilePath(home_short).AppendASCII("foo").AsUTFUnsafe(), > prettified.AsUTFUnsafe()); > > Would work and at least ensure that nothing breaks down the road, right? Or is > it more complicated because of our crazy test bot setup? I agree that at the very least we should add a regression test for an empty file path, and link to that API test which supposedly tests the rest - and your example test looks pretty safe too (though you shouldn't need the AsUTFUnsafe calls).
On 2014/08/04 23:55:36, kalman wrote: > On 2014/08/04 23:50:33, Devlin wrote: > > On 2014/08/04 23:39:07, kalman wrote: > > > The functionality was added here: > > > > > > https://chromiumcodereview.appspot.com/10693089 > > > > > > which does include test, an API test, though curiously for windows only. > > > > > > I'm a little scared of testing this because it's so dependent on the context > > of > > > where the test is running from. > > > > Yeah, it can be a bit tricky to test. But it seems like a few quick sanity > > checks like: > > > > FILE_PATH_LITERAL home_short("~"); > > FilePath home = GetHomeDir(); > > FilePath prettified = PrettifyPath(home.AppendASCII("foo")); > > ASSERT_EQ(FilePath(home_short).AppendASCII("foo").AsUTFUnsafe(), > > prettified.AsUTFUnsafe()); > > > > Would work and at least ensure that nothing breaks down the road, right? Or > is > > it more complicated because of our crazy test bot setup? > > I agree that at the very least we should add a regression test for an empty file > path, and link to that API test which supposedly tests the rest - and your > example test looks pretty safe too (though you shouldn't need the AsUTFUnsafe > calls). So this should be done in a path_util_unittest file? I started adding one and ran into a conflict with this file. Would I just be able to rename the class to ExtensionsPathUtilTest, or something to that effect? https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/loc... Also, I shuffled around the #ifdefs a bit, but I still need that function to be in the #ifdef because of mac specific function calls. Can you take a look and see what you think?
yes that's the sort of thing I had in mind. what do you mean it conflicts? they should be in different namespaces. https://codereview.chromium.org/439873002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/path_util.cc (right): https://codereview.chromium.org/439873002/diff/20001/chrome/browser/extension... chrome/browser/extensions/path_util.cc:23: namespace OS_MACOSX { why do you have this extra namespace? it can stay in an anonymous namespace.
On 2014/08/05 00:02:54, gpdavis wrote: > On 2014/08/04 23:55:36, kalman wrote: > > On 2014/08/04 23:50:33, Devlin wrote: > > > On 2014/08/04 23:39:07, kalman wrote: > > > > The functionality was added here: > > > > > > > > https://chromiumcodereview.appspot.com/10693089 > > > > > > > > which does include test, an API test, though curiously for windows only. > > > > > > > > I'm a little scared of testing this because it's so dependent on the > context > > > of > > > > where the test is running from. > > > > > > Yeah, it can be a bit tricky to test. But it seems like a few quick sanity > > > checks like: > > > > > > FILE_PATH_LITERAL home_short("~"); > > > FilePath home = GetHomeDir(); > > > FilePath prettified = PrettifyPath(home.AppendASCII("foo")); > > > ASSERT_EQ(FilePath(home_short).AppendASCII("foo").AsUTFUnsafe(), > > > prettified.AsUTFUnsafe()); > > > > > > Would work and at least ensure that nothing breaks down the road, right? Or > > is > > > it more complicated because of our crazy test bot setup? > > > > I agree that at the very least we should add a regression test for an empty > file > > path, and link to that API test which supposedly tests the rest - and your > > example test looks pretty safe too (though you shouldn't need the AsUTFUnsafe > > calls). > > So this should be done in a path_util_unittest file? > I started adding one and ran into a conflict with this file. Would I just be > able to rename the class to ExtensionsPathUtilTest, or something to that effect? > The conflict is probably because we don't allow the same target in the same .gypi file or something like that (path_util is in chrome_browser_extensions.gypi, but all the unittests are in the same file. I think that extension_path_util_unittest.cc is probably fine - just make a note in both files.
On 2014/08/04 23:55:36, kalman wrote: > I agree that at the very least we should add a regression test for an empty file > path, and link to that API test which supposedly tests the rest - and your > example test looks pretty safe too (though you shouldn't need the AsUTFUnsafe > calls). Can I also ask for a bit of clarification on this? https://codereview.chromium.org/439873002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/path_util.cc (right): https://codereview.chromium.org/439873002/diff/20001/chrome/browser/extension... chrome/browser/extensions/path_util.cc:23: namespace OS_MACOSX { On 2014/08/05 00:11:05, kalman wrote: > why do you have this extra namespace? it can stay in an anonymous namespace. I thought this was what you meant by "specific OS_MACOSX" namespace. Did you just mean an anonymous namespace with a #ifdef wrapped around it?
Here's a test file with a couple of basic tests. When you say we should link to that API test, what do you mean exactly? Regarding a couple of smaller things, should this test be in the extensions namespace (as it is now) since it's the "extensions path util" test?
On 2014/08/05 20:41:10, gpdavis wrote: > Here's a test file with a couple of basic tests. When you say we should link to > that API test, what do you mean exactly? Like say: // Basic unittests for WhateverTheFunctionIsCalled. // For legacy reasons, it's tested more in WhateverThatTest.IsCalled. > > Regarding a couple of smaller things, should this test be in the extensions > namespace (as it is now) since it's the "extensions path util" test? Yep. and to reply to your MACOSX question - yes I suppose I wasn't thinking straight and should have said ifdef not namespace.
On 2014/08/05 20:42:41, kalman wrote: > On 2014/08/05 20:41:10, gpdavis wrote: > > Here's a test file with a couple of basic tests. When you say we should link > to > > that API test, what do you mean exactly? > > Like say: > > // Basic unittests for WhateverTheFunctionIsCalled. > // For legacy reasons, it's tested more in WhateverThatTest.IsCalled. > > > > > Regarding a couple of smaller things, should this test be in the extensions > > namespace (as it is now) since it's the "extensions path util" test? > > Yep. > > and to reply to your MACOSX question - yes I suppose I wasn't thinking straight > and should have said ifdef not namespace. Right on. How's this look? I also documented the fact that extension_path_util_unittest.cc is the test file for path_util.cc above the PrettifyPath declaration.
https://codereview.chromium.org/439873002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_path_util_unittest.cc (right): https://codereview.chromium.org/439873002/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_path_util_unittest.cc:9: #include "chrome/test/base/testing_profile.h" ?? https://codereview.chromium.org/439873002/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_path_util_unittest.cc:17: // For legacy reasons, it's tested more in: nit: remove the colon and the indentation. https://codereview.chromium.org/439873002/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_path_util_unittest.cc:19: class ExtensionPathUtilTest : public testing::Test { This doesn't do anything. Just do TEST(ExtensionPathUtilTest, EmptyPath) https://codereview.chromium.org/439873002/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_path_util_unittest.cc:22: namespace { We don't need to put these tests in their own namespace. https://codereview.chromium.org/439873002/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_path_util_unittest.cc:31: TEST_F(ExtensionPathUtilTest, HomeDirectory) { Rename this to be "BasicPrettifyPathTest" or something like that. https://codereview.chromium.org/439873002/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_path_util_unittest.cc:34: ASSERT_EQ(FilePath(FILE_PATH_LITERAL("~")).AppendASCII("foo"), prettified); This is a little condensed for readability in a test (when I suggested the test, I was being lazy and minimizing what I had to type). We should make it more clear what we're testing. Also, we should test that the path before prettifying doesn't equal the expected result (otherwise, our second expectation means nothing). Also, we should test an extra layer, e.g. ~/foo/bar (no need to go deeper than two), and no addition (i.e., home_path = ~). https://codereview.chromium.org/439873002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/path_util.cc (right): https://codereview.chromium.org/439873002/diff/60001/chrome/browser/extension... chrome/browser/extensions/path_util.cc:79: base::FilePath display_path = base::FilePath::FromUTF8Unsafe("~"); nit: prefer to make this a FILE_PATH_LITERAL, and use it in both mac-specific and not. https://codereview.chromium.org/439873002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/path_util.h (right): https://codereview.chromium.org/439873002/diff/60001/chrome/browser/extension... chrome/browser/extensions/path_util.h:19: // This function is tested in extension_path_util_unittest.cc spell out the whole path, so that handy util like move_source_file work.
https://codereview.chromium.org/439873002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_path_util_unittest.cc (right): https://codereview.chromium.org/439873002/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_path_util_unittest.cc:9: #include "chrome/test/base/testing_profile.h" On 2014/08/05 23:40:35, Devlin wrote: > ?? Oops-- I migrated the includes from another file, and missed this one when I deleted the ones I didn't need. Done. https://codereview.chromium.org/439873002/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_path_util_unittest.cc:17: // For legacy reasons, it's tested more in: On 2014/08/05 23:40:35, Devlin wrote: > nit: remove the colon and the indentation. Done. https://codereview.chromium.org/439873002/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_path_util_unittest.cc:19: class ExtensionPathUtilTest : public testing::Test { On 2014/08/05 23:40:35, Devlin wrote: > This doesn't do anything. Just do TEST(ExtensionPathUtilTest, EmptyPath) Done. https://codereview.chromium.org/439873002/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_path_util_unittest.cc:22: namespace { On 2014/08/05 23:40:35, Devlin wrote: > We don't need to put these tests in their own namespace. Done. https://codereview.chromium.org/439873002/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_path_util_unittest.cc:31: TEST_F(ExtensionPathUtilTest, HomeDirectory) { On 2014/08/05 23:40:35, Devlin wrote: > Rename this to be "BasicPrettifyPathTest" or something like that. Done. https://codereview.chromium.org/439873002/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_path_util_unittest.cc:34: ASSERT_EQ(FilePath(FILE_PATH_LITERAL("~")).AppendASCII("foo"), prettified); On 2014/08/05 23:40:35, Devlin wrote: > This is a little condensed for readability in a test (when I suggested the test, > I was being lazy and minimizing what I had to type). We should make it more > clear what we're testing. > > Also, we should test that the path before prettifying doesn't equal the expected > result (otherwise, our second expectation means nothing). > > Also, we should test an extra layer, e.g. ~/foo/bar (no need to go deeper than > two), and no addition (i.e., home_path = ~). Done. https://codereview.chromium.org/439873002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/path_util.cc (right): https://codereview.chromium.org/439873002/diff/60001/chrome/browser/extension... chrome/browser/extensions/path_util.cc:79: base::FilePath display_path = base::FilePath::FromUTF8Unsafe("~"); On 2014/08/05 23:40:35, Devlin wrote: > nit: prefer to make this a FILE_PATH_LITERAL, and use it in both mac-specific > and not. You mean like this, right? base::FilePath display_path = base::FilePath(FILE_PATH_LITERAL("~")); https://codereview.chromium.org/439873002/diff/60001/chrome/browser/extension... File chrome/browser/extensions/path_util.h (right): https://codereview.chromium.org/439873002/diff/60001/chrome/browser/extension... chrome/browser/extensions/path_util.h:19: // This function is tested in extension_path_util_unittest.cc On 2014/08/05 23:40:35, Devlin wrote: > spell out the whole path, so that handy util like move_source_file work. Done.
https://codereview.chromium.org/439873002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_path_util_unittest.cc (right): https://codereview.chromium.org/439873002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_path_util_unittest.cc:21: FilePath actual; "actual" is a bit misleading, since they are both the "actual" path - or at least should be. Perhaps "initial" or even "unprettified". https://codereview.chromium.org/439873002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_path_util_unittest.cc:23: ASSERT_EQ(actual, prettified); This can be an EXPECT, as can all of the others. https://codereview.chromium.org/439873002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_path_util_unittest.cc:29: ASSERT_EQ(FilePath(FILE_PATH_LITERAL("~")), prettified); Save the FILE_PATH_LITERAL. https://codereview.chromium.org/439873002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/path_util.cc (right): https://codereview.chromium.org/439873002/diff/80001/chrome/browser/extension... chrome/browser/extensions/path_util.cc:70: display_path = base::FilePath(FILE_PATH_LITERAL("~")); Save the FILE_PATH_LITERAL so you don't have to keep redeclaring it (it'd be fine the anon namespace above).
https://codereview.chromium.org/439873002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_path_util_unittest.cc (right): https://codereview.chromium.org/439873002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_path_util_unittest.cc:21: FilePath actual; On 2014/08/06 23:35:56, Devlin wrote: > "actual" is a bit misleading, since they are both the "actual" path - or at > least should be. Perhaps "initial" or even "unprettified". Done. https://codereview.chromium.org/439873002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_path_util_unittest.cc:23: ASSERT_EQ(actual, prettified); On 2014/08/06 23:35:56, Devlin wrote: > This can be an EXPECT, as can all of the others. Done. https://codereview.chromium.org/439873002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_path_util_unittest.cc:29: ASSERT_EQ(FilePath(FILE_PATH_LITERAL("~")), prettified); On 2014/08/06 23:35:55, Devlin wrote: > Save the FILE_PATH_LITERAL. Done. https://codereview.chromium.org/439873002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/path_util.cc (right): https://codereview.chromium.org/439873002/diff/80001/chrome/browser/extension... chrome/browser/extensions/path_util.cc:70: display_path = base::FilePath(FILE_PATH_LITERAL("~")); On 2014/08/06 23:35:56, Devlin wrote: > Save the FILE_PATH_LITERAL so you don't have to keep redeclaring it (it'd be > fine the anon namespace above). Done. https://codereview.chromium.org/439873002/diff/80002/chrome/browser/extension... File chrome/browser/extensions/extension_path_util_unittest.cc (right): https://codereview.chromium.org/439873002/diff/80002/chrome/browser/extension... chrome/browser/extensions/extension_path_util_unittest.cc:20: const FilePath::CharType kHomeShortcut[] = FILE_PATH_LITERAL("~"); Is this okay to put here, or should it go up in an anonymous namespace?
lg after these two small changes. https://codereview.chromium.org/439873002/diff/80002/chrome/browser/extension... File chrome/browser/extensions/extension_path_util_unittest.cc (right): https://codereview.chromium.org/439873002/diff/80002/chrome/browser/extension... chrome/browser/extensions/extension_path_util_unittest.cc:20: const FilePath::CharType kHomeShortcut[] = FILE_PATH_LITERAL("~"); On 2014/08/07 01:20:06, gpdavis wrote: > Is this okay to put here, or should it go up in an anonymous namespace? It's fine here, but it doesn't really need to be an array. https://codereview.chromium.org/439873002/diff/80002/chrome/browser/extension... File chrome/browser/extensions/path_util.cc (right): https://codereview.chromium.org/439873002/diff/80002/chrome/browser/extension... chrome/browser/extensions/path_util.cc:44: const base::FilePath::CharType kHomeShortcut[] = FILE_PATH_LITERAL("~"); no need for an array, right?
https://codereview.chromium.org/439873002/diff/80002/chrome/browser/extension... File chrome/browser/extensions/path_util.cc (right): https://codereview.chromium.org/439873002/diff/80002/chrome/browser/extension... chrome/browser/extensions/path_util.cc:44: const base::FilePath::CharType kHomeShortcut[] = FILE_PATH_LITERAL("~"); On 2014/08/07 15:53:45, Devlin wrote: > no need for an array, right? It doesn't compile if it's just a single CharType, since we can construct FilePaths from StringTypes, but not CharTypes. (Line 73, for example, would be trying to construct a FilePath out of a CharType). It'll work as a pointer, though. Which would you prefer?
https://codereview.chromium.org/439873002/diff/80002/chrome/browser/extension... File chrome/browser/extensions/path_util.cc (right): https://codereview.chromium.org/439873002/diff/80002/chrome/browser/extension... chrome/browser/extensions/path_util.cc:44: const base::FilePath::CharType kHomeShortcut[] = FILE_PATH_LITERAL("~"); On 2014/08/07 18:19:21, gpdavis wrote: > On 2014/08/07 15:53:45, Devlin wrote: > > no need for an array, right? > > It doesn't compile if it's just a single CharType, since we can construct > FilePaths from StringTypes, but not CharTypes. (Line 73, for example, would be > trying to construct a FilePath out of a CharType). > > It'll work as a pointer, though. Which would you prefer? Hmm. How annoying. I wonder why we don't have a FilePath(FilePath::CharType). Ah well. Leaving it as an array is fine.
lgtm
The CQ bit was checked by gpdavis.chromium@gmail.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gpdavis.chromium@gmail.com/439873002/8...
Message was sent while issue was closed.
Change committed as 288187 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
