Use gtest failures and EXPECTS instead of returning a failure enum in test target base/test/test_sh…

Use gtest failures and EXPECTS instead of returning a failure enum in test target base/test/test_shortcut_win.cc

BUG=148539
TEST=base_unittests.exe --gtest_filter=ShortcutTest*
installer_util_unittests.exe --gtest_filter=ShellUtilShortcutTest*
unit_tests --gtest_filter=ProfileShortcutManagerTest*

Locally tried a test meant to fail for every property:
TEST_F(ShortcutTest, CreateShortcutVerifyPropertiesWrong) {
  ASSERT_TRUE(base::win::CreateOrUpdateShortcutLink(
      link_file_, link_properties_, base::win::SHORTCUT_CREATE_ALWAYS));

  base::win::ShortcutProperties wrong_properties = link_properties_;
  wrong_properties.set_target(FilePath(L"g:\\src\\component\\.gclient"));
  wrong_properties.set_working_dir(temp_dir_2_.path());
  wrong_properties.set_arguments(string16());
  wrong_properties.set_description(L"foo");
  wrong_properties.set_icon(temp_dir_2_.path().Append(L"joe.ico"), 8);
  wrong_properties.set_app_id(L"JOE");
  wrong_properties.set_dual_mode(true);
  base::win::ValidateShortcut(link_file_, wrong_properties);
}

And got expected failure output for every property:
[ RUN      ] ShortcutTest.CreateShortcutVerifyPropertiesWrong
g:\src\component\src\base\test\test_shortcut_win.cc(54): error: Value of: long_actual_path
  Actual: G:\src\temp\scoped_dir14192_645\Target 1.txt
Expected: long_expected_path
Which is: g:\src\component\.gclient
g:\src\component\src\base\test\test_shortcut_win.cc(54): error: Value of: long_actual_path
  Actual: G:\src\temp\scoped_dir14192_645
Expected: long_expected_path
Which is: G:\src\temp\scoped_dir14192_27681
g:\src\component\src\base\test\test_shortcut_win.cc(95): error: Value of: read_arguments
  Actual: L"--magic --awesome"
Expected: properties.arguments
Which is: L""
g:\src\component\src\base\test\test_shortcut_win.cc(101): error: Value of: read_description
  Actual: L"Chrome is awesome."
Expected: properties.description
Which is: L"foo"
g:\src\component\src\base\test\test_shortcut_win.cc(43): error: Expected: (0U) != (::GetLongPathNameW( expected_path.val
ue().c_str(), long_expected_path_chars, 260)), actual: 0 vs 0
Failed to get LongPathName of G:\src\temp\scoped_dir14192_27681\joe.ico
g:\src\component\src\base\test\test_shortcut_win.cc(108): error: Value of: read_icon_index
  Actual: 4
Expected: properties.icon_index
Which is: 8
g:\src\component\src\base\test\test_shortcut_win.cc(129): error: Value of: read_app_id
  Actual: L"Chrome"
Expected: properties.app_id
Which is: L"JOE"
g:\src\component\src\base\test\test_shortcut_win.cc(138): error: Value of: static_cast<bool>(read_dual_mode)
  Actual: false
Expected: properties.dual_mode
Which is: true
[  FAILED  ] ShortcutTest.CreateShortcutVerifyPropertiesWrong (37 ms)

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

[robertshield, 2012-09-25 21:06:34]

http://codereview.chromium.org/10996005/diff/2001/base/test/test_shortcut_win.cc
File base/test/test_shortcut_win.cc (right):

http://codereview.chromium.org/10996005/diff/2001/base/test/test_shortcut_win...
base/test/test_shortcut_win.cc:87: UTF16ToUTF8(shortcut_path.value())).c_str());
I don't see any other precedent in the code base for directly using macros
defined in gtest-internal.h. I'm not sure they are supposed to be part of
GTest's API. What to say to instead doing:
EXPECT_TRUE(SUCCEEDED(hr)) << "Log message";

http://codereview.chromium.org/10996005/diff/2001/base/test/test_shortcut_win...
base/test/test_shortcut_win.cc:99: UTF16ToUTF8(shortcut_path.value())).c_str());
similar comment here, and in the other places in the file.

http://codereview.chromium.org/10996005/diff/2001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

http://codereview.chromium.org/10996005/diff/2001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:39: FAIL();
Suggest adding a log message to the failure here. Also, please consider the more
conventional factorisation: 

EXPECT_TRUE(ShellUtil::GetDesktopPath(false, path)) << "Log message;
EXPECT_TRUE(ShellUtil::GetChromeShortcutName(dist, false, profile_name,
&shortcut_name)) << " Log message";

[gab, 2012-09-25 22:10:00]

Indeed, thanks for making the CL better :).

PTAL

Cheers,
Gab

http://codereview.chromium.org/10996005/diff/2001/base/test/test_shortcut_win.cc
File base/test/test_shortcut_win.cc (right):

http://codereview.chromium.org/10996005/diff/2001/base/test/test_shortcut_win...
base/test/test_shortcut_win.cc:87: UTF16ToUTF8(shortcut_path.value())).c_str());
On 2012/09/25 21:06:34, robertshield wrote:
> I don't see any other precedent in the code base for directly using macros
> defined in gtest-internal.h. I'm not sure they are supposed to be part of
> GTest's API. What to say to instead doing:
> EXPECT_TRUE(SUCCEEDED(hr)) << "Log message";

Done all over the place as suggested. Only added logs where useful.

http://codereview.chromium.org/10996005/diff/2001/base/test/test_shortcut_win...
base/test/test_shortcut_win.cc:99: UTF16ToUTF8(shortcut_path.value())).c_str());
On 2012/09/25 21:06:34, robertshield wrote:
> similar comment here, and in the other places in the file.

Done.

http://codereview.chromium.org/10996005/diff/2001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right):

http://codereview.chromium.org/10996005/diff/2001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:39: FAIL();
On 2012/09/25 21:06:34, robertshield wrote:
> Suggest adding a log message to the failure here. Also, please consider the
more
> conventional factorisation: 
> 
> EXPECT_TRUE(ShellUtil::GetDesktopPath(false, path)) << "Log message;
> EXPECT_TRUE(ShellUtil::GetChromeShortcutName(dist, false, profile_name,
> &shortcut_name)) << " Log message";

Done (but with asserts as it should never fail and it doesn't make sense to keep
going if it does).

[robertshield, 2012-09-25 23:47:05]

lgtm

[gab, 2012-09-26 03:03:06]

Thanks,

@brettw: base OWNER approval + chrome/browser/profiles side-effect (feel free to
add sail@ if you prefer an OWNER from that directory to take a look at the
side-effects there).

Feel free to CQ if it LGTY :)!

Cheers,
Gab

[gab, 2012-09-26 16:23:28]

I was using ASSERTs to do control flow (ASSERTs kindly return only from the
current method, not the whole test, on failure), but robertshield opposed to
this with passion, so PTAL at patch set 4 where I use only EXPECTs and return;'s
to generate the same behavior.

Thanks,
Gab

PS: The log output of the bad test is now:
[ RUN      ] ShortcutTest.CreateShortcutVerifyPropertiesWrong
g:\src\component\src\base\test\test_shortcut_win.cc(54): error: Value of:
long_actual_path
  Actual: G:\src\temp\scoped_dir8852_26055\Target 1.txt
Expected: long_expected_path
Which is: g:\src\component\.gclient
g:\src\component\src\base\test\test_shortcut_win.cc(54): error: Value of:
long_actual_path
  Actual: G:\src\temp\scoped_dir8852_26055
Expected: long_expected_path
Which is: G:\src\temp\scoped_dir8852_4328
g:\src\component\src\base\test\test_shortcut_win.cc(105): error: Value of:
read_arguments
  Actual: L"--magic --awesome"
Expected: properties.arguments
Which is: L""
g:\src\component\src\base\test\test_shortcut_win.cc(111): error: Value of:
read_description
  Actual: L"Chrome is awesome."
Expected: properties.description
Which is: L"foo"
g:\src\component\src\base\test\test_shortcut_win.cc(43): error: Expected: (0U)
!= (::GetLongPathNameW( expected_path.val
ue().c_str(), long_expected_path_chars, 260)), actual: 0 vs 0
Failed to get LongPathName of G:\src\temp\scoped_dir8852_4328\joe.ico
g:\src\component\src\base\test\test_shortcut_win.cc(51): error: Value of:
long_expected_path.empty()
  Actual: true
Expected: false
g:\src\component\src\base\test\test_shortcut_win.cc(54): error: Value of:
long_actual_path
  Actual: G:\src\temp\scoped_dir8852_26055\Target 1.txt
Expected: long_expected_path
Which is:
g:\src\component\src\base\test\test_shortcut_win.cc(118): error: Value of:
read_icon_index
  Actual: 4
Expected: properties.icon_index
Which is: 8
g:\src\component\src\base\test\test_shortcut_win.cc(141): error: Value of:
read_app_id
  Actual: L"Chrome"
Expected: properties.app_id
Which is: L"JOE"
g:\src\component\src\base\test\test_shortcut_win.cc(150): error: Value of:
static_cast<bool>(read_dual_mode)
  Actual: false
Expected: properties.dual_mode
Which is: true
[  FAILED  ] ShortcutTest.CreateShortcutVerifyPropertiesWrong (40 ms)

[robertshield, 2012-09-26 16:55:45]

LGTM++ Thanks for doing this :-)

[brettw, 2012-09-26 18:07:37]

lgtm rubbertamp

[commit-bot: I haz the power, 2012-09-26 18:09:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/10996005/4004

[commit-bot: I haz the power, 2012-09-26 18:25:59]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-26 18:28:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/10996005/4004

[commit-bot: I haz the power, 2012-09-26 18:36:05]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-26 18:56:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/10996005/4004

[commit-bot: I haz the power, 2012-09-26 22:21:09]

Change committed as 158909