[i18n streaming] improve unit tests

content/browser/webui/i18n_source_stream_unittest.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <utility>
 
 #include "content/browser/webui/i18n_source_stream.h"
 #include "net/base/io_buffer.h"
 #include "net/base/test_completion_callback.h"
 #include "net/filter/mock_source_stream.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace content {
 
 namespace {
 
 // These constants are rather arbitrary, though the offsets and other sizes must
 // be less than kBufferSize.
 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 int kSourceSize =
-    50 + arraysize(kShortReplacementToken) + arraysize(kLongReplacementToken);
-const int kResultSize =
-    50 + arraysize(kShortReplacementValue) + arraysize(kLongReplacementValue);
+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 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
 
 class I18nSourceStreamTest : public ::testing::TestWithParam<I18nTestParam> {
  protected:
   I18nSourceStreamTest() : output_buffer_size_(GetParam().buffer_size) {}
 
   // 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_);
   }
 
   // If MockSourceStream::Mode is ASYNC, completes 1 read from |mock_stream| and
   // wait for |callback| to complete. If Mode is not ASYNC, does nothing and
   // returns |previous_result|.
   int CompleteReadIfAsync(int previous_result,
                           net::TestCompletionCallback* callback,
                           net::MockSourceStream* mock_stream) {
     if (GetParam().mode == net::MockSourceStream::ASYNC) {
       EXPECT_EQ(net::ERR_IO_PENDING, previous_result);
       mock_stream->CompleteNextRead();
       return callback->WaitForResult();
     }
     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(); }
   size_t output_buffer_size() { return output_buffer_size_; }
 
   net::MockSourceStream* source() { return source_; }
   I18nSourceStream* stream() { return stream_.get(); }
 
   // Reads from |stream_| until an error occurs or the EOF is reached.
   // When an error occurs, returns the net error code. When an EOF is reached,
@@ skipping 11 matching lines @@
       if (rv < net::OK)
         return rv;
       EXPECT_GT(rv, net::OK);
       bytes_read += rv;
       output->append(output_data(), rv);
     }
     return bytes_read;
   }
 
  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_;
 
   net::MockSourceStream* source_;
   std::unique_ptr<I18nSourceStream> stream_;
 
   ui::TemplateReplacements replacements_;
 };
 
 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.

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

Should this just be TEST_F, with a sync and async one?

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

The reason for that is that your test would add two 0-byte reads, but only one
would be used.  You could modify I18nOneRead to, if the source string was empty,
not make the first AddReadResult call.

If you also follow my suggestions for the cases below, then you could make it a
test case like the others.

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

Done.

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

Acknowledged.

   Init();
   source()->AddReadResult("", 0, net::OK, GetParam().mode);

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

"" -> nullptr

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

Done.

   std::string actual_output;
   int result = ReadStream(&actual_output);
   EXPECT_EQ(net::OK, result);
   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());
 }
 
 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());
 }
 
 }  // namespace content