Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18569002:
Clean up ASSERT/EXPECT in file_util_unittest.cc. (Closed)
|
Created:
7 years, 5 months ago by gavinp Modified:
7 years, 5 months ago CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionClean up ASSERT/EXPECT in file_util_unittest.cc.
ASSERT only on functions with side effects that are depended on,
EXPECT otherwise allows tests to show more failures, helping
debugging.
R=mark@chromium.org,brettw
BUG=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209996
Patch Set 1 #
Total comments: 6
Patch Set 2 : remediate #Messages
Total messages: 11 (0 generated)
mark, PTAL; brettw would be first choice but he's on vacation. brettw, FYI. I came across these while working on a related change.
Wrong Mark I think, I'm not on this project! Sent from my iPhone On 2 Jul 2013, at 19:32, gavinp@chromium.org wrote: > Reviewers: brettw, mark, > > Message: > mark, PTAL; brettw would be first choice but he's on vacation. > > brettw, FYI. > > I came across these while working on a related change. > > Description: > Clean up ASSERT/EXPECT in file_util_unittest.cc. > > ASSERT only on functions with side effects that are depended on, > EXPECT otherwise allows tests to show more failures, helping > debugging. > > R=mark,brettw > BUG=None > > Please review this at https://codereview.chromium.org/18569002/ > > SVN Base: svn://svn.chromium.org/chrome/trunk/src > > Affected files: > M base/file_util_unittest.cc > > > Index: base/file_util_unittest.cc > diff --git a/base/file_util_unittest.cc b/base/file_util_unittest.cc > index cbe8f26ea0545bdc06dc6c29afc026ae3a74fe5a..910e84c493f3f98c406e5ce83beaf8650cf37bde 100644 > --- a/base/file_util_unittest.cc > +++ b/base/file_util_unittest.cc > @@ -664,28 +664,27 @@ TEST_F(FileUtilTest, CreateAndReadSymlinks) { > ASSERT_TRUE(file_util::CreateSymbolicLink(link_to, link_from)) > << "Failed to create file symlink."; > > - // If we created the link properly, we should be able to read the > - // contents through it. > + // If we created the link properly, we should be able to read the contents > + // through it. > std::wstring contents = ReadTextFile(link_from); > - ASSERT_EQ(contents, bogus_content); > + EXPECT_EQ(contents, bogus_content); > > FilePath result; > - ASSERT_TRUE(file_util::ReadSymbolicLink(link_from, &result)); > - ASSERT_EQ(link_to.value(), result.value()); > + EXPECT_TRUE(file_util::ReadSymbolicLink(link_from, &result)); > + EXPECT_EQ(link_to.value(), result.value()); > > // Link to a directory. > link_from = temp_dir_.path().Append(FPL("from_dir")); > link_to = temp_dir_.path().Append(FPL("to_dir")); > - file_util::CreateDirectory(link_to); > - > + ASSERT_TRUE(file_util::CreateDirectory(link_to)); > ASSERT_TRUE(file_util::CreateSymbolicLink(link_to, link_from)) > << "Failed to create directory symlink."; > > // Test failures. > - ASSERT_FALSE(file_util::CreateSymbolicLink(link_to, link_to)); > - ASSERT_FALSE(file_util::ReadSymbolicLink(link_to, &result)); > + EXPECT_FALSE(file_util::CreateSymbolicLink(link_to, link_to)); > + EXPECT_FALSE(file_util::ReadSymbolicLink(link_to, &result)); > FilePath missing = temp_dir_.path().Append(FPL("missing")); > - ASSERT_FALSE(file_util::ReadSymbolicLink(missing, &result)); > + EXPECT_FALSE(file_util::ReadSymbolicLink(missing, &result)); > } > > // The following test of NormalizeFilePath() require that we create a symlink. > @@ -705,20 +704,19 @@ TEST_F(FileUtilTest, NormalizeFilePathSymlinks) { > << "Failed to create file symlink."; > > // Check that NormalizeFilePath sees the link. > - ASSERT_TRUE(file_util::NormalizeFilePath(link_from, &normalized_path)); > - ASSERT_TRUE(link_to != link_from); > - ASSERT_EQ(link_to.BaseName().value(), normalized_path.BaseName().value()); > - ASSERT_EQ(link_to.BaseName().value(), normalized_path.BaseName().value()); > + EXPECT_TRUE(file_util::NormalizeFilePath(link_from, &normalized_path)); > + EXPECT_NE(link_from, link_to); > + EXPECT_EQ(link_to.BaseName().value(), normalized_path.BaseName().value()); > + EXPECT_EQ(link_to.BaseName().value(), normalized_path.BaseName().value()); > > // Link to a directory. > link_from = temp_dir_.path().Append(FPL("from_dir")); > link_to = temp_dir_.path().Append(FPL("to_dir")); > - file_util::CreateDirectory(link_to); > - > + ASSERT_TRUE(file_util::CreateDirectory(link_to)); > ASSERT_TRUE(file_util::CreateSymbolicLink(link_to, link_from)) > << "Failed to create directory symlink."; > > - ASSERT_FALSE(file_util::NormalizeFilePath(link_from, &normalized_path)) > + EXPECT_FALSE(file_util::NormalizeFilePath(link_from, &normalized_path)) > << "Links to directories should return false."; > > // Test that a loop in the links causes NormalizeFilePath() to return false. > @@ -730,7 +728,7 @@ TEST_F(FileUtilTest, NormalizeFilePathSymlinks) { > << "Failed to create loop symlink b."; > > // Infinite loop! > - ASSERT_FALSE(file_util::NormalizeFilePath(link_from, &normalized_path)); > + EXPECT_FALSE(file_util::NormalizeFilePath(link_from, &normalized_path)); > } > #endif // defined(OS_POSIX) > > >
- wrong mark + right mark Mark, PTAL.
Why prefer Brett? All of the OWNERS in here are totally competent to review any change. https://codereview.chromium.org/18569002/diff/1/base/file_util_unittest.cc File base/file_util_unittest.cc (right): https://codereview.chromium.org/18569002/diff/1/base/file_util_unittest.cc#ne... base/file_util_unittest.cc:670: EXPECT_EQ(contents, bogus_content); ASSERT_EQ and EXPECT_EQ take arguments as “expected, actual”. On _EQ comparison failure, the message identifies which value was the expected one and which was the actual one, so it’s nice to get this right. Since |bogus_content| is just a const, and the “actual” here is what’s returned from ReadTextFile, |contents|, I think the arguments should be switched. https://codereview.chromium.org/18569002/diff/1/base/file_util_unittest.cc#ne... base/file_util_unittest.cc:674: EXPECT_EQ(link_to.value(), result.value()); This uses “result” which won’t be set if ReadSymbolicLink failed, so I think ASSERT_TRUE is correct on the preceding line. https://codereview.chromium.org/18569002/diff/1/base/file_util_unittest.cc#ne... base/file_util_unittest.cc:709: EXPECT_EQ(link_to.BaseName().value(), normalized_path.BaseName().value()); normalized_path won’t be set unless NormalizeFilePath worked, so that one should stay an ASSERT. But I’d call NormalizeFilePath after the EXPECT_NE(link_from, link_to) check, since the EXPECT_NE check can still run regardless of the status of the ASSERT_TRUE. And I’d move the declaration of normalized_path down to the line immediately before where it’s used.
Mark, thanks for the careful review. I know there's a silliness to refactoring this kind of existing code, but I particularly appreciate that you paid enough attention to fix my fixes. Thank you. https://codereview.chromium.org/18569002/diff/1/base/file_util_unittest.cc File base/file_util_unittest.cc (right): https://codereview.chromium.org/18569002/diff/1/base/file_util_unittest.cc#ne... base/file_util_unittest.cc:670: EXPECT_EQ(contents, bogus_content); On 2013/07/02 20:24:53, Mark Mentovai wrote: > ASSERT_EQ and EXPECT_EQ take arguments as “expected, actual”. On _EQ comparison > failure, the message identifies which value was the expected one and which was > the actual one, so it’s nice to get this right. Since |bogus_content| is just a > const, and the “actual” here is what’s returned from ReadTextFile, |contents|, I > think the arguments should be switched. Done. https://codereview.chromium.org/18569002/diff/1/base/file_util_unittest.cc#ne... base/file_util_unittest.cc:674: EXPECT_EQ(link_to.value(), result.value()); On 2013/07/02 20:24:53, Mark Mentovai wrote: > This uses “result” which won’t be set if ReadSymbolicLink failed, so I think > ASSERT_TRUE is correct on the preceding line. Done. https://codereview.chromium.org/18569002/diff/1/base/file_util_unittest.cc#ne... base/file_util_unittest.cc:709: EXPECT_EQ(link_to.BaseName().value(), normalized_path.BaseName().value()); On 2013/07/02 20:24:53, Mark Mentovai wrote: > normalized_path won’t be set unless NormalizeFilePath worked, so that one should > stay an ASSERT. > > But I’d call NormalizeFilePath after the EXPECT_NE(link_from, link_to) check, > since the EXPECT_NE check can still run regardless of the status of the > ASSERT_TRUE. And I’d move the declaration of normalized_path down to the line > immediately before where it’s used. Done and done.
On 2013/07/02 20:24:53, Mark Mentovai wrote: > Why prefer Brett? All of the OWNERS in here are totally competent to review any > change. I preferred Brett because his blame was deeper on this file. I concur that all OWNERS are competent, but I like to choose the OWNER with the most work in the area. Was my way of choosing appropriate? Was the way I phrased it appropriate? Thanks. > > https://codereview.chromium.org/18569002/diff/1/base/file_util_unittest.cc > File base/file_util_unittest.cc (right): > > https://codereview.chromium.org/18569002/diff/1/base/file_util_unittest.cc#ne... > base/file_util_unittest.cc:670: EXPECT_EQ(contents, bogus_content); > ASSERT_EQ and EXPECT_EQ take arguments as “expected, actual”. On _EQ comparison > failure, the message identifies which value was the expected one and which was > the actual one, so it’s nice to get this right. Since |bogus_content| is just a > const, and the “actual” here is what’s returned from ReadTextFile, |contents|, I > think the arguments should be switched. > > https://codereview.chromium.org/18569002/diff/1/base/file_util_unittest.cc#ne... > base/file_util_unittest.cc:674: EXPECT_EQ(link_to.value(), result.value()); > This uses “result” which won’t be set if ReadSymbolicLink failed, so I think > ASSERT_TRUE is correct on the preceding line. > > https://codereview.chromium.org/18569002/diff/1/base/file_util_unittest.cc#ne... > base/file_util_unittest.cc:709: EXPECT_EQ(link_to.BaseName().value(), > normalized_path.BaseName().value()); > normalized_path won’t be set unless NormalizeFilePath worked, so that one should > stay an ASSERT. > > But I’d call NormalizeFilePath after the EXPECT_NE(link_from, link_to) check, > since the EXPECT_NE check can still run regardless of the status of the > ASSERT_TRUE. And I’d move the declaration of normalized_path down to the line > immediately before where it’s used.
Oh, that’s fine. It seemed that you were picking simply based on OWNERS and not on blame.
On 2013/07/03 13:23:07, Mark Mentovai wrote: > Oh, that’s fine. It seemed that you were picking simply based on OWNERS and not > on blame. I'll be more clear in the future about why I made the choices I did. Does my latest upload make the cut?
Yup, this one’s good. LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/18569002/20001
Message was sent while issue was closed.
Change committed as 209996 |
