Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2610063003:
[i18n streaming] improve unit tests (Closed)
Description[i18n streaming] improve unit tests
This CL adds and improves the unit tests for the i18n streaming. It does
not add features or other changes (test case changes only).
(Not a fix for bug 677555, but related test cases).
BUG=677555
Review-Url: https://codereview.chromium.org/2610063003
Cr-Commit-Position: refs/heads/master@{#442450}
Committed: https://chromium.googlesource.com/chromium/src/+/ead12efed77ced7980c10c557dd390e288def6ed
Patch Set 1 #
Total comments: 48
Patch Set 2 : review changes #Patch Set 3 : move padding init #
Total comments: 14
Patch Set 4 : updated #
Total comments: 4
Patch Set 5 : reviewChanges #Messages
Total messages: 47 (29 generated)
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dschuyler@chromium.org changed reviewers: + mmenke@chromium.org
These are some optional improvements to the i18n streaming unit tests. It's not critical whether this CL lands or not, but imo it's a nicer arrangement, so I wanted to run it by you and see if you liked it as well.
https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... File content/browser/webui/i18n_source_stream_unittest.cc (left): https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:167: TEST_P(I18nSourceStreamTest, EmptyStream) { 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. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:176: TEST_P(I18nSourceStreamTest, NoTranslations) { This is replaced by kTestNoReplacements. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:219: TEST_P(I18nSourceStreamTest, I18nTagAtEndOfLine) { This test was replaced with kTestTagAtEndOfLine and is run for varying buffer sizes. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... File content/browser/webui/i18n_source_stream_unittest.cc (right): https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:49: const I18nTest kTest1 = 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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... File content/browser/webui/i18n_source_stream_unittest.cc (right): https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:23: I18nTest(const char* input, const char* expected_output) 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. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:49: const I18nTest kTest1 = On 2017/01/04 01:07:51, dschuyler wrote: > 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. Maybe just a comment along those lines? https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:58: I18nTestParam(int buf_size, per above comment, this should be constexpr as well, I believe. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:76: std::memset(padding, 'z', 1024); Maybe make this more interesting? (0x00 to 0xFF repeating?) https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:103: char padding[1024]; padding_. Should also be below all methods. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:150: &kTestNoReplacements), 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. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:187: I18nTestParam(kSmallBufferSize, net::MockSourceStream::SYNC, &kTest2))); 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). https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:189: TEST_P(I18nSourceStreamTest, EmptyStream) { Should this just be TEST_F, with a sync and async one? https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:189: TEST_P(I18nSourceStreamTest, EmptyStream) { On 2017/01/04 01:07:51, dschuyler wrote: > 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. 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. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:191: source()->AddReadResult("", 0, net::OK, GetParam().mode); "" -> nullptr https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:206: size_t kResultSize = strlen(GetParam().test->expected_output); result_size https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:217: size_t kSourceSize = strlen(GetParam().test->input); source_size https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:224: kSourceSize - written, net::OK, GetParam().mode); 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. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:228: size_t kResultSize = strlen(GetParam().test->expected_output); result_size https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:237: for (int i = 0; i < 1024; ++i) { 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. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:238: source()->AddReadResult(padding, 1024, net::OK, GetParam().mode); Given that this is the only test that uses padding, maybe make it a local variable here? https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:242: size_t kSourceSize = strlen(GetParam().test->input); source_size https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:249: kSourceSize - written, net::OK, GetParam().mode); Same suggestion as for the previous test here. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:253: size_t kResultSize = strlen(GetParam().test->expected_output); result_size https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:256: &actual_output[1024 * 1024]); Check the padding matches as well?
Forgot to say, I do think this is an improvement in test coverage.
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... File content/browser/webui/i18n_source_stream_unittest.cc (right): https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:23: I18nTest(const char* input, const char* expected_output) On 2017/01/04 16:12:50, mmenke wrote: > 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. Done. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:49: const I18nTest kTest1 = On 2017/01/04 16:12:50, mmenke wrote: > On 2017/01/04 01:07:51, dschuyler wrote: > > 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. > > Maybe just a comment along those lines? Done. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:58: I18nTestParam(int buf_size, On 2017/01/04 16:12:49, mmenke wrote: > per above comment, this should be constexpr as well, I believe. Done. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:76: std::memset(padding, 'z', 1024); On 2017/01/04 16:12:50, mmenke wrote: > Maybe make this more interesting? (0x00 to 0xFF repeating?) 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? https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:103: char padding[1024]; On 2017/01/04 16:12:49, mmenke wrote: > padding_. Should also be below all methods. Done. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:150: &kTestNoReplacements), On 2017/01/04 16:12:50, mmenke wrote: > 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. Done. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:187: I18nTestParam(kSmallBufferSize, net::MockSourceStream::SYNC, &kTest2))); On 2017/01/04 16:12:50, mmenke wrote: > 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). I make the param optional. So it's semi-removed. I haven't decided which way to go on async tests. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:189: TEST_P(I18nSourceStreamTest, EmptyStream) { On 2017/01/04 16:12:50, mmenke wrote: > On 2017/01/04 01:07:51, dschuyler wrote: > > 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. > > 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. Done. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:189: TEST_P(I18nSourceStreamTest, EmptyStream) { On 2017/01/04 16:12:49, mmenke wrote: > Should this just be TEST_F, with a sync and async one? Acknowledged. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:191: source()->AddReadResult("", 0, net::OK, GetParam().mode); On 2017/01/04 16:12:49, mmenke wrote: > "" -> nullptr Done. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:206: size_t kResultSize = strlen(GetParam().test->expected_output); On 2017/01/04 16:12:50, mmenke wrote: > result_size Done. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:217: size_t kSourceSize = strlen(GetParam().test->input); On 2017/01/04 16:12:50, mmenke wrote: > source_size Done. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:224: kSourceSize - written, net::OK, GetParam().mode); On 2017/01/04 16:12:50, mmenke wrote: > 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. Done. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:228: size_t kResultSize = strlen(GetParam().test->expected_output); On 2017/01/04 16:12:49, mmenke wrote: > result_size Done. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:237: for (int i = 0; i < 1024; ++i) { On 2017/01/04 16:12:50, mmenke wrote: > 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. I reworked this. PTAL. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:238: source()->AddReadResult(padding, 1024, net::OK, GetParam().mode); On 2017/01/04 16:12:50, mmenke wrote: > Given that this is the only test that uses padding, maybe make it a local > variable here? The function is called once per I18nTestParam, so I wanted to do the init of padding_ once rather than with each call. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:242: size_t kSourceSize = strlen(GetParam().test->input); On 2017/01/04 16:12:50, mmenke wrote: > source_size Done. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:249: kSourceSize - written, net::OK, GetParam().mode); On 2017/01/04 16:12:50, mmenke wrote: > Same suggestion as for the previous test here. Done. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:253: size_t kResultSize = strlen(GetParam().test->expected_output); On 2017/01/04 16:12:50, mmenke wrote: > result_size Done. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:256: &actual_output[1024 * 1024]); On 2017/01/04 16:12:49, mmenke wrote: > Check the padding matches as well? Done.
https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... File content/browser/webui/i18n_source_stream_unittest.cc (right): https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:238: source()->AddReadResult(padding, 1024, net::OK, GetParam().mode); On 2017/01/04 23:06:53, dschuyler wrote: > On 2017/01/04 16:12:50, mmenke wrote: > > Given that this is the only test that uses padding, maybe make it a local > > variable here? > > The function is called once per I18nTestParam, so I wanted to do the init of > padding_ once rather than with each call. 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.
https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... File content/browser/webui/i18n_source_stream_unittest.cc (right): https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:238: source()->AddReadResult(padding, 1024, net::OK, GetParam().mode); On 2017/01/04 23:10:09, mmenke wrote: > On 2017/01/04 23:06:53, dschuyler wrote: > > On 2017/01/04 16:12:50, mmenke wrote: > > > Given that this is the only test that uses padding, maybe make it a local > > > variable here? > > > > The function is called once per I18nTestParam, so I wanted to do the init of > > padding_ once rather than with each call. > > 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. (Beyond being a different class instance, they're also different subclasses, actually)
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... File content/browser/webui/i18n_source_stream_unittest.cc (right): https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:238: source()->AddReadResult(padding, 1024, net::OK, GetParam().mode); On 2017/01/04 23:10:09, mmenke wrote: > On 2017/01/04 23:06:53, dschuyler wrote: > > On 2017/01/04 16:12:50, mmenke wrote: > > > Given that this is the only test that uses padding, maybe make it a local > > > variable here? > > > > The function is called once per I18nTestParam, so I wanted to do the init of > > padding_ once rather than with each call. > > 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. You're right of course. I should have realized that. https://codereview.chromium.org/2610063003/diff/1/content/browser/webui/i18n_... content/browser/webui/i18n_source_stream_unittest.cc:238: source()->AddReadResult(padding, 1024, net::OK, GetParam().mode); On 2017/01/04 23:18:38, mmenke wrote: > On 2017/01/04 23:10:09, mmenke wrote: > > On 2017/01/04 23:06:53, dschuyler wrote: > > > On 2017/01/04 16:12:50, mmenke wrote: > > > > Given that this is the only test that uses padding, maybe make it a local > > > > variable here? > > > > > > The function is called once per I18nTestParam, so I wanted to do the init of > > > padding_ once rather than with each call. > > > > 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. > > (Beyond being a different class instance, they're also different subclasses, > actually) Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
A couple more suggestions, but looks good. https://codereview.chromium.org/2610063003/diff/60001/content/browser/webui/i... File content/browser/webui/i18n_source_stream_unittest.cc (right): https://codereview.chromium.org/2610063003/diff/60001/content/browser/webui/i... content/browser/webui/i18n_source_stream_unittest.cc:82: const int pad_count; Suggest a bool instead - add_padding, and then add padding a fixed number of times, since I don't see a future in which we'd try different amounts of padding. https://codereview.chromium.org/2610063003/diff/60001/content/browser/webui/i... content/browser/webui/i18n_source_stream_unittest.cc:165: I18nTestParam(&kTestNoReplacements, kBufferSize, kInOneReadSize), On 2017/01/04 23:06:53, dschuyler wrote: > On 2017/01/04 16:12:50, mmenke wrote: > > 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. > > Done. Feel free not to do this, but "by string", I meant by string from the top of the file. i.e. I18nTestParam(&kTestEmpty, kBufferSize, kInOneReadSize), I18nTestParam(&kTestEmpty, kBufferSize, kInOneReadSize, kPadCount), I18nTestParam(&kTestNoReplacements, kBufferSize, kInOneReadSize), I18nTestParam(&kTestNoReplacements, kBufferSize, kInOneReadSize, kPadCount), I18nTestParam(&kTestNoReplacements, kBufferSize, kSmallSize), I18nTestParam(&kTestNoReplacements, kMinimumSize, kSmallSize), I18nTestParam(&kTestNoReplacements, kMinimumSize, kMinimumSize), That makes it easy to tell which tests are run on each string, and when adding new strings, easier to decide what tests it should be run on. https://codereview.chromium.org/2610063003/diff/60001/content/browser/webui/i... content/browser/webui/i18n_source_stream_unittest.cc:229: TEST_P(I18nSourceStreamTest, I18nLargeAndInMultipleReads) { I18nLargeAndInMultipleReads -> RunFilterTests? (Or some other name, the name just seems no longer accurate) https://codereview.chromium.org/2610063003/diff/60001/content/browser/webui/i... content/browser/webui/i18n_source_stream_unittest.cc:229: TEST_P(I18nSourceStreamTest, I18nLargeAndInMultipleReads) { optional: Could remove padding size from I18nTestParam and run each test with and without padding. Makes test list a bit simpler, and removes the worry over what tests used it and what tests don't. https://codereview.chromium.org/2610063003/diff/60001/content/browser/webui/i... content/browser/webui/i18n_source_stream_unittest.cc:233: padding[i] = i % 251; // 251 is prime and avoids power-of-two repetition. suggest making this an std::string (Or making a padding_string that's a std::string), to avoid all the sizeofs, which I think reduce readability a bit. https://codereview.chromium.org/2610063003/diff/60001/content/browser/webui/i... content/browser/webui/i18n_source_stream_unittest.cc:257: EXPECT_EQ(expected_output.size() + pad_size, static_cast<size_t>(rv)); ASSERT_EQ? If this fails, other EXPECTs may crash https://codereview.chromium.org/2610063003/diff/60001/content/browser/webui/i... content/browser/webui/i18n_source_stream_unittest.cc:260: padding, sizeof padding) == 0); EXPECT_EQ(padding (or padding_string), actual_output.substring(i * padding.size(), padding.size())); It's a lot of stuff to output on failure, but still may produce more useful output from the bots on tryjob failure.
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2610063003/diff/60001/content/browser/webui/i... File content/browser/webui/i18n_source_stream_unittest.cc (right): https://codereview.chromium.org/2610063003/diff/60001/content/browser/webui/i... content/browser/webui/i18n_source_stream_unittest.cc:82: const int pad_count; On 2017/01/05 17:25:53, mmenke wrote: > Suggest a bool instead - add_padding, and then add padding a fixed number of > times, since I don't see a future in which we'd try different amounts of > padding. Acknowledged. https://codereview.chromium.org/2610063003/diff/60001/content/browser/webui/i... content/browser/webui/i18n_source_stream_unittest.cc:165: I18nTestParam(&kTestNoReplacements, kBufferSize, kInOneReadSize), On 2017/01/05 17:25:53, mmenke wrote: > On 2017/01/04 23:06:53, dschuyler wrote: > > On 2017/01/04 16:12:50, mmenke wrote: > > > 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. > > > > Done. > > Feel free not to do this, but "by string", I meant by string from the top of the > file. i.e. > > I18nTestParam(&kTestEmpty, kBufferSize, kInOneReadSize), > I18nTestParam(&kTestEmpty, kBufferSize, kInOneReadSize, kPadCount), > > I18nTestParam(&kTestNoReplacements, kBufferSize, kInOneReadSize), > I18nTestParam(&kTestNoReplacements, kBufferSize, kInOneReadSize, kPadCount), > I18nTestParam(&kTestNoReplacements, kBufferSize, kSmallSize), > I18nTestParam(&kTestNoReplacements, kMinimumSize, kSmallSize), > I18nTestParam(&kTestNoReplacements, kMinimumSize, kMinimumSize), > > That makes it easy to tell which tests are run on each string, and when adding > new strings, easier to decide what tests it should be run on. Done. https://codereview.chromium.org/2610063003/diff/60001/content/browser/webui/i... content/browser/webui/i18n_source_stream_unittest.cc:229: TEST_P(I18nSourceStreamTest, I18nLargeAndInMultipleReads) { On 2017/01/05 17:25:53, mmenke wrote: > I18nLargeAndInMultipleReads -> RunFilterTests? (Or some other name, the name > just seems no longer accurate) FilterTests and LargeFilterTests? https://codereview.chromium.org/2610063003/diff/60001/content/browser/webui/i... content/browser/webui/i18n_source_stream_unittest.cc:229: TEST_P(I18nSourceStreamTest, I18nLargeAndInMultipleReads) { On 2017/01/05 17:25:53, mmenke wrote: > optional: Could remove padding size from I18nTestParam and run each test with > and without padding. Makes test list a bit simpler, and removes the worry over > what tests used it and what tests don't. Done. https://codereview.chromium.org/2610063003/diff/60001/content/browser/webui/i... content/browser/webui/i18n_source_stream_unittest.cc:233: padding[i] = i % 251; // 251 is prime and avoids power-of-two repetition. On 2017/01/05 17:25:53, mmenke wrote: > suggest making this an std::string (Or making a padding_string that's a > std::string), to avoid all the sizeofs, which I think reduce readability a bit. Done. https://codereview.chromium.org/2610063003/diff/60001/content/browser/webui/i... content/browser/webui/i18n_source_stream_unittest.cc:257: EXPECT_EQ(expected_output.size() + pad_size, static_cast<size_t>(rv)); On 2017/01/05 17:25:53, mmenke wrote: > ASSERT_EQ? If this fails, other EXPECTs may crash Done. https://codereview.chromium.org/2610063003/diff/60001/content/browser/webui/i... content/browser/webui/i18n_source_stream_unittest.cc:260: padding, sizeof padding) == 0); On 2017/01/05 17:25:53, mmenke wrote: > EXPECT_EQ(padding (or padding_string), > actual_output.substring(i * padding.size(), padding.size())); > > It's a lot of stuff to output on failure, but still may produce more useful > output from the bots on tryjob failure. Done.
LGTM! https://codereview.chromium.org/2610063003/diff/80001/content/browser/webui/i... File content/browser/webui/i18n_source_stream_unittest.cc (right): https://codereview.chromium.org/2610063003/diff/80001/content/browser/webui/i... content/browser/webui/i18n_source_stream_unittest.cc:226: ASSERT_EQ(expected_output.size(), static_cast<size_t>(rv)); optional: This is redundant with the EXPECT_EQ on the next line, and so isn't needed (Alternatively, ASSERT seems worse than EXPECT on this test - we won't crash on mismatch, unlike in the next one) https://codereview.chromium.org/2610063003/diff/80001/content/browser/webui/i... content/browser/webui/i18n_source_stream_unittest.cc:242: int pad_count = 128; // Arbitrary number of pads to add. nit: const int kPadCount
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dschuyler@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2610063003/diff/80001/content/browser/webui/i... File content/browser/webui/i18n_source_stream_unittest.cc (right): https://codereview.chromium.org/2610063003/diff/80001/content/browser/webui/i... content/browser/webui/i18n_source_stream_unittest.cc:226: ASSERT_EQ(expected_output.size(), static_cast<size_t>(rv)); On 2017/01/06 22:39:40, mmenke wrote: > optional: This is redundant with the EXPECT_EQ on the next line, and so isn't > needed (Alternatively, ASSERT seems worse than EXPECT on this test - we won't > crash on mismatch, unlike in the next one) Converted to expect rather than assert. I agree that it is somewhat redundant, but it kinda seems like a reasonable test since the value comes out of the api in two places (return and .size() of param). Done. https://codereview.chromium.org/2610063003/diff/80001/content/browser/webui/i... content/browser/webui/i18n_source_stream_unittest.cc:242: int pad_count = 128; // Arbitrary number of pads to add. On 2017/01/06 22:39:40, mmenke wrote: > nit: const int kPadCount Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dschuyler@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2610063003/#ps100001 (title: "reviewChanges")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dschuyler@chromium.org changed reviewers: + dbeam@chromium.org
dbeam@ for owner.
^_^ rs lgtm ^_^
The CQ bit was checked by dschuyler@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484009245059240,
"parent_rev": "848410dc4e71fa3ce9727478ab013a8948c4dfc6", "commit_rev":
"ead12efed77ced7980c10c557dd390e288def6ed"}
Message was sent while issue was closed.
Description was changed from ========== [i18n streaming] improve unit tests This CL adds and improves the unit tests for the i18n streaming. It does not add features or other changes (test case changes only). (Not a fix for bug 677555, but related test cases). BUG=677555 ========== to ========== [i18n streaming] improve unit tests This CL adds and improves the unit tests for the i18n streaming. It does not add features or other changes (test case changes only). (Not a fix for bug 677555, but related test cases). BUG=677555 Review-Url: https://codereview.chromium.org/2610063003 Cr-Commit-Position: refs/heads/master@{#442450} Committed: https://chromium.googlesource.com/chromium/src/+/ead12efed77ced7980c10c557dd3... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/ead12efed77ced7980c10c557dd3... |
