Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2038183002:
Refactor ResizingHostObserver tests (Closed)
DescriptionRefactor ResizingHostObserver tests
Remove a couple of possible sources of dangling pointers and make the
tests cleaner and more compact.
Committed: https://crrev.com/7142e66c06dcd7f7a5b33dec7004a700231b951d
Cr-Commit-Position: refs/heads/master@{#398152}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Attempt to better follow style-guide recommendations #Patch Set 3 : Address comments #
Total comments: 2
Patch Set 4 : Change CHECK to ASSERT; change some EXPECTs to have expected value first #Patch Set 5 : Replace {} with std::vector<ScreenResolution>() #Messages
Total messages: 15 (3 generated)
rkjnsn@chromium.org changed reviewers: + jamiewalch@chromium.org, lambroslambrou@chromium.org
I refactored the ResizingHostObserver a bit to make things safer and easier. https://codereview.chromium.org/2038183002/diff/1/remoting/host/resizing_host... File remoting/host/resizing_host_observer_unittest.cc (right): https://codereview.chromium.org/2038183002/diff/1/remoting/host/resizing_host... remoting/host/resizing_host_observer_unittest.cc:144: std::unique_ptr<ResizingHostObserver> resizing_host_observer_ = nullptr; ` = nullptr` isn't actually necessary, because that's what unique_ptr will do by default. I put it here because the previous version was explicitly initializing this field to `nullptr`. I'm not sure if it improves clarity.
LGTM with nits. https://codereview.chromium.org/2038183002/diff/1/remoting/host/resizing_host... File remoting/host/resizing_host_observer_unittest.cc (right): https://codereview.chromium.org/2038183002/diff/1/remoting/host/resizing_host... remoting/host/resizing_host_observer_unittest.cc:106: void SetDesktopResizer(const ScreenResolution& initial_resolution, Maybe rename this InitDesktopResizer, since it's now doing more than just set a field. https://codereview.chromium.org/2038183002/diff/1/remoting/host/resizing_host... remoting/host/resizing_host_observer_unittest.cc:128: for (auto client = client_sizes.begin(), expected = expected_sizes.begin(); Since we can validate that they are the same size now, we should. https://codereview.chromium.org/2038183002/diff/1/remoting/host/resizing_host... remoting/host/resizing_host_observer_unittest.cc:144: std::unique_ptr<ResizingHostObserver> resizing_host_observer_ = nullptr; On 2016/06/03 21:33:37, rkjnsn wrote: > ` = nullptr` isn't actually necessary, because that's what unique_ptr will do by > default. I put it here because the previous version was explicitly initializing > this field to `nullptr`. I'm not sure if it improves clarity. I think it's fine to remove this. The original code predates unique_ptr, and perhaps the predecessor didn't have a default ctor. https://codereview.chromium.org/2038183002/diff/1/remoting/host/resizing_host... remoting/host/resizing_host_observer_unittest.cc:151: VerifySizes({}, {}); I don't think this is needed (and wasn't needed before your change). Passing a compile-time constant for client_sizes is not actually testing anything other than the VerifySizes function.
https://codereview.chromium.org/2038183002/diff/1/remoting/host/resizing_host... File remoting/host/resizing_host_observer_unittest.cc (right): https://codereview.chromium.org/2038183002/diff/1/remoting/host/resizing_host... remoting/host/resizing_host_observer_unittest.cc:106: void SetDesktopResizer(const ScreenResolution& initial_resolution, On 2016/06/03 23:47:20, Jamie wrote: > Maybe rename this InitDesktopResizer, since it's now doing more than just set a > field. Acknowledged. https://codereview.chromium.org/2038183002/diff/1/remoting/host/resizing_host... remoting/host/resizing_host_observer_unittest.cc:128: for (auto client = client_sizes.begin(), expected = expected_sizes.begin(); On 2016/06/03 23:47:20, Jamie wrote: > Since we can validate that they are the same size now, we should. Would CHECK_EQ be the preferred way of doing so? Would you leave the for loop the same, or remove one of the termination conditions? https://codereview.chromium.org/2038183002/diff/1/remoting/host/resizing_host... remoting/host/resizing_host_observer_unittest.cc:144: std::unique_ptr<ResizingHostObserver> resizing_host_observer_ = nullptr; On 2016/06/03 23:47:20, Jamie wrote: > On 2016/06/03 21:33:37, rkjnsn wrote: > > ` = nullptr` isn't actually necessary, because that's what unique_ptr will do > by > > default. I put it here because the previous version was explicitly > initializing > > this field to `nullptr`. I'm not sure if it improves clarity. > > I think it's fine to remove this. The original code predates unique_ptr, and > perhaps the predecessor didn't have a default ctor. Acknowledged. https://codereview.chromium.org/2038183002/diff/1/remoting/host/resizing_host... remoting/host/resizing_host_observer_unittest.cc:151: VerifySizes({}, {}); On 2016/06/03 23:47:20, Jamie wrote: > I don't think this is needed (and wasn't needed before your change). Passing a > compile-time constant for client_sizes is not actually testing anything other > than the VerifySizes function. Acknowledged.
https://codereview.chromium.org/2038183002/diff/1/remoting/host/resizing_host... File remoting/host/resizing_host_observer_unittest.cc (right): https://codereview.chromium.org/2038183002/diff/1/remoting/host/resizing_host... remoting/host/resizing_host_observer_unittest.cc:128: for (auto client = client_sizes.begin(), expected = expected_sizes.begin(); On 2016/06/04 00:08:21, rkjnsn wrote: > On 2016/06/03 23:47:20, Jamie wrote: > > Since we can validate that they are the same size now, we should. > > Would CHECK_EQ be the preferred way of doing so? Would you leave the for loop > the same, or remove one of the termination conditions? I don't think it matters tbh. The extra check is just to make sure tests don't accidentally pass if you have a list size mismatch.
https://codereview.chromium.org/2038183002/diff/1/remoting/host/resizing_host... File remoting/host/resizing_host_observer_unittest.cc (right): https://codereview.chromium.org/2038183002/diff/1/remoting/host/resizing_host... remoting/host/resizing_host_observer_unittest.cc:128: for (auto client = client_sizes.begin(), expected = expected_sizes.begin(); On 2016/06/04 00:29:48, Jamie wrote: > On 2016/06/04 00:08:21, rkjnsn wrote: > > On 2016/06/03 23:47:20, Jamie wrote: > > > Since we can validate that they are the same size now, we should. > > > > Would CHECK_EQ be the preferred way of doing so? Would you leave the for loop > > the same, or remove one of the termination conditions? > > I don't think it matters tbh. The extra check is just to make sure tests don't > accidentally pass if you have a list size mismatch. Sorry, I missed the first half of your question. It's been a while since I wrote C++ unit tests; is CHECK_EQ more like an assert than EXPECT_EQ? If so, then it seems appropriate, since both values are supplied by test code, so if they don't match it's not a failure of the code under test.
https://codereview.chromium.org/2038183002/diff/1/remoting/host/resizing_host... File remoting/host/resizing_host_observer_unittest.cc (right): https://codereview.chromium.org/2038183002/diff/1/remoting/host/resizing_host... remoting/host/resizing_host_observer_unittest.cc:128: for (auto client = client_sizes.begin(), expected = expected_sizes.begin(); On 2016/06/04 00:32:05, Jamie wrote: > On 2016/06/04 00:29:48, Jamie wrote: > > On 2016/06/04 00:08:21, rkjnsn wrote: > > > On 2016/06/03 23:47:20, Jamie wrote: > > > > Since we can validate that they are the same size now, we should. > > > > > > Would CHECK_EQ be the preferred way of doing so? Would you leave the for > loop > > > the same, or remove one of the termination conditions? > > > > I don't think it matters tbh. The extra check is just to make sure tests don't > > accidentally pass if you have a list size mismatch. > > Sorry, I missed the first half of your question. It's been a while since I wrote > C++ unit tests; is CHECK_EQ more like an assert than EXPECT_EQ? If so, then it > seems appropriate, since both values are supplied by test code, so if they don't > match it's not a failure of the code under test. Honestly, I have no idea how the various macros differ. The previous code had `CHECK(!desktop_resizer_) << "Call SetDeskopResizer once per test";`, which is similarly verifying the behavior of the test code itself, so I figured this might call for something similar.
Updated to address comments. https://codereview.chromium.org/2038183002/diff/40001/remoting/host/resizing_... File remoting/host/resizing_host_observer_unittest.cc (right): https://codereview.chromium.org/2038183002/diff/40001/remoting/host/resizing_... remoting/host/resizing_host_observer_unittest.cc:129: << "Client and expected vectors must have the same length"; Is CHECK the appropriate thing to use here?
lgtm https://codereview.chromium.org/2038183002/diff/1/remoting/host/resizing_host... File remoting/host/resizing_host_observer_unittest.cc (right): https://codereview.chromium.org/2038183002/diff/1/remoting/host/resizing_host... remoting/host/resizing_host_observer_unittest.cc:128: for (auto client = client_sizes.begin(), expected = expected_sizes.begin(); On 2016/06/04 00:43:53, rkjnsn wrote: > On 2016/06/04 00:32:05, Jamie wrote: > > On 2016/06/04 00:29:48, Jamie wrote: > > > On 2016/06/04 00:08:21, rkjnsn wrote: > > > > On 2016/06/03 23:47:20, Jamie wrote: > > > > > Since we can validate that they are the same size now, we should. > > > > > > > > Would CHECK_EQ be the preferred way of doing so? Would you leave the for > > loop > > > > the same, or remove one of the termination conditions? > > > > > > I don't think it matters tbh. The extra check is just to make sure tests > don't > > > accidentally pass if you have a list size mismatch. > > > > Sorry, I missed the first half of your question. It's been a while since I > wrote > > C++ unit tests; is CHECK_EQ more like an assert than EXPECT_EQ? If so, then it > > seems appropriate, since both values are supplied by test code, so if they > don't > > match it's not a failure of the code under test. > > Honestly, I have no idea how the various macros differ. The previous code had > `CHECK(!desktop_resizer_) << "Call SetDeskopResizer once per test";`, which is > similarly verifying the behavior of the test code itself, so I figured this > might call for something similar. Basically, CHECK does the same thing it does in production code--crashes the process if it fails. You should only use it if a failure indicates something wrong with the test code itself (because even a catastrophic failure in the code under test should be indicated by a test failure, not a test crash). The difference between ASSERT and TEST is whether or not the test code continues to run in case of failure; TEST will continue, ASSERT will abort. https://codereview.chromium.org/2038183002/diff/40001/remoting/host/resizing_... File remoting/host/resizing_host_observer_unittest.cc (right): https://codereview.chromium.org/2038183002/diff/40001/remoting/host/resizing_... remoting/host/resizing_host_observer_unittest.cc:129: << "Client and expected vectors must have the same length"; On 2016/06/06 18:26:47, rkjnsn wrote: > Is CHECK the appropriate thing to use here? See my other comment. Since a failure here indicates a bug in the test code, either ASSERT or CHECK would be appropriate. You could grep our test code to see which is more common if you're interested.
lgtm
The CQ bit was checked by rkjnsn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2038183002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Refactor ResizingHostObserver tests Remove a couple of possible sources of dangling pointers and make the tests cleaner and more compact. ========== to ========== Refactor ResizingHostObserver tests Remove a couple of possible sources of dangling pointers and make the tests cleaner and more compact. Committed: https://crrev.com/7142e66c06dcd7f7a5b33dec7004a700231b951d Cr-Commit-Position: refs/heads/master@{#398152} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7142e66c06dcd7f7a5b33dec7004a700231b951d Cr-Commit-Position: refs/heads/master@{#398152} |
