[i18n streaming] improve unit tests

content/browser/webui/i18n_source_stream_unittest.cc

Index: content/browser/webui/i18n_source_stream_unittest.cc
diff --git a/content/browser/webui/i18n_source_stream_unittest.cc b/content/browser/webui/i18n_source_stream_unittest.cc
index d031240be17df763e6c65925d9577d78e7a8e58f..dffca57bc4c3f64b8e4f8849f390d7f627245e7f 100644
--- a/content/browser/webui/i18n_source_stream_unittest.cc
+++ b/content/browser/webui/i18n_source_stream_unittest.cc
@@ -19,27 +19,50 @@ namespace {
 const int kBufferSize = 256;
 const int kSmallBufferSize = 1;
 
-const int kShortReplacementOffset = 5;
-const char kShortReplacementKey[] = "a";
-const char kShortReplacementToken[] = "$i18n{a}";
-const char kShortReplacementValue[] = "short";
+struct I18nTest {
+  I18nTest(const char* input, const char* expected_output)

[mmenke, 2017/01/04 16:12:50]

To avoid static initializers, I think this needs to be:

constexpr 18nTest(const char* input, const char* expected_output)

Believe you need to replace the consts below with constexpr as well.

[dschuyler, 2017/01/04 23:06:53]

Done.

+      : input(input), expected_output(expected_output) {}
 
-const int kLongReplacementOffset = 33;
-const char kLongReplacementKey[] = "aLongerReplacementName";
-const char kLongReplacementToken[] = "$i18n{aLongerReplacementName}";
-const char kLongReplacementValue[] = "second replacement";
+  const char* input;
+  const char* expected_output;
+};
+
+const I18nTest kTestNoReplacements =
+    I18nTest("This text has no i18n replacements.",
+             "This text has no i18n replacements.");
+
+const I18nTest kTestTagAtEndOfLine = I18nTest("test with tag at end of line $",
+                                              "test with tag at end of line $");
+
+const I18nTest kTestOneReplacement = I18nTest("$i18n{alpha}", "apple");
+
+const I18nTest kTestOneReplacementPlus =
+    I18nTest("Extra text $i18n{alpha}.", "Extra text apple.");
+
+const I18nTest kTestThreeReplacements =
+    I18nTest("$i18n{alpha}^$i18n{beta}_$i18n{gamma}", "apple^banana_carrot");
+
+const I18nTest kTestExtraBraces =
+    I18nTest("($i18n{alpha})^_^_^_^_$i18n{beta}_beta_$i18n{gamma}}}}}}",
+             "(apple)^_^_^_^_banana_beta_carrot}}}}}");
 
-const int kSourceSize =
-    50 + arraysize(kShortReplacementToken) + arraysize(kLongReplacementToken);
-const int kResultSize =
-    50 + arraysize(kShortReplacementValue) + arraysize(kLongReplacementValue);
+const I18nTest kTest1 =

[dschuyler, 2017/01/04 01:07:51]

I kinda wanted better names than kTest1 and kTest2, but the summary is: a bunch
of stuff that I thought may be an issue depending on how the code changes in the
future. i.e. Test 1 and Test 2.

[mmenke, 2017/01/04 16:12:50]

Maybe just a comment along those lines?

[dschuyler, 2017/01/04 23:06:52]

Done.

+    I18nTest("  }    $($i18n{gamma})^_^_^_^_$i18n{alpha}_$i18n{gamma}$",
+             "  }    $(carrot)^_^_^_^_apple_carrot$");
+
+const I18nTest kTest2 =
+    I18nTest("$i18n{alpha} gamma}{ ^_^_^_^_$abc{beta}:$i18n{gamma}z",
+             "apple gamma}{ ^_^_^_^_$abc{beta}:carrotz");
 
 struct I18nTestParam {
-  I18nTestParam(int buf_size, net::MockSourceStream::Mode read_mode)
-      : buffer_size(buf_size), mode(read_mode) {}
+  I18nTestParam(int buf_size,

[mmenke, 2017/01/04 16:12:49]

per above comment, this should be constexpr as well, I believe.

[dschuyler, 2017/01/04 23:06:53]

Done.

+                net::MockSourceStream::Mode read_mode,
+                const I18nTest* test)
+      : buffer_size(buf_size), mode(read_mode), test(test) {}
 
   const int buffer_size;
   const net::MockSourceStream::Mode mode;
+  const I18nTest* test;
 };
 
 }  // namespace
@@ -50,32 +73,15 @@ class I18nSourceStreamTest : public ::testing::TestWithParam<I18nTestParam> {
 
   // Helpful function to initialize the test fixture.
   void Init() {
-    source_data_len_ = kBufferSize;
-    for (size_t i = 0; i < source_data_len_; i++)
-      source_data_[i] = i % 256;
-
-    // Inserts must be done last to first as they appear in the buffer.
-    InsertText(source_data_, source_data_len_, kLongReplacementOffset,
-               kLongReplacementToken);
-    InsertText(source_data_, source_data_len_, kShortReplacementOffset,
-               kShortReplacementToken);
-
-    result_data_len_ = kBufferSize;
-    for (size_t i = 0; i < result_data_len_; i++)
-      result_data_[i] = i % 256;
-
-    // Inserts must be done last to first as they appear in the buffer.
-    InsertText(result_data_, result_data_len_, kLongReplacementOffset,
-               kLongReplacementValue);
-    InsertText(result_data_, result_data_len_, kShortReplacementOffset,
-               kShortReplacementValue);
+    std::memset(padding, 'z', 1024);

[mmenke, 2017/01/04 16:12:50]

Maybe make this more interesting?  (0x00 to 0xFF repeating?)

[dschuyler, 2017/01/04 23:06:52]

I wanted to exclude nulls. I think it's more likely that errors will be made in
doing char* to std::string conversion in the tests themselves with nulls in the
buffer than it's likely to find errors in the code under test. WDYT?

 
     output_buffer_ = new net::IOBuffer(output_buffer_size_);
     std::unique_ptr<net::MockSourceStream> source(new net::MockSourceStream());
     source_ = source.get();
 
-    replacements_[kShortReplacementKey] = kShortReplacementValue;
-    replacements_[kLongReplacementKey] = kLongReplacementValue;
+    replacements_["alpha"] = "apple";
+    replacements_["beta"] = "banana";
+    replacements_["gamma"] = "carrot";
     stream_ = I18nSourceStream::Create(
         std::move(source), net::SourceStream::TYPE_NONE, &replacements_);
   }
@@ -94,23 +100,7 @@ class I18nSourceStreamTest : public ::testing::TestWithParam<I18nTestParam> {
     return previous_result;
   }
 
-  void InsertText(char* buffer,
-                  size_t buffer_length,
-                  size_t offset,
-                  const char* text) {
-    // Intended to be dead simple so that it can be confirmed
-    // as correct by hand.
-    size_t text_length = strlen(text);
-    memmove(buffer + offset + text_length, buffer + offset,
-            buffer_length - offset - text_length);
-    memcpy(buffer + offset, text, text_length);
-  }
-
-  char* source_data() { return source_data_; }
-  size_t source_data_len() { return source_data_len_; }
-
-  char* result_data() { return result_data_; }
-  size_t result_data_len() { return result_data_len_; }
+  char padding[1024];

[mmenke, 2017/01/04 16:12:49]

padding_.  Should also be below all methods.

[dschuyler, 2017/01/04 23:06:53]

Done.

 
   net::IOBuffer* output_buffer() { return output_buffer_.get(); }
   char* output_data() { return output_buffer_->data(); }
@@ -142,12 +132,6 @@ class I18nSourceStreamTest : public ::testing::TestWithParam<I18nTestParam> {
   }
 
  private:
-  char source_data_[kBufferSize];
-  size_t source_data_len_;
-
-  char result_data_[kBufferSize];
-  size_t result_data_len_;
-
   scoped_refptr<net::IOBuffer> output_buffer_;
   const int output_buffer_size_;
 
@@ -160,9 +144,47 @@ class I18nSourceStreamTest : public ::testing::TestWithParam<I18nTestParam> {
 INSTANTIATE_TEST_CASE_P(
     I18nSourceStreamTests,
     I18nSourceStreamTest,
-    ::testing::Values(I18nTestParam(kBufferSize, net::MockSourceStream::SYNC),
-                      I18nTestParam(kSmallBufferSize,
-                                    net::MockSourceStream::SYNC)));
+    ::testing::Values(
+        I18nTestParam(kBufferSize,
+                      net::MockSourceStream::SYNC,
+                      &kTestNoReplacements),

[mmenke, 2017/01/04 16:12:50]

optional: Suggest arranging these by string, rather than by buffer size.  It's
easier to check at a glance that each string has two tests, rather than each
buffer size is used for all strings.

[dschuyler, 2017/01/04 23:06:53]

Done.

+        I18nTestParam(kBufferSize,
+                      net::MockSourceStream::SYNC,
+                      &kTestTagAtEndOfLine),
+        I18nTestParam(kBufferSize,
+                      net::MockSourceStream::SYNC,
+                      &kTestOneReplacement),
+        I18nTestParam(kBufferSize,
+                      net::MockSourceStream::SYNC,
+                      &kTestOneReplacementPlus),
+        I18nTestParam(kBufferSize,
+                      net::MockSourceStream::SYNC,
+                      &kTestThreeReplacements),
+        I18nTestParam(kBufferSize,
+                      net::MockSourceStream::SYNC,
+                      &kTestExtraBraces),
+        I18nTestParam(kBufferSize, net::MockSourceStream::SYNC, &kTest1),
+        I18nTestParam(kBufferSize, net::MockSourceStream::SYNC, &kTest2),
+        I18nTestParam(kSmallBufferSize,
+                      net::MockSourceStream::SYNC,
+                      &kTestNoReplacements),
+        I18nTestParam(kSmallBufferSize,
+                      net::MockSourceStream::SYNC,
+                      &kTestTagAtEndOfLine),
+        I18nTestParam(kSmallBufferSize,
+                      net::MockSourceStream::SYNC,
+                      &kTestOneReplacement),
+        I18nTestParam(kSmallBufferSize,
+                      net::MockSourceStream::SYNC,
+                      &kTestOneReplacementPlus),
+        I18nTestParam(kSmallBufferSize,
+                      net::MockSourceStream::SYNC,
+                      &kTestThreeReplacements),
+        I18nTestParam(kSmallBufferSize,
+                      net::MockSourceStream::SYNC,
+                      &kTestExtraBraces),
+        I18nTestParam(kSmallBufferSize, net::MockSourceStream::SYNC, &kTest1),
+        I18nTestParam(kSmallBufferSize, net::MockSourceStream::SYNC, &kTest2)));

[mmenke, 2017/01/04 16:12:50]

No async tests?  Looks like that was the case before this change as well.

If we don't want/need them, then should get rid of the parameter, and remove
CompleteReadIfAsync as well, since reads are never async.  If we do want them,
should add them (Fine to do in a follow up CL, if they can't just be trivially
added).

[dschuyler, 2017/01/04 23:06:53]

I make the param optional. So it's semi-removed. I haven't decided which way to
go on async tests.

 
 TEST_P(I18nSourceStreamTest, EmptyStream) {

[dschuyler, 2017/01/04 01:07:51]

I wanted to run this as a 
const I18nTest kTestNoReplacements = I18nTest("", "");
but there is a dcheck iirc that was alarming. So this is done separately.

   Init();
@@ -173,27 +195,18 @@ TEST_P(I18nSourceStreamTest, EmptyStream) {
   EXPECT_EQ("i18n", stream()->Description());
 }
 
-TEST_P(I18nSourceStreamTest, NoTranslations) {

[dschuyler, 2017/01/04 01:07:51]

This is replaced by kTestNoReplacements.

-  Init();
-  const char kText[] = "This text has no i18n replacements.";
-  size_t kTextLength = strlen(kText);
-  source()->AddReadResult(kText, kTextLength, net::OK, GetParam().mode);
-  source()->AddReadResult(nullptr, 0, net::OK, GetParam().mode);
-  std::string actual_output;
-  int rv = ReadStream(&actual_output);
-  EXPECT_EQ(static_cast<int>(kTextLength), rv);
-  EXPECT_EQ(kText, actual_output);
-  EXPECT_EQ("i18n", stream()->Description());
-}
-
 TEST_P(I18nSourceStreamTest, I18nOneRead) {
   Init();
-  source()->AddReadResult(source_data(), kSourceSize, net::OK, GetParam().mode);
+  size_t source_size = strlen(GetParam().test->input);
+  source()->AddReadResult(GetParam().test->input, source_size, net::OK,
+                          GetParam().mode);
   source()->AddReadResult(nullptr, 0, net::OK, GetParam().mode);
   std::string actual_output;
   int rv = ReadStream(&actual_output);
+  size_t kResultSize = strlen(GetParam().test->expected_output);

[mmenke, 2017/01/04 16:12:50]

result_size

[dschuyler, 2017/01/04 23:06:53]

Done.

   EXPECT_EQ(static_cast<int>(kResultSize), rv);
-  EXPECT_EQ(std::string(result_data(), kResultSize), actual_output);
+  EXPECT_EQ(std::string(GetParam().test->expected_output, kResultSize),
+            actual_output);
   EXPECT_EQ("i18n", stream()->Description());
 }
 
@@ -201,31 +214,46 @@ TEST_P(I18nSourceStreamTest, I18nInMultipleReads) {
   Init();
   size_t chunk_size = 5;
   size_t written = 0;
+  size_t kSourceSize = strlen(GetParam().test->input);

[mmenke, 2017/01/04 16:12:50]

source_size

[dschuyler, 2017/01/04 23:06:53]

Done.

   while (written + chunk_size < kSourceSize) {
-    source()->AddReadResult(source_data() + written, chunk_size, net::OK,
-                            GetParam().mode);
+    source()->AddReadResult(GetParam().test->input + written, chunk_size,
+                            net::OK, GetParam().mode);
     written += chunk_size;
   }
-  source()->AddReadResult(source_data() + written, kSourceSize - written,
-                          net::OK, GetParam().mode);
+  source()->AddReadResult(GetParam().test->input + written,
+                          kSourceSize - written, net::OK, GetParam().mode);

[mmenke, 2017/01/04 16:12:50]

Suggest just rewriting the loop to include this result:

i.e:

while (written < kSourceSize) {
  int write_size = std::min(chunk_size, kSourceSize
  source()->AddReadResult(GetParam().test->input + written, write_size, ...);
  written += chunk_size;
}

Think it's easier to read.

Or could just use a chunk_size of 1, which simplifies the code and may (or may
not) provide better corner case coverage.  Know we recently ran into a gzip
filter bug that only would have surfaced with 1-byte reads.

[dschuyler, 2017/01/04 23:06:53]

Done.

   source()->AddReadResult(nullptr, 0, net::OK, GetParam().mode);
   std::string actual_output;
   int rv = ReadStream(&actual_output);
+  size_t kResultSize = strlen(GetParam().test->expected_output);

[mmenke, 2017/01/04 16:12:49]

result_size

[dschuyler, 2017/01/04 23:06:53]

Done.

   EXPECT_EQ(static_cast<int>(kResultSize), rv);
-  EXPECT_EQ(std::string(result_data(), kResultSize), actual_output);
+  EXPECT_EQ(std::string(GetParam().test->expected_output, kResultSize),
+            actual_output);
   EXPECT_EQ("i18n", stream()->Description());
 }
 
-TEST_P(I18nSourceStreamTest, I18nTagAtEndOfLine) {

[dschuyler, 2017/01/04 01:07:51]

This test was replaced with kTestTagAtEndOfLine and is run for varying buffer
sizes.

+TEST_P(I18nSourceStreamTest, I18nLargeAndInMultipleReads) {
   Init();
-  const char kSourceData[] = "test with tag at end of line $";
-  const size_t source_size = strlen(kSourceData);
-  source()->AddReadResult(kSourceData, source_size, net::OK, GetParam().mode);
+  for (int i = 0; i < 1024; ++i) {

[mmenke, 2017/01/04 16:12:50]

Should probably have two 1024 consts here (One for padding and one for padding
length).  Altenatively, just have one constant for total length, and keep on
adding padding until we reach that length.

[dschuyler, 2017/01/04 23:06:53]

I reworked this. PTAL.

+    source()->AddReadResult(padding, 1024, net::OK, GetParam().mode);

[mmenke, 2017/01/04 16:12:50]

Given that this is the only test that uses padding, maybe make it a local
variable here?

[dschuyler, 2017/01/04 23:06:53]

The function is called once per I18nTestParam, so I wanted to do the init of
padding_ once rather than with each call.

[mmenke, 2017/01/04 23:10:09]

The test fixture is re-created for each test that's run.  Each test case is
actually a separate class instance, so you don't actually get those savings.

[mmenke, 2017/01/04 23:18:38]

(Beyond being a different class instance, they're also different subclasses,
actually)

[dschuyler, 2017/01/04 23:33:10]

You're right of course. I should have realized that.

[dschuyler, 2017/01/04 23:33:11]

Acknowledged.

+  }
+  size_t chunk_size = 5;
+  size_t written = 0;
+  size_t kSourceSize = strlen(GetParam().test->input);

[mmenke, 2017/01/04 16:12:50]

source_size

[dschuyler, 2017/01/04 23:06:52]

Done.

+  while (written + chunk_size < kSourceSize) {
+    source()->AddReadResult(GetParam().test->input + written, chunk_size,
+                            net::OK, GetParam().mode);
+    written += chunk_size;
+  }
+  source()->AddReadResult(GetParam().test->input + written,
+                          kSourceSize - written, net::OK, GetParam().mode);

[mmenke, 2017/01/04 16:12:50]

Same suggestion as for the previous test here.

[dschuyler, 2017/01/04 23:06:53]

Done.

   source()->AddReadResult(nullptr, 0, net::OK, GetParam().mode);
   std::string actual_output;
   int rv = ReadStream(&actual_output);
-  EXPECT_EQ(static_cast<int>(source_size), rv);
-  EXPECT_EQ(kSourceData, actual_output);
+  size_t kResultSize = strlen(GetParam().test->expected_output);

[mmenke, 2017/01/04 16:12:50]

result_size

[dschuyler, 2017/01/04 23:06:53]

Done.

+  EXPECT_EQ(static_cast<int>(kResultSize) + 1024 * 1024, rv);
+  EXPECT_EQ(std::string(GetParam().test->expected_output, kResultSize),
+            &actual_output[1024 * 1024]);

[mmenke, 2017/01/04 16:12:49]

Check the padding matches as well?

[dschuyler, 2017/01/04 23:06:53]

Done.

   EXPECT_EQ("i18n", stream()->Description());
 }