Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2023973002:
Introduce cc StructTraits unit tests (Closed)
|
Created:
4 years, 6 months ago by Fady Samuel Modified:
4 years, 6 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, cc-bugs_chromium.org, chromium-reviews, darin (slow to review), qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce cc StructTraits unit tests
BUG=611802
TBR=danakj@chromium.org for cc/test
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/410c960e9ad39ac706a14b57f158f32d9f83ce70
Cr-Commit-Position: refs/heads/master@{#396941}
Patch Set 1 #Patch Set 2 : Added RenderPassId test #Patch Set 3 : Fixed gyp build #Patch Set 4 : Remove unnecessary MessageLoop #
Total comments: 10
Patch Set 5 : Addressed Ken's comment #
Total comments: 6
Patch Set 6 : Added missing gyp dependency #Patch Set 7 : Pass input by value #Patch Set 8 : Capture constants #
Total comments: 5
Patch Set 9 : Moved DEPS #
Messages
Total messages: 46 (18 generated)
Description was changed from ========== Introduce cc StructTraits unit tests BUG= ========== to ========== Introduce cc StructTraits unit tests BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== Introduce cc StructTraits unit tests BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Introduce cc StructTraits unit tests BUG=611802 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
fsamuel@chromium.org changed reviewers: + danakj@chromium.org, tsepez@chromium.org
+danakj@ for cc OWNERs +tsepez for ipc
The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2023973002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2023973002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
fsamuel@chromium.org changed reviewers: + rockot@chromium.org
+rockot@ for mojo deps in cc.
The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2023973002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2023973002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2023973002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2023973002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2023973002/diff/60001/cc/BUILD.gn File cc/BUILD.gn (right): https://codereview.chromium.org/2023973002/diff/60001/cc/BUILD.gn#newcode962 cc/BUILD.gn:962: "//mojo/edk/system", You should also include a dependency on "//mojo/public/cpp/bindings" https://codereview.chromium.org/2023973002/diff/60001/cc/BUILD.gn#newcode963 cc/BUILD.gn:963: "//mojo/edk/test:test_support", And if you get rid of SCopedIPCSupport you can get rid of this https://codereview.chromium.org/2023973002/diff/60001/cc/ipc/DEPS File cc/ipc/DEPS (right): https://codereview.chromium.org/2023973002/diff/60001/cc/ipc/DEPS#newcode8 cc/ipc/DEPS:8: "+mojo/public/cpp/bindings/binding_set.h", Any reason not to just add +mojo/public to the include_rules? I think specific_include_rules should be reserved for exceptional cases. Allowing mojo/public does not seem like it should be a problem for cc/ipc. https://codereview.chromium.org/2023973002/diff/60001/cc/test/run_all_unittes... File cc/test/run_all_unittests.cc (right): https://codereview.chromium.org/2023973002/diff/60001/cc/test/run_all_unittes... cc/test/run_all_unittests.cc:9: #include "mojo/edk/embedder/embedder.h" // nogncheck Guess we need to fix this target somehow. This is annoying. https://codereview.chromium.org/2023973002/diff/60001/cc/test/run_all_unittes... cc/test/run_all_unittests.cc:15: mojo::edk::Init(); If you're not doing actual IPC (i.e. no multiprocess tests) you only need mojo::edk::Init(). You can omit the IO thread and ScopedIPCSupport.
On 2016/05/31 at 15:49:22, Ken Rockot wrote: > https://codereview.chromium.org/2023973002/diff/60001/cc/BUILD.gn > File cc/BUILD.gn (right): > > https://codereview.chromium.org/2023973002/diff/60001/cc/BUILD.gn#newcode962 > cc/BUILD.gn:962: "//mojo/edk/system", > You should also include a dependency on "//mojo/public/cpp/bindings" > > https://codereview.chromium.org/2023973002/diff/60001/cc/BUILD.gn#newcode963 > cc/BUILD.gn:963: "//mojo/edk/test:test_support", > And if you get rid of SCopedIPCSupport you can get rid of this > > https://codereview.chromium.org/2023973002/diff/60001/cc/ipc/DEPS > File cc/ipc/DEPS (right): > > https://codereview.chromium.org/2023973002/diff/60001/cc/ipc/DEPS#newcode8 > cc/ipc/DEPS:8: "+mojo/public/cpp/bindings/binding_set.h", > Any reason not to just add +mojo/public to the include_rules? I think specific_include_rules should be reserved for exceptional cases. Allowing mojo/public does not seem like it should be a problem for cc/ipc. > > https://codereview.chromium.org/2023973002/diff/60001/cc/test/run_all_unittes... > File cc/test/run_all_unittests.cc (right): > > https://codereview.chromium.org/2023973002/diff/60001/cc/test/run_all_unittes... > cc/test/run_all_unittests.cc:9: #include "mojo/edk/embedder/embedder.h" // nogncheck > Guess we need to fix this target somehow. This is annoying. BTW to clarify: this is just a general comment - no action required for this CL > > https://codereview.chromium.org/2023973002/diff/60001/cc/test/run_all_unittes... > cc/test/run_all_unittests.cc:15: mojo::edk::Init(); > If you're not doing actual IPC (i.e. no multiprocess tests) you only need mojo::edk::Init(). You can omit the IO thread and ScopedIPCSupport.
PTAL Ken! https://codereview.chromium.org/2023973002/diff/60001/cc/BUILD.gn File cc/BUILD.gn (right): https://codereview.chromium.org/2023973002/diff/60001/cc/BUILD.gn#newcode962 cc/BUILD.gn:962: "//mojo/edk/system", On 2016/05/31 15:49:22, Ken Rockot wrote: > You should also include a dependency on "//mojo/public/cpp/bindings" Done. https://codereview.chromium.org/2023973002/diff/60001/cc/BUILD.gn#newcode963 cc/BUILD.gn:963: "//mojo/edk/test:test_support", On 2016/05/31 15:49:22, Ken Rockot wrote: > And if you get rid of SCopedIPCSupport you can get rid of this Done. https://codereview.chromium.org/2023973002/diff/60001/cc/ipc/DEPS File cc/ipc/DEPS (right): https://codereview.chromium.org/2023973002/diff/60001/cc/ipc/DEPS#newcode8 cc/ipc/DEPS:8: "+mojo/public/cpp/bindings/binding_set.h", On 2016/05/31 15:49:22, Ken Rockot wrote: > Any reason not to just add +mojo/public to the include_rules? I think > specific_include_rules should be reserved for exceptional cases. Allowing > mojo/public does not seem like it should be a problem for cc/ipc. Done. https://codereview.chromium.org/2023973002/diff/60001/cc/test/run_all_unittes... File cc/test/run_all_unittests.cc (right): https://codereview.chromium.org/2023973002/diff/60001/cc/test/run_all_unittes... cc/test/run_all_unittests.cc:9: #include "mojo/edk/embedder/embedder.h" // nogncheck On 2016/05/31 15:49:22, Ken Rockot wrote: > Guess we need to fix this target somehow. This is annoying. Looks like it's not actually necessary anymore. Removed. https://codereview.chromium.org/2023973002/diff/60001/cc/test/run_all_unittes... cc/test/run_all_unittests.cc:15: mojo::edk::Init(); On 2016/05/31 15:49:22, Ken Rockot wrote: > If you're not doing actual IPC (i.e. no multiprocess tests) you only need > mojo::edk::Init(). You can omit the IO thread and ScopedIPCSupport. Done.
The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2023973002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2023973002/80001
lgtm https://codereview.chromium.org/2023973002/diff/80001/cc/cc_tests.gyp File cc/cc_tests.gyp (right): https://codereview.chromium.org/2023973002/diff/80001/cc/cc_tests.gyp#newcode341 cc/cc_tests.gyp:341: '../mojo/mojo_edk.gyp:mojo_common_test_support', Oops, missed this. You should also include a dep on ../mojo/mojo_public.gyp:mojo_cpp_bindings
PTAL Tom and Dana. https://codereview.chromium.org/2023973002/diff/80001/cc/cc_tests.gyp File cc/cc_tests.gyp (right): https://codereview.chromium.org/2023973002/diff/80001/cc/cc_tests.gyp#newcode341 cc/cc_tests.gyp:341: '../mojo/mojo_edk.gyp:mojo_common_test_support', On 2016/05/31 16:00:13, Ken Rockot wrote: > Oops, missed this. You should also include a dep on > ../mojo/mojo_public.gyp:mojo_cpp_bindings Done.
The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2023973002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2023973002/100001
https://codereview.chromium.org/2023973002/diff/80001/cc/ipc/struct_traits_un... File cc/ipc/struct_traits_unittest.cc (right): https://codereview.chromium.org/2023973002/diff/80001/cc/ipc/struct_traits_un... cc/ipc/struct_traits_unittest.cc:48: proxy->EchoRenderPassId(input, [&](const RenderPassId& pass) { I'd expect we'd capture layer_id and index by value, and test against them, so as to be immune to any unexpected transformation of |input|. https://codereview.chromium.org/2023973002/diff/80001/cc/ipc/struct_traits_un... cc/ipc/struct_traits_unittest.cc:64: EXPECT_EQ(input.id_namespace(), pass.id_namespace()); same here.
PTAL Tom! https://codereview.chromium.org/2023973002/diff/80001/cc/ipc/struct_traits_un... File cc/ipc/struct_traits_unittest.cc (right): https://codereview.chromium.org/2023973002/diff/80001/cc/ipc/struct_traits_un... cc/ipc/struct_traits_unittest.cc:48: proxy->EchoRenderPassId(input, [&](const RenderPassId& pass) { On 2016/05/31 16:07:43, Tom Sepez wrote: > I'd expect we'd capture layer_id and index by value, and test against them, so > as to be immune to any unexpected transformation of |input|. I'm passing all of input by value now. WDYT? https://codereview.chromium.org/2023973002/diff/80001/cc/ipc/struct_traits_un... cc/ipc/struct_traits_unittest.cc:64: EXPECT_EQ(input.id_namespace(), pass.id_namespace()); On 2016/05/31 16:07:42, Tom Sepez wrote: > same here. Passing input by value.
PTAL Tom! Now just capturing constants.
The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2023973002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2023973002/140001
lgtm
Description was changed from ========== Introduce cc StructTraits unit tests BUG=611802 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Introduce cc StructTraits unit tests BUG=611802 TBR=danakj@chromium.org for cc/test CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
https://codereview.chromium.org/2023973002/diff/140001/cc/ipc/struct_traits_u... File cc/ipc/struct_traits_unittest.cc (right): https://codereview.chromium.org/2023973002/diff/140001/cc/ipc/struct_traits_u... cc/ipc/struct_traits_unittest.cc:48: proxy->EchoRenderPassId(input, How is this testing the traits? This calls a method with a RenderPassId, and a callback that takes a RenderPassId, the method forwards the reference to that RenderPassId to the method, which checks equality. So the EXPECT things here are checking a reference to |input| against the locals we used here to initialize it?
https://codereview.chromium.org/2023973002/diff/140001/cc/ipc/struct_traits_u... File cc/ipc/struct_traits_unittest.cc (right): https://codereview.chromium.org/2023973002/diff/140001/cc/ipc/struct_traits_u... cc/ipc/struct_traits_unittest.cc:48: proxy->EchoRenderPassId(input, On 2016/05/31 18:30:58, danakj wrote: > How is this testing the traits? > > This calls a method with a RenderPassId, and a callback that takes a > RenderPassId, the method forwards the reference to that RenderPassId to the > method, which checks equality. So the EXPECT things here are checking a > reference to |input| against the locals we used here to initialize it? Mojo converts the cc::RenderPassId to a cc::mojom::RenderPassId wire format, sends that over the wire and then back to cc::RenderPassId in the service. TraitsTestService implements such a service. The callback goes back into mojo which converts the returned cc::RenderPassId to cc::mojom::RenderPassId wireformat over the MessagePipe. Thus, this code exercises both serialization and deserialization twice.
https://codereview.chromium.org/2023973002/diff/140001/cc/ipc/struct_traits_u... File cc/ipc/struct_traits_unittest.cc (right): https://codereview.chromium.org/2023973002/diff/140001/cc/ipc/struct_traits_u... cc/ipc/struct_traits_unittest.cc:48: proxy->EchoRenderPassId(input, On 2016/05/31 18:43:05, Fady Samuel wrote: > On 2016/05/31 18:30:58, danakj wrote: > > How is this testing the traits? > > > > This calls a method with a RenderPassId, and a callback that takes a > > RenderPassId, the method forwards the reference to that RenderPassId to the > > method, which checks equality. So the EXPECT things here are checking a > > reference to |input| against the locals we used here to initialize it? > > Mojo converts the cc::RenderPassId to a cc::mojom::RenderPassId wire format, > sends that over the wire and then back to cc::RenderPassId in the service. > TraitsTestService implements such a service. The callback goes back into mojo > which converts the returned cc::RenderPassId to cc::mojom::RenderPassId > wireformat over the MessagePipe. Thus, this code exercises both serialization > and deserialization twice. I see. Maybe it's a bit confusing that the proxy method is the same as the service method. I read this as calling the service. Thanks though. LGTM
https://codereview.chromium.org/2023973002/diff/140001/cc/DEPS File cc/DEPS (right): https://codereview.chromium.org/2023973002/diff/140001/cc/DEPS#newcode24 cc/DEPS:24: specific_include_rules = { Or just put this in cc/test/DEPS?
Thanks everyone! CQ'ing! https://codereview.chromium.org/2023973002/diff/140001/cc/DEPS File cc/DEPS (right): https://codereview.chromium.org/2023973002/diff/140001/cc/DEPS#newcode24 cc/DEPS:24: specific_include_rules = { On 2016/05/31 19:58:41, danakj wrote: > Or just put this in cc/test/DEPS? Done.
The CQ bit was checked by fsamuel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, danakj@chromium.org, tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/2023973002/#ps160001 (title: "Moved DEPS")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2023973002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2023973002/160001
Message was sent while issue was closed.
Description was changed from ========== Introduce cc StructTraits unit tests BUG=611802 TBR=danakj@chromium.org for cc/test CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Introduce cc StructTraits unit tests BUG=611802 TBR=danakj@chromium.org for cc/test CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Introduce cc StructTraits unit tests BUG=611802 TBR=danakj@chromium.org for cc/test CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Introduce cc StructTraits unit tests BUG=611802 TBR=danakj@chromium.org for cc/test CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/410c960e9ad39ac706a14b57f158f32d9f83ce70 Cr-Commit-Position: refs/heads/master@{#396941} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/410c960e9ad39ac706a14b57f158f32d9f83ce70 Cr-Commit-Position: refs/heads/master@{#396941} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
