New issue
Advanced search Search tips

Issue 760100 link

Starred by 1 user
Status: Fixed
Owner:
sdefresne@chromium.org
Closed: Aug 2017
Cc:
ios-bugs@chromium.org
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 3
Type: Bug



Sign in to add a comment

EXPECT_EQ should not be used with NSString*

Project Member Reported by sdefresne@chromium.org, Aug 29 2017 
Strings may or may not be interned. EXPECT_EQ should not be used to compare them as it just compare the pointers and can fail flakily. Instead EXPECT_NSEQ should be used as it use -isEqual: which will compare the data.

Using EXPECT_EQ can lead to the following funny failures:

[ RUN      ] ChromeIconTest.TemplateBarButtonItem
../../ios/chrome/browser/ui/icons/chrome_icon_unittest.mm:61: Failure
      Expected: @"label"
      Which is: 1
To be equal to: barButtonItem.accessibilityLabel
      Which is: 1
[  FAILED  ] ChromeIconTest.TemplateBarButtonItem (1 ms)
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 29 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/29039e3b42bf63f99fabe03fdea188ad288684dc

commit 29039e3b42bf63f99fabe03fdea188ad288684dc
Author: Sylvain Defresne <sdefresne@chromium.org>
Date: Tue Aug 29 14:11:31 2017

Fix incorrect uses of EXPECT_EQ with NSString*.

EXPECT_EQ does compare using == which is incorrect for NSString*
as it only compare the pointers not the object. Instead use the
correct EXPECT_NSEQ that uses -isEqual:.

Fixes some tests flakyness like the following when the compiler
decide to not intern the strings.

  [ RUN      ] ChromeIconTest.TemplateBarButtonItem
  ../../ios/chrome/browser/ui/icons/chrome_icon_unittest.mm:61: Failure
        Expected: @"label"
        Which is: 1
  To be equal to: barButtonItem.accessibilityLabel
        Which is: 1
  [  FAILED  ] ChromeIconTest.TemplateBarButtonItem (1 ms)

Bug:  760100 
Change-Id: I76cfb50a1d04b851cc4db3d3ae0cf5f81a6edb5b
Reviewed-on: https://chromium-review.googlesource.com/641471
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Stepan Khapugin <stkhapugin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498101}
[modify] https://crrev.com/29039e3b42bf63f99fabe03fdea188ad288684dc/ios/chrome/app/application_delegate/url_opener_unittest.mm
[modify] https://crrev.com/29039e3b42bf63f99fabe03fdea188ad288684dc/ios/chrome/browser/ui/bookmarks/cells/bookmark_parent_folder_item_unittest.mm
[modify] https://crrev.com/29039e3b42bf63f99fabe03fdea188ad288684dc/ios/chrome/browser/ui/collection_view/cells/collection_view_item_unittest.mm
[modify] https://crrev.com/29039e3b42bf63f99fabe03fdea188ad288684dc/ios/chrome/browser/ui/icons/chrome_icon_unittest.mm
[modify] https://crrev.com/29039e3b42bf63f99fabe03fdea188ad288684dc/ios/chrome/browser/ui/ntp/most_visited_cell_unittest.mm
[modify] https://crrev.com/29039e3b42bf63f99fabe03fdea188ad288684dc/ios/chrome/browser/ui/settings/cells/version_item_unittest.mm

Comment 2 by sdefresne@chromium.org, Aug 29 2017

Status: Fixed (was: Assigned)
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 30 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/289639481045995fa69c3ed3dd5cb6ac03bacd85

commit 289639481045995fa69c3ed3dd5cb6ac03bacd85
Author: Sylvain Defresne <sdefresne@chromium.org>
Date: Wed Aug 30 07:39:59 2017

Fix incorrect uses of EXPECT_EQ with NSString*.

EXPECT_EQ does compare using == which is incorrect for NSString*
as it only compare the pointers not the object. Instead use the
correct EXPECT_NSEQ that uses -isEqual:.

Fixes uses of non-litteral NSString*.

Bug:  760100 
Change-Id: Ieddec072e29f8da944691c03bf82b670fb570735
Reviewed-on: https://chromium-review.googlesource.com/641710
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Stepan Khapugin <stkhapugin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498388}
[modify] https://crrev.com/289639481045995fa69c3ed3dd5cb6ac03bacd85/ios/chrome/app/application_delegate/url_opener_unittest.mm
►

Sign in to add a comment