New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 780062 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Task

Blocking:
issue 814709



Sign in to add a comment

Test helper functions which return results should have WARN_UNUSED_RESULT annotation

Project Member Reported by eugene...@chromium.org, Oct 31 2017

Issue description

It'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.  
 

Comment 1 by baxley@chromium.org, Oct 31 2017

eugenebut@ I want to clarify one thing...

Part of this method was to push the assertion to the caller. Are we still happy with this pattern? This gives the caller the flexibility of what to do on failure (retry, assert, crash, ignore ...)

I think your proposed changes has no effect on this. The caller must do something with the result, which I think makes sense. I just wanted to clarify that we're okay with it returning false not necessarily being a failure.
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.
Project Member

Comment 3 by bugdroid1@chromium.org, 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

Comment 4 by sczs@chromium.org, Nov 1 2017

Owner: eugene...@chromium.org
Status: Assigned (was: Untriaged)
eugenebut@ can we close this?
Owner: ----
Status: Untriaged (was: Assigned)
We can not close this bug. Most of test functions dont have WARN_UNUSED_RESULT
Project Member

Comment 6 by bugdroid1@chromium.org, 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

Project Member

Comment 7 by bugdroid1@chromium.org, 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

Project Member

Comment 8 by bugdroid1@chromium.org, 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

Project Member

Comment 9 by bugdroid1@chromium.org, 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

Owner: eugene...@chromium.org
Status: Assigned (was: Untriaged)
Cc: -baxley@chromium.org
Owner: baxley@chromium.org
Mike (as maintainer of test framework) to triage.
Project Member

Comment 12 by bugdroid1@chromium.org, 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

Cc: huangml@chromium.org liaoyuke@chromium.org
Status: Available (was: Assigned)
Owner: ----
Blocking: 814709

Sign in to add a comment