Test helper functions which return results should have WARN_UNUSED_RESULT annotation |
||||||||
Issue descriptionIt's very often an error to ignore the result of the test helper function call. In many case the result may indicate a failure and the test has to fail. Adding WARN_UNUSED_RESULT to every test helper function which returns success flag will make test failure more discoverable and easier to debug.
,
Oct 31 2017
I this the pattern if forcing the caller to make the decision is pretty good. ASSERT calls produce useful logs if they are a part of the test, and giving the caller a chance to retry is a good way to provide API flexibility.
,
Oct 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bcdcbc98ac675a71e441e765d84157e0e4662426 commit bcdcbc98ac675a71e441e765d84157e0e4662426 Author: Eugene But <eugenebut@google.com> Date: Tue Oct 31 20:31:09 2017 Do not ignore result of WaitUntilConditionOrTimeout. Ignoring the result of WaitUntilConditionOrTimeout call is incorrect in many cases. So WaitUntilConditionOrTimeout will have WARN_UNUSED_RESULT annotation to force callers to do the checks. This CL prepares ios/web for WaitUntilConditionOrTimeout changes. Bug: 780062 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: Id6dbc3f21da5c3c4c907eca5009964dfc67ab510 Reviewed-on: https://chromium-review.googlesource.com/747066 Reviewed-by: Mike Baxley <baxley@chromium.org> Commit-Queue: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/heads/master@{#512939} [modify] https://crrev.com/bcdcbc98ac675a71e441e765d84157e0e4662426/ios/web/public/test/web_view_interaction_test_util.mm [modify] https://crrev.com/bcdcbc98ac675a71e441e765d84157e0e4662426/ios/web/web_state/session_certificate_policy_cache_impl_unittest.mm
,
Nov 1 2017
eugenebut@ can we close this?
,
Nov 1 2017
We can not close this bug. Most of test functions dont have WARN_UNUSED_RESULT
,
Nov 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f0655b360f114000f6702289d92d1ba35c9e4e99 commit f0655b360f114000f6702289d92d1ba35c9e4e99 Author: Eugene But <eugenebut@google.com> Date: Wed Nov 01 20:18:32 2017 Do not ignore result of WaitUntilConditionOrTimeout. Ignoring the result of WaitUntilConditionOrTimeout call is incorrect in many cases. So WaitUntilConditionOrTimeout will have WARN_UNUSED_RESULT annotation to force callers to do the checks. This CL prepares ios/web for WaitUntilConditionOrTimeout changes. Bug: 780062 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: Ide53b4613df407149c43cdfa68751be171db313f Reviewed-on: https://chromium-review.googlesource.com/746995 Reviewed-by: Mike Baxley <baxley@chromium.org> Commit-Queue: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/heads/master@{#513240} [modify] https://crrev.com/f0655b360f114000f6702289d92d1ba35c9e4e99/ios/chrome/browser/ui/bookmarks/bookmarks_new_generation_egtest.mm [modify] https://crrev.com/f0655b360f114000f6702289d92d1ba35c9e4e99/ios/chrome/test/app/static_html_view_test_util.mm [modify] https://crrev.com/f0655b360f114000f6702289d92d1ba35c9e4e99/ios/chrome/test/app/tab_test_util.h [modify] https://crrev.com/f0655b360f114000f6702289d92d1ba35c9e4e99/ios/chrome/test/app/tab_test_util.mm [modify] https://crrev.com/f0655b360f114000f6702289d92d1ba35c9e4e99/ios/web_view/test/web_view_test_util.mm
,
Nov 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7fa8f03f813112bb0237fcc2dbd5f8212eee6e3b commit 7fa8f03f813112bb0237fcc2dbd5f8212eee6e3b Author: Eugene But <eugenebut@google.com> Date: Wed Nov 01 21:32:00 2017 Added WARN_UNUSED_RESULT annotation to WaitUntilConditionOrTimeout. Ignoring the result of WaitUntilConditionOrTimeout call is incorrect in many cases. CL adds WARN_UNUSED_RESULT annotation to WaitUntilConditionOrTimeout to force callers to do the checks. Bug: 780062 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: I96daa8dfc6ec280d7f622a91f474714fe48fe3bf Reviewed-on: https://chromium-review.googlesource.com/748108 Reviewed-by: Mike Baxley <baxley@chromium.org> Commit-Queue: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/heads/master@{#513277} [modify] https://crrev.com/7fa8f03f813112bb0237fcc2dbd5f8212eee6e3b/ios/testing/wait_util.h
,
Nov 1 2017
The following revision refers to this bug: https://chrome-internal.googlesource.com/chrome/ios_internal.git/+/da9ada59806e8f2d9ab4d69ec71280ddce2b2c21 commit da9ada59806e8f2d9ab4d69ec71280ddce2b2c21 Author: Eugene But <eugenebut@google.com> Date: Wed Nov 01 22:27:25 2017
,
Nov 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ef9d50e0b42c24f3de95c856e18a373175ecee00 commit ef9d50e0b42c24f3de95c856e18a373175ecee00 Author: Eugene But <eugenebut@google.com> Date: Thu Nov 02 01:14:32 2017 Do not ignore result of CloseAllIncognitoTabs. Added WARN_UNUSED_RESULT annotation to CloseAllIncognitoTabs, because if the function returns NO, then the calling test should fail. Bug: 780062 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: I5c643a959005d639b48c2b8b436d77227bab3cbf Reviewed-on: https://chromium-review.googlesource.com/747690 Commit-Queue: Eugene But <eugenebut@chromium.org> Reviewed-by: Mike Baxley <baxley@chromium.org> Cr-Commit-Position: refs/heads/master@{#513356} [modify] https://crrev.com/ef9d50e0b42c24f3de95c856e18a373175ecee00/ios/chrome/browser/net/cookies_egtest.mm [modify] https://crrev.com/ef9d50e0b42c24f3de95c856e18a373175ecee00/ios/chrome/browser/ui/bookmarks/bookmarks_new_generation_egtest.mm [modify] https://crrev.com/ef9d50e0b42c24f3de95c856e18a373175ecee00/ios/chrome/browser/ui/ntp/new_tab_page_egtest.mm [modify] https://crrev.com/ef9d50e0b42c24f3de95c856e18a373175ecee00/ios/chrome/browser/ui/settings/settings_egtest.mm [modify] https://crrev.com/ef9d50e0b42c24f3de95c856e18a373175ecee00/ios/chrome/test/app/tab_test_util.h
,
Nov 2 2017
,
Nov 2 2017
Mike (as maintainer of test framework) to triage.
,
Jan 24 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/785470b74c2fa2bb45f36097eb236852de1e28f5 commit 785470b74c2fa2bb45f36097eb236852de1e28f5 Author: Eugene But <eugenebut@google.com> Date: Wed Jan 24 16:54:09 2018 Return bool from WebTestWithWebState::LoadHtml. For now this always retrun true and callers asserts that result is true. In future CLs this method will return false if html has failed to load. Bug: 780062 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: I19555e2dc9e797b466f70df0f644c2e280ba7340 Reviewed-on: https://chromium-review.googlesource.com/875321 Reviewed-by: Mike Baxley <baxley@chromium.org> Commit-Queue: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/heads/master@{#531564} [modify] https://crrev.com/785470b74c2fa2bb45f36097eb236852de1e28f5/ios/chrome/browser/autofill/form_structure_browsertest.mm [modify] https://crrev.com/785470b74c2fa2bb45f36097eb236852de1e28f5/ios/web/public/test/web_test_with_web_state.h [modify] https://crrev.com/785470b74c2fa2bb45f36097eb236852de1e28f5/ios/web/public/test/web_test_with_web_state.mm [modify] https://crrev.com/785470b74c2fa2bb45f36097eb236852de1e28f5/ios/web/web_state/js/context_menu_js_unittest.mm [modify] https://crrev.com/785470b74c2fa2bb45f36097eb236852de1e28f5/ios/web/web_state/web_state_unittest.mm
,
Feb 14 2018
,
Feb 14 2018
,
Feb 22 2018
|
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by baxley@chromium.org
, Oct 31 2017