Displaying human-readable Android credentials on Android OS.

components/password_manager/core/browser/password_ui_utils_unittest.cc

Index: components/password_manager/core/browser/password_ui_utils_unittest.cc
diff --git a/components/password_manager/core/browser/password_ui_utils_unittest.cc b/components/password_manager/core/browser/password_ui_utils_unittest.cc
index 780fa8c50e468c9fde8c98658af768d3efbd845c..6d7e0748b8d444ecee8f980dd1513021d81934d0 100644
--- a/components/password_manager/core/browser/password_ui_utils_unittest.cc
+++ b/components/password_manager/core/browser/password_ui_utils_unittest.cc
@@ -93,4 +93,16 @@ TEST(GetShownOriginAndLinkUrlTest, OriginFromNonAndroidForm) {
   EXPECT_EQ(form.origin, link_url);
 }
 
+TEST(SplitByDotAndReverseTest, ReversedHostname) {
+  const struct {
+    const base::StringPiece input;

[Bernhard Bauer, 2016/06/13 09:55:07]

Just FYI: In unit tests we don't care so much, but in production code you should
use const char* here, because StringPiece and std::string have constructors, and
we don't want static initializers in production code.

[vabr (Chromium), 2016/06/13 10:01:03]

But here there is no static variable, kTestCases is a local one, its life span
is limited to the current function.

[Bernhard Bauer, 2016/06/13 10:34:01]

(Which is also a thing I would avoid in production code ☺)

[dozsa, 2016/06/13 12:36:18]

Done.

+    const std::string output;
+  } kTestCases[] = {{"com.example.android", "android.example.com"},
+                    {"com.example.mobile", "mobile.example.com"},
+                    {"net.example.subdomain", "subdomain.example.net"}};
+  for (const auto& test_case : kTestCases) {

[Bernhard Bauer, 2016/06/13 09:34:59]

FWIW, I think in this case you'd be better served by unrolling the loop:

  EXPECT_EQ("android.example.com", SplitByDotAndReverse("com.example.android"));
  ...

For example, that will tell you the expected and actual value in case of a
failure.

[vabr (Chromium), 2016/06/13 09:45:21]

Bernhard, this seems to be the case already. EXPECT_EQ will print out both
arguments and the values they evaluate to.

My personal preference here is for the current code, because if we need to add
more pairs, reading the test cases array definition is easier than the lines
cluttered with EXPECT_EQ and SplitByDotAndReverse.

[Bernhard Bauer, 2016/06/13 09:55:07]

Yes, but it will print the unevaluated arguments literally as what is passed to
the EXPECT_EQ macro, i.e. "SplitByDotAndReverse(test_case.input)".

[vabr (Chromium), 2016/06/13 10:01:03]

But there is no advantage in that, as long as SplitByDotAndReverse is injective
on the domain of strings in the test cases.

[Bernhard Bauer, 2016/06/13 10:34:01]

I personally would find it more readable than having to infer the input from the
expected output, but I don't feel strongly enough about it to object.

[dozsa, 2016/06/13 12:36:18]

I used the unit test for GetShownOrigin as a model for this test. Conform to
vabr@'s comments, I've left the test as is. 

+    EXPECT_EQ(test_case.output, SplitByDotAndReverse(test_case.input));
+  }
+}
+
 }  // namespace password_manager