Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2578073002:
Worker: Supress gmock warnings in WorkerThreadTest (Closed)
|
Created:
4 years ago by nhiroki Modified:
4 years ago Reviewers:
falken CC:
chromium-reviews, shimazu+worker_chromium.org, kinuko+worker_chromium.org, blink-reviews, horo+watch_chromium.org, falken+watch_chromium.org, blink-worker-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWorker: Supress gmock warnings in WorkerThreadTest
These warnings started emerging after:
https://codereview.chromium.org/2535093003
BUG=376039, 667357
Committed: https://crrev.com/ee81deeea47deea00c48f85c44bb7d17e82b4db0
Cr-Commit-Position: refs/heads/master@{#439420}
Patch Set 1 #
Total comments: 2
Patch Set 2 : replace AtLeast with AnyNumber #Messages
Total messages: 17 (10 generated)
The CQ bit was checked by nhiroki@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...
nhiroki@chromium.org changed reviewers: + falken@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2578073002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp (right): https://codereview.chromium.org/2578073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp:88: EXPECT_CALL(*m_reportingProxy, countFeature(_)).Times(AtLeast(1)); I don't have a strong opinion but the warning says "this can be ignored" and recommends against adding an EXPECT_CALL just to suppress it. The linked doc recommends using ON_CALL or NiceMock, can that be used here? https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#... Also do we really want to enforce countFeature is called here or could it be AnyNumber() instead of AtLeast(1)?
Patchset #2 (id:20001) has been deleted
Thank you. Updated. https://codereview.chromium.org/2578073002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp (right): https://codereview.chromium.org/2578073002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/workers/WorkerThreadTest.cpp:88: EXPECT_CALL(*m_reportingProxy, countFeature(_)).Times(AtLeast(1)); On 2016/12/15 15:36:25, falken wrote: > I don't have a strong opinion but the warning says "this can be ignored" and > recommends against adding an EXPECT_CALL just to suppress it. The linked doc > recommends using ON_CALL or NiceMock, can that be used here? > > https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#... Thank you for the information. I'd prefer to explicitly suppress warnings because it'd be helpful for future readers to judge whether this is an uninteresting call or an author simply forgot to add an expectation. NiceMock seems to unconditionally hide all uninteresting calls that will be added by future changes and we might easily forget to add expectations. I think this is not desirable. Regarding ON_CALL, IIUC this is used for overriding a default action, not used for suppressing warnings. Actually I tried ON_CALL(*m_reportingProxy, countFeature(_)).WillByDefault(Return()), but it emitted a warning message. > Also do we really want to enforce countFeature is called here or could it be > AnyNumber() instead of AtLeast(1)? countFeature() is tested in unit tests for each global scope, so AnyNumber() should be OK. Replaced.
OK thanks for considering. LGTM.
The CQ bit was checked by nhiroki@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": 40001, "attempt_start_ts": 1482126436695480,
"parent_rev": "5f59f1a23694f138f25131e0befbe1d7ecba580c", "commit_rev":
"7cd80a17a8a44c11e25f360a2459d13276e0e261"}
Message was sent while issue was closed.
Description was changed from ========== Worker: Supress gmock warnings in WorkerThreadTest These warnings started emerging after: https://codereview.chromium.org/2535093003 BUG=376039, 667357 ========== to ========== Worker: Supress gmock warnings in WorkerThreadTest These warnings started emerging after: https://codereview.chromium.org/2535093003 BUG=376039, 667357 Review-Url: https://codereview.chromium.org/2578073002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Worker: Supress gmock warnings in WorkerThreadTest These warnings started emerging after: https://codereview.chromium.org/2535093003 BUG=376039, 667357 Review-Url: https://codereview.chromium.org/2578073002 ========== to ========== Worker: Supress gmock warnings in WorkerThreadTest These warnings started emerging after: https://codereview.chromium.org/2535093003 BUG=376039, 667357 Committed: https://crrev.com/ee81deeea47deea00c48f85c44bb7d17e82b4db0 Cr-Commit-Position: refs/heads/master@{#439420} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ee81deeea47deea00c48f85c44bb7d17e82b4db0 Cr-Commit-Position: refs/heads/master@{#439420} |
