Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2251893004:
Use parameterized tests to test multiple render text implementations. (Closed)
|
Created:
4 years, 4 months ago by karandeepb Modified:
4 years, 3 months ago Reviewers:
msw CC:
chromium-reviews, derat+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@selection_direction Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse parameterized tests to test multiple render text implementations.
Currently, on Mac most of the tests in render_text_unittest.cc use
RenderText::CreateInstance to create their render text instance. Also, many
tests which would fail on Mac with RenderTextMac are not compiled on Mac. As a
result there is negligible test coverage for RenderTextHarfBuzz on Mac.
This CL uses parameterized tests to test both RenderTextMac and
RenderTextHarfBuzz on Mac. It does this by introducing three new test fixture
classes which support parameterized tests-
-RenderTextTest
-RenderTextHarfBuzzTest
-RenderTextMacTest
The test helper class RenderTextAllBackends is removed.
Before this CL, 65 tests in render_text_unittest.cc are run on Mac. After this
CL, a total of 129 tests in render_text_unittest.cc are run on Mac.
BUG=639194
Committed: https://crrev.com/50b6fa15b332c7f049069a1f569ca1e374462601
Cr-Commit-Position: refs/heads/master@{#414644}
Patch Set 1 #Patch Set 2 : Remove RenderTextAllBackends. #Patch Set 3 : Fix linux compile. #Patch Set 4 : Refer crbug. #
Total comments: 2
Patch Set 5 #Patch Set 6 : Decrease diff. #Patch Set 7 #
Total comments: 27
Patch Set 8 : Rename RenderTextTestAll->RenderTextTest #Patch Set 9 : Address nits. #
Total comments: 2
Patch Set 10 : Address review comments. #
Depends on Patchset: Messages
Total messages: 65 (57 generated)
The CQ bit was checked by karandeepb@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...
Description was changed from ========== --- --- --- --- --- BUG= ========== to ========== --- --- --- --- --- BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by karandeepb@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by karandeepb@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== --- --- --- --- --- BUG= ========== to ========== ==========
Description was changed from ========== ========== to ========== Use parameterized tests to test multiple render text implementations. Currently, on Mac most of the tests in render_text_unittest.cc use RenderText::CreateInstance to create their render text instance. Also, many tests which would fail on Mac with RenderTextMac are not compiled on Mac. As a result there is negligible test coverage for RenderTextHarfBuzz on Mac. This CL uses parameterized tests to test both RenderTextMac and RenderTextHarfBuzz on Mac. It does this by introducing three new test fixture classes which support parameterized tests- -RenderTextTestAll -RenderTextHarfBuzzTest -RenderTextMacTest The existing fixture class RenderTextTest and the helper class RenderTextAllBackends are removed. Before this CL, 65 tests in render_text_unittest.cc are run on Mac. After this CL, a total of 129 tests in render_text_unittest.cc are run on Mac. BUG=639194 ==========
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Patchset #6 (id:100001) has been deleted
Patchset #6 (id:120001) has been deleted
Patchset #3 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #5 (id:140001) has been deleted
The CQ bit was checked by karandeepb@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by karandeepb@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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by karandeepb@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by karandeepb@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 karandeepb@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 #10 (id:260001) has been deleted
Patchset #9 (id:240001) has been deleted
Patchset #6 (id:180001) has been deleted
Patchset #5 (id:160001) has been deleted
karandeepb@chromium.org changed reviewers: + msw@chromium.org
PTAL msw@. Thanks. https://codereview.chromium.org/2251893004/diff/80001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (left): https://codereview.chromium.org/2251893004/diff/80001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:1918: TEST_F(RenderTextTest, StringSizeEmptyString) { This doesn't fail with RenderTextMac now. https://codereview.chromium.org/2251893004/diff/80001/ui/gfx/render_text_unit... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2251893004/diff/80001/ui/gfx/render_text_unit... ui/gfx/render_text_unittest.cc:2018: #if !defined(OS_WIN) It seems that the TEST_P macro doesn't work correctly when the macro arguments themselves are macros. This is probably because the TEST_P macro directly uses the stringify(#) macro operator instead of using a second macro to do the job - https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest-param-.... Will file a bug/issue a patch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Nice change; some minor comments; mostly optional nits. https://codereview.chromium.org/2251893004/diff/280001/tools/valgrind/gtest_e... File tools/valgrind/gtest_exclude/ui_base_unittests.gtest-memcheck.txt (right): https://codereview.chromium.org/2251893004/diff/280001/tools/valgrind/gtest_e... tools/valgrind/gtest_exclude/ui_base_unittests.gtest-memcheck.txt:2: RenderTextHarfBuzzTest.DisplayRectShowsCursorLTR I wonder if we can remove this exclusion; the cited bug is closed (but not directly related?) https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text.h#n... ui/gfx/render_text.h:653: FRIEND_TEST_ALL_PREFIXES(RenderTextTestAll, DefaultStyles); This isn't a blocker for this CL, but removing friend test fixtures here and in RenderTextHarfBuzz would be nice (make them use RenderTextTestApi or friend the RenderTextTestAll/RenderTextHarfBuzzTest classes and add helpers on those class definitions that the fixtures can use). Again, not a blocker, just a thought. https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:291: class RenderTextTestAll nit: maybe just "RenderTextTest"? https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:305: render_text.reset(new RenderTextHarfBuzz()); optional nit: return from within cases. https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:366: // Test fixture class. Use for tests which are only to be run on nit: s/on/for/ (be consistent with comment below) https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:519: // implemented. optional nit: cite http://crbug.com/131618 https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:850: #endif // !defined(OS_MACOSX) nit: add a blank line above this. https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:852: // TODO(PORT): Fails for RenderTextMac. optional nit: cite a bug? https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:1114: RunMoveCursorTestAndClearExpectations(render_text, WORD_BREAK, CURSOR_RIGHT, nit: consistent arg wrapping for these calls across fixtures. https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:1846: // TODO(msw): Make these work on Windows. optional nit: cite http://crbug.com/196326 https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:3725: INSTANTIATE_TEST_CASE_P(, nit: comment on empty first arg. https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:3736: RenderTextTestAll, nit: the test logs print as "RenderTextTestAll.TruncatedText/RenderTextHarfBuzz", could we get them to print either: "RenderTextHarfBuzzTest.TruncatedText" (probably not...) or "RenderTextTest.TruncatedText/HarfBuzz" or similar?
The CQ bit was checked by karandeepb@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...
Description was changed from ========== Use parameterized tests to test multiple render text implementations. Currently, on Mac most of the tests in render_text_unittest.cc use RenderText::CreateInstance to create their render text instance. Also, many tests which would fail on Mac with RenderTextMac are not compiled on Mac. As a result there is negligible test coverage for RenderTextHarfBuzz on Mac. This CL uses parameterized tests to test both RenderTextMac and RenderTextHarfBuzz on Mac. It does this by introducing three new test fixture classes which support parameterized tests- -RenderTextTestAll -RenderTextHarfBuzzTest -RenderTextMacTest The existing fixture class RenderTextTest and the helper class RenderTextAllBackends are removed. Before this CL, 65 tests in render_text_unittest.cc are run on Mac. After this CL, a total of 129 tests in render_text_unittest.cc are run on Mac. BUG=639194 ========== to ========== Use parameterized tests to test multiple render text implementations. Currently, on Mac most of the tests in render_text_unittest.cc use RenderText::CreateInstance to create their render text instance. Also, many tests which would fail on Mac with RenderTextMac are not compiled on Mac. As a result there is negligible test coverage for RenderTextHarfBuzz on Mac. This CL uses parameterized tests to test both RenderTextMac and RenderTextHarfBuzz on Mac. It does this by introducing three new test fixture classes which support parameterized tests- -RenderTextTest -RenderTextHarfBuzzTest -RenderTextMacTest The test helper class RenderTextAllBackends is removed. Before this CL, 65 tests in render_text_unittest.cc are run on Mac. After this CL, a total of 129 tests in render_text_unittest.cc are run on Mac. BUG=639194 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by karandeepb@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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL msw@. https://codereview.chromium.org/2251893004/diff/280001/tools/valgrind/gtest_e... File tools/valgrind/gtest_exclude/ui_base_unittests.gtest-memcheck.txt (right): https://codereview.chromium.org/2251893004/diff/280001/tools/valgrind/gtest_e... tools/valgrind/gtest_exclude/ui_base_unittests.gtest-memcheck.txt:2: RenderTextHarfBuzzTest.DisplayRectShowsCursorLTR On 2016/08/25 03:02:19, msw wrote: > I wonder if we can remove this exclusion; the cited bug is closed (but not > directly related?) I am also not sure. Since the cited bug is closed, we can probably see if removing this causes a failure. That way we can at least file a new bug. https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text.h#n... ui/gfx/render_text.h:653: FRIEND_TEST_ALL_PREFIXES(RenderTextTestAll, DefaultStyles); On 2016/08/25 03:02:19, msw wrote: > This isn't a blocker for this CL, but removing friend test fixtures here and in > RenderTextHarfBuzz would be nice (make them use RenderTextTestApi or friend the > RenderTextTestAll/RenderTextHarfBuzzTest classes and add helpers on those class > definitions that the fixtures can use). Again, not a blocker, just a thought. Will look to do this in a future CL. https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:291: class RenderTextTestAll On 2016/08/25 03:02:19, msw wrote: > nit: maybe just "RenderTextTest"? Done. https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:305: render_text.reset(new RenderTextHarfBuzz()); On 2016/08/25 03:02:19, msw wrote: > optional nit: return from within cases. Done. https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:366: // Test fixture class. Use for tests which are only to be run on On 2016/08/25 03:02:19, msw wrote: > nit: s/on/for/ (be consistent with comment below) Done. https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:519: // implemented. On 2016/08/25 03:02:19, msw wrote: > optional nit: cite http://crbug.com/131618 Done. https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:850: #endif // !defined(OS_MACOSX) On 2016/08/25 03:02:19, msw wrote: > nit: add a blank line above this. Done. https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:852: // TODO(PORT): Fails for RenderTextMac. On 2016/08/25 03:02:19, msw wrote: > optional nit: cite a bug? Can't find a relevant bug. This was just copied from an existing comment. https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:1114: RunMoveCursorTestAndClearExpectations(render_text, WORD_BREAK, CURSOR_RIGHT, On 2016/08/25 03:02:19, msw wrote: > nit: consistent arg wrapping for these calls across fixtures. I guess this is how clang format likes it. https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:1846: // TODO(msw): Make these work on Windows. On 2016/08/25 03:02:19, msw wrote: > optional nit: cite http://crbug.com/196326 Done. https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:3725: INSTANTIATE_TEST_CASE_P(, On 2016/08/25 03:02:19, msw wrote: > nit: comment on empty first arg. Done. https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:3736: RenderTextTestAll, On 2016/08/25 03:02:19, msw wrote: > nit: the test logs print as > "RenderTextTestAll.TruncatedText/RenderTextHarfBuzz", could we get them to print > either: "RenderTextHarfBuzzTest.TruncatedText" (probably not...) or > "RenderTextTest.TruncatedText/HarfBuzz" or similar? For INSTANTIATE_TEST_CASE_P(prefix, test_case_name); The test is printed as prefix.test_case_name.test_name/{parameterised value tested} Have changed so that the tests are printed as- RenderTextTest.TruncatedText/HarfBuzz RenderTextTest.TruncatedText/Mac RenderTextHarfBuzzTest.ObscuredText/HarfBuzz RenderTextMacTest.Mac_ElidedText/Mac
lgtm with a nit, and maybe leave the valgrind exception in for now. https://codereview.chromium.org/2251893004/diff/280001/tools/valgrind/gtest_e... File tools/valgrind/gtest_exclude/ui_base_unittests.gtest-memcheck.txt (right): https://codereview.chromium.org/2251893004/diff/280001/tools/valgrind/gtest_e... tools/valgrind/gtest_exclude/ui_base_unittests.gtest-memcheck.txt:2: RenderTextHarfBuzzTest.DisplayRectShowsCursorLTR On 2016/08/25 08:13:10, karandeepb wrote: > On 2016/08/25 03:02:19, msw wrote: > > I wonder if we can remove this exclusion; the cited bug is closed (but not > > directly related?) > > I am also not sure. Since the cited bug is closed, we can probably see if > removing this causes a failure. That way we can at least file a new bug. Sorry, I should have suggested trying this in a separate CL, I'll feel bad if this CL were reverted just because of this speculative change. https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2251893004/diff/280001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:3736: RenderTextTestAll, On 2016/08/25 08:13:10, karandeepb wrote: > On 2016/08/25 03:02:19, msw wrote: > > nit: the test logs print as > > "RenderTextTestAll.TruncatedText/RenderTextHarfBuzz", could we get them to > print > > either: "RenderTextHarfBuzzTest.TruncatedText" (probably not...) or > > "RenderTextTest.TruncatedText/HarfBuzz" or similar? > > For INSTANTIATE_TEST_CASE_P(prefix, test_case_name); > > The test is printed as > > prefix.test_case_name.test_name/{parameterised value tested} > > Have changed so that the tests are printed as- > RenderTextTest.TruncatedText/HarfBuzz > RenderTextTest.TruncatedText/Mac > RenderTextHarfBuzzTest.ObscuredText/HarfBuzz > RenderTextMacTest.Mac_ElidedText/Mac Nice; thanks! https://codereview.chromium.org/2251893004/diff/320001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2251893004/diff/320001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:304: return base::WrapUnique(new RenderTextHarfBuzz()); nit: prefer MakeUnique over WrapUnique.
https://codereview.chromium.org/2251893004/diff/280001/tools/valgrind/gtest_e... File tools/valgrind/gtest_exclude/ui_base_unittests.gtest-memcheck.txt (right): https://codereview.chromium.org/2251893004/diff/280001/tools/valgrind/gtest_e... tools/valgrind/gtest_exclude/ui_base_unittests.gtest-memcheck.txt:2: RenderTextHarfBuzzTest.DisplayRectShowsCursorLTR On 2016/08/25 15:20:37, msw wrote: > On 2016/08/25 08:13:10, karandeepb wrote: > > On 2016/08/25 03:02:19, msw wrote: > > > I wonder if we can remove this exclusion; the cited bug is closed (but not > > > directly related?) > > > > I am also not sure. Since the cited bug is closed, we can probably see if > > removing this causes a failure. That way we can at least file a new bug. > > Sorry, I should have suggested trying this in a separate CL, I'll feel bad if > this CL were reverted just because of this speculative change. Sure, will do this in a separate change. https://codereview.chromium.org/2251893004/diff/320001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2251893004/diff/320001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:304: return base::WrapUnique(new RenderTextHarfBuzz()); On 2016/08/25 15:20:37, msw wrote: > nit: prefer MakeUnique over WrapUnique. Done.
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2251893004/#ps340001 (title: "Address review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Use parameterized tests to test multiple render text implementations. Currently, on Mac most of the tests in render_text_unittest.cc use RenderText::CreateInstance to create their render text instance. Also, many tests which would fail on Mac with RenderTextMac are not compiled on Mac. As a result there is negligible test coverage for RenderTextHarfBuzz on Mac. This CL uses parameterized tests to test both RenderTextMac and RenderTextHarfBuzz on Mac. It does this by introducing three new test fixture classes which support parameterized tests- -RenderTextTest -RenderTextHarfBuzzTest -RenderTextMacTest The test helper class RenderTextAllBackends is removed. Before this CL, 65 tests in render_text_unittest.cc are run on Mac. After this CL, a total of 129 tests in render_text_unittest.cc are run on Mac. BUG=639194 ========== to ========== Use parameterized tests to test multiple render text implementations. Currently, on Mac most of the tests in render_text_unittest.cc use RenderText::CreateInstance to create their render text instance. Also, many tests which would fail on Mac with RenderTextMac are not compiled on Mac. As a result there is negligible test coverage for RenderTextHarfBuzz on Mac. This CL uses parameterized tests to test both RenderTextMac and RenderTextHarfBuzz on Mac. It does this by introducing three new test fixture classes which support parameterized tests- -RenderTextTest -RenderTextHarfBuzzTest -RenderTextMacTest The test helper class RenderTextAllBackends is removed. Before this CL, 65 tests in render_text_unittest.cc are run on Mac. After this CL, a total of 129 tests in render_text_unittest.cc are run on Mac. BUG=639194 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Use parameterized tests to test multiple render text implementations. Currently, on Mac most of the tests in render_text_unittest.cc use RenderText::CreateInstance to create their render text instance. Also, many tests which would fail on Mac with RenderTextMac are not compiled on Mac. As a result there is negligible test coverage for RenderTextHarfBuzz on Mac. This CL uses parameterized tests to test both RenderTextMac and RenderTextHarfBuzz on Mac. It does this by introducing three new test fixture classes which support parameterized tests- -RenderTextTest -RenderTextHarfBuzzTest -RenderTextMacTest The test helper class RenderTextAllBackends is removed. Before this CL, 65 tests in render_text_unittest.cc are run on Mac. After this CL, a total of 129 tests in render_text_unittest.cc are run on Mac. BUG=639194 ========== to ========== Use parameterized tests to test multiple render text implementations. Currently, on Mac most of the tests in render_text_unittest.cc use RenderText::CreateInstance to create their render text instance. Also, many tests which would fail on Mac with RenderTextMac are not compiled on Mac. As a result there is negligible test coverage for RenderTextHarfBuzz on Mac. This CL uses parameterized tests to test both RenderTextMac and RenderTextHarfBuzz on Mac. It does this by introducing three new test fixture classes which support parameterized tests- -RenderTextTest -RenderTextHarfBuzzTest -RenderTextMacTest The test helper class RenderTextAllBackends is removed. Before this CL, 65 tests in render_text_unittest.cc are run on Mac. After this CL, a total of 129 tests in render_text_unittest.cc are run on Mac. BUG=639194 Committed: https://crrev.com/50b6fa15b332c7f049069a1f569ca1e374462601 Cr-Commit-Position: refs/heads/master@{#414644} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/50b6fa15b332c7f049069a1f569ca1e374462601 Cr-Commit-Position: refs/heads/master@{#414644} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
