Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2843653002:
[DIAL tests] Slightly improve DialRegistry tests. (Closed)
Description[DIAL tests] Slightly improve DialRegistry tests.
Update the TestDeviceExpires test case with a slightly more complicated
scenario.
Also, to make the tests more robust, we will use a testing clock to
generate the base::Time for various objects. DialRegistry::Now() is
replaced with a clock that can be swapped out in tests.
BUG=661457
Review-Url: https://codereview.chromium.org/2843653002
Cr-Commit-Position: refs/heads/master@{#468556}
Committed: https://chromium.googlesource.com/chromium/src/+/8780ab5722f24aa3f463b2b5838b5ae229b3f946
Patch Set 1 : #
Total comments: 4
Patch Set 2 : Addressed Bin's comments #
Total comments: 6
Patch Set 3 : Addressed mark's comments #
Messages
Total messages: 24 (16 generated)
Patchset #1 (id:1) has been deleted
imcheng@chromium.org changed reviewers: + zhaobin@chromium.org
Bin: PTAL. Thanks!
lgtm https://codereview.chromium.org/2843653002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_registry_unittest.cc (right): https://codereview.chromium.org/2843653002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_registry_unittest.cc:48: explicit MockDialRegistry(base::Clock* clock) : DialRegistry() { Make MockDialRegistry own a SimpleTestClock, and add base::Clock* GetClock() function to return the clock? Do not feel strongly though. https://codereview.chromium.org/2843653002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_registry_unittest.cc:205: EXPECT_CALL(registry_->mock_service(), Discover()); Move EXPECT_CALL() to lines before registry_->DoDiscovry()? So it is more clear what to expect after calling each registry_->DoDiscovery()?
The CQ bit was checked by imcheng@chromium.org to run a CQ dry run
imcheng@chromium.org changed reviewers: + mfoltz@chromium.org
+mfoltz for committers but feel free to take a look at the patch too. https://codereview.chromium.org/2843653002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_registry_unittest.cc (right): https://codereview.chromium.org/2843653002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_registry_unittest.cc:48: explicit MockDialRegistry(base::Clock* clock) : DialRegistry() { On 2017/04/25 20:45:23, zhaobin wrote: > Make MockDialRegistry own a SimpleTestClock, and add base::Clock* GetClock() > function to return the clock? Do not feel strongly though. Done. I made the method return base::SimpleTestClock since I need to be able to Advance() it in test. https://codereview.chromium.org/2843653002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_registry_unittest.cc:205: EXPECT_CALL(registry_->mock_service(), Discover()); On 2017/04/25 20:45:23, zhaobin wrote: > Move EXPECT_CALL() to lines before registry_->DoDiscovry()? So it is more clear > what to expect after calling each registry_->DoDiscovery()? Done. I made it a bit more stringent by verifying the mocks after each batch of registry calls, but it does require replacing the SetListenerExpectations() call with expectations in beginning and end. We can probably do the same with the other tests, but don't feel a strong need for that since many of them have InSequence already.
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.
LGTM https://codereview.chromium.org/2843653002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_registry_unittest.cc (right): https://codereview.chromium.org/2843653002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_registry_unittest.cc:78: base::SimpleTestClock* const clock_; It would seem simpler for DialRegistryTest to hold a pointer to clock_? https://codereview.chromium.org/2843653002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_registry_unittest.cc:103: void ResetMockExpectations() { Prefer VerifyAndResetMocks() - when I read the tests below it wasn't clear when expectations were verified. https://codereview.chromium.org/2843653002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_registry_unittest.cc:233: // First device has expired, second device has not expired yet. How is the expiration time set for the test devices?
https://codereview.chromium.org/2843653002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/dial/dial_registry_unittest.cc (right): https://codereview.chromium.org/2843653002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_registry_unittest.cc:78: base::SimpleTestClock* const clock_; On 2017/05/01 21:01:10, mark a. foltz wrote: > It would seem simpler for DialRegistryTest to hold a pointer to clock_? That's what I had originally, but the clock is more closely related to the DialRegistry. Not a big difference either way. https://codereview.chromium.org/2843653002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_registry_unittest.cc:103: void ResetMockExpectations() { On 2017/05/01 21:01:10, mark a. foltz wrote: > Prefer VerifyAndResetMocks() - when I read the tests below it wasn't clear when > expectations were verified. Done. https://codereview.chromium.org/2843653002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/dial/dial_registry_unittest.cc:233: // First device has expired, second device has not expired yet. On 2017/05/01 21:01:10, mark a. foltz wrote: > How is the expiration time set for the test devices? The response time (which determines expiry time) are set to Now() during construction. For the second device, I made a copy and then modified the response time manually.
The CQ bit was checked by imcheng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zhaobin@chromium.org, mfoltz@chromium.org Link to the patchset: https://codereview.chromium.org/2843653002/#ps60001 (title: "Addressed mark's comments")
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 imcheng@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 imcheng@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": 60001, "attempt_start_ts": 1493699069485390,
"parent_rev": "f6ff4cd290401c5720e534e52f7ed04544b06e58", "commit_rev":
"8780ab5722f24aa3f463b2b5838b5ae229b3f946"}
Message was sent while issue was closed.
Description was changed from ========== [DIAL tests] Slightly improve DialRegistry tests. Update the TestDeviceExpires test case with a slightly more complicated scenario. Also, to make the tests more robust, we will use a testing clock to generate the base::Time for various objects. DialRegistry::Now() is replaced with a clock that can be swapped out in tests. BUG=661457 ========== to ========== [DIAL tests] Slightly improve DialRegistry tests. Update the TestDeviceExpires test case with a slightly more complicated scenario. Also, to make the tests more robust, we will use a testing clock to generate the base::Time for various objects. DialRegistry::Now() is replaced with a clock that can be swapped out in tests. BUG=661457 Review-Url: https://codereview.chromium.org/2843653002 Cr-Commit-Position: refs/heads/master@{#468556} Committed: https://chromium.googlesource.com/chromium/src/+/8780ab5722f24aa3f463b2b5838b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/8780ab5722f24aa3f463b2b5838b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
