Refactor Assert[No]CookieExists to return a boolean

ios/chrome/browser/ui/settings/settings_egtest.mm

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import <EarlGrey/EarlGrey.h>
 #import <UIKit/UIKit.h>
 #import <XCTest/XCTest.h>
 #include <map>
 
 #include "base/bind.h"
 #import "base/mac/bind_objc_block.h"
+#include "base/mac/foundation_util.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/sys_string_conversions.h"
 #include "components/browsing_data/core/pref_names.h"
 #include "components/metrics/metrics_pref_names.h"
 #include "components/password_manager/core/common/password_manager_pref_names.h"
 #include "components/prefs/pref_member.h"
 #include "components/prefs/pref_service.h"
 #include "components/strings/grit/components_strings.h"
 #import "ios/chrome/app/main_controller.h"
 #include "ios/chrome/browser/browser_state/chrome_browser_state.h"
@@ skipping 32 matching lines @@
 using chrome_test_util::NavigationBarDoneButton;
 
 namespace {
 
 const char kTestOrigin1[] = "http://host1:1/";
 
 const char kUrl[] = "http://foo/browsing";
 const char kUrlWithSetCookie[] = "http://foo/set_cookie";
 const char kResponse[] = "bar";
 const char kResponseWithSetCookie[] = "bar with set cookie";
-const char kNoCookieText[] = "No cookies";
-const char kCookie[] = "a=b";
-const char kJavaScriptGetCookies[] =
-    "var c = document.cookie ? document.cookie.split(/;\\s*/) : [];"
-    "if (!c.length) {"
-    "  document.documentElement.innerHTML = 'No cookies!';"
-    "} else {"
-    "  document.documentElement.innerHTML = 'OK: ' + c.join(',');"
-    "}";
+NSString* const kCookieKey = @"a";
+NSString* const kCookieValue = @"b";

[baxley, 2017/03/13 21:29:04]

nit: would this be better for debugging if it had more descriptive names?
@"key", @"value" ? @"key1", @"value2"?

[liaoyuke, 2017/03/13 22:50:40]

Done.

 
 enum MetricsServiceType {
   kMetrics,
   kBreakpad,
   kBreakpadFirstLaunch,
 };
 
 // Matcher for the clear browsing history cell on the clear browsing data panel.
 id<GREYMatcher> ClearBrowsingHistoryButton() {
   return grey_allOf(grey_accessibilityID(kClearBrowsingHistoryCellId),
@@ skipping 74 matching lines @@
 }
 // Matcher for the Privacy Translate Settings button on the privacy UI.
 id<GREYMatcher> TranslateSettingsButton() {
   return ButtonWithAccessibilityLabelId(IDS_IOS_TRANSLATE_SETTING);
 }
 // Matcher for the save button in the save password bar.
 id<GREYMatcher> savePasswordButton() {
   return ButtonWithAccessibilityLabelId(IDS_IOS_PASSWORD_MANAGER_SAVE_BUTTON);
 }
 
-// Asserts that there is no cookie in current web state.
-void AssertNoCookieExists() {
-  NSError* error = nil;
-  chrome_test_util::ExecuteJavaScript(
-      base::SysUTF8ToNSString(kJavaScriptGetCookies), &error);
-  [[EarlGrey selectElementWithMatcher:chrome_test_util::WebViewContainingText(
-                                          kNoCookieText)]
-      assertWithMatcher:grey_notNil()];
-}
-
-// Asserts |cookie| exists in current web state.
-void AssertCookieExists(const char cookie[]) {
-  NSError* error = nil;
-  chrome_test_util::ExecuteJavaScript(
-      base::SysUTF8ToNSString(kJavaScriptGetCookies), &error);
-  NSString* const expectedCookieText =
-      [NSString stringWithFormat:@"OK: %@", base::SysUTF8ToNSString(cookie)];
-  [[EarlGrey
-      selectElementWithMatcher:chrome_test_util::WebViewContainingText(
-                                   base::SysNSStringToUTF8(expectedCookieText))]
-      assertWithMatcher:grey_notNil()];
-}
-
 // Run as a task to check if a certificate has been added to the ChannelIDStore.
 // Signals the given |semaphore| if the cert was added, or reposts itself
 // otherwise.
 void CheckCertificate(scoped_refptr<net::URLRequestContextGetter> getter,
                       dispatch_semaphore_t semaphore) {
   net::ChannelIDService* channel_id_service =
       getter->GetURLRequestContext()->channel_id_service();
   if (channel_id_service->channel_id_count() == 0) {
     // If the channel_id_count is still 0, no certs have been added yet.
     // Re-post this task and check again later.
@@ skipping 458 matching lines @@
   // kMetricsReportingEnabled ON and kMetricsReportingWifiOnly OFF
   chrome_test_util::SetBooleanLocalStatePref(
       metrics::prefs::kMetricsReportingEnabled, YES);
   chrome_test_util::SetBooleanLocalStatePref(prefs::kMetricsReportingWifiOnly,
                                              NO);
   // Service should remain disabled.
   [self assertMetricsServiceDisabled:serviceType];
 #endif
 }
 
+// Creates a map of canned responses and set up the test HTML server, which
+// always sets cookie in response header.
+- (void)setUpSimpleHttpServerWithSetCookies {

[baxley, 2017/03/13 21:29:04]

Does only one method use this? Do you think it makes it more readable to
abstract it? It does make the method shorter, but it requires the user to jump
here to see what cookies are being set up, which will be asserted within the
method.

[liaoyuke, 2017/03/13 22:50:40]

Makes sense. But ideally, I think we should separate the cookie test into a file
and have this method inside the setUp function.

+  // Creates a map of canned responses and set up the test HTML server.
+  std::map<GURL, std::pair<std::string, std::string>> response;
+
+  NSString* cookie =
+      [NSString stringWithFormat:@"%@=%@", kCookieKey, kCookieValue];
+
+  response[web::test::HttpServer::MakeUrl(kUrlWithSetCookie)] =
+      std::pair<std::string, std::string>(base::SysNSStringToUTF8(cookie),
+                                          kResponseWithSetCookie);
+  response[web::test::HttpServer::MakeUrl(kUrl)] =
+      std::pair<std::string, std::string>("", kResponse);
+
+  web::test::SetUpSimpleHttpServerWithSetCookies(response);
+}
+
 #pragma mark Tests
 
 // Tests that clearing the cookies through the UI does clear all of them. Use a
 // local server to navigate to a page that sets then tests a cookie, and then
 // clears the cookie and tests it is not set.
 // TODO(crbug.com/638674): Evaluate if this can move to shared code.
 - (void)testClearCookies {
-  // Creates a map of canned responses and set up the test HTML server.
-  std::map<GURL, std::pair<std::string, std::string>> response;
-
-  response[web::test::HttpServer::MakeUrl(kUrlWithSetCookie)] =
-      std::pair<std::string, std::string>(kCookie, kResponseWithSetCookie);
-  response[web::test::HttpServer::MakeUrl(kUrl)] =
-      std::pair<std::string, std::string>("", kResponse);
-
-  web::test::SetUpSimpleHttpServerWithSetCookies(response);
+  [self setUpSimpleHttpServerWithSetCookies];
 
   // Load |kUrl| and check that cookie is not set.
   [ChromeEarlGrey loadURL:web::test::HttpServer::MakeUrl(kUrl)];
   [[EarlGrey selectElementWithMatcher:chrome_test_util::WebViewContainingText(
                                           kResponse)]
       assertWithMatcher:grey_notNil()];
-  AssertNoCookieExists();
+
+  NSDictionary* cookies = [ChromeEarlGrey getCookies];
+  GREYAssertTrue(cookies, @"Failed to get cookies.");
+  GREYAssertEqual(0U, cookies.count, @"No cookie should be found.");

[baxley, 2017/03/13 21:29:04]

The two lines above are a little confusing to me. Do we need the case where we
check GREYAssertTrue(cookies, ...)

Under what condition would it be false (the code checks for True in cases where
there are and are not cookies).

[liaoyuke, 2017/03/13 22:50:40]

Acknowledged.

 
   // Visit |kUrlWithSetCookie| to set a cookie and then load |kUrl| to check it
   // is still set.
   [ChromeEarlGrey loadURL:web::test::HttpServer::MakeUrl(kUrlWithSetCookie)];
   [[EarlGrey selectElementWithMatcher:chrome_test_util::WebViewContainingText(
                                           kResponseWithSetCookie)]
       assertWithMatcher:grey_notNil()];
   [ChromeEarlGrey loadURL:web::test::HttpServer::MakeUrl(kUrl)];
   [[EarlGrey selectElementWithMatcher:chrome_test_util::WebViewContainingText(
                                           kResponse)]
       assertWithMatcher:grey_notNil()];
 
-  AssertCookieExists(kCookie);
+  cookies = [ChromeEarlGrey getCookies];
+  GREYAssertTrue(cookies, @"Failed to get cookies.");
+  GREYAssertTrue([kCookieValue isEqualToString:cookies[kCookieKey]],
+                 @"Failed to set cookie.");
+  GREYAssertEqual(1U, cookies.count, @"Only one cookie should be found.");
 
-  // Restore the Clear Browsing Data checkmarks prefs to their default state in
-  // Teardown.
+  // Restore the Clear Browsing Data checkmarks prefs to their default state
+  // in Teardown.
   [self setTearDownHandler:^{
     [self restoreClearBrowsingDataCheckmarksToDefault];
   }];
 
   // Clear all cookies.
   [self clearCookiesAndSiteData];
 
   // Reload and test that there are no cookies left.
   [ChromeEarlGrey loadURL:web::test::HttpServer::MakeUrl(kUrl)];
   [[EarlGrey selectElementWithMatcher:chrome_test_util::WebViewContainingText(
                                           kResponse)]
       assertWithMatcher:grey_notNil()];
-  AssertNoCookieExists();
+
+  cookies = [ChromeEarlGrey getCookies];
+  GREYAssertTrue(cookies, @"Failed to get cookies.");
+  GREYAssertEqual(0U, cookies.count, @"No cookie should be found.");
+
   chrome_test_util::CloseAllTabs();
 }
 
 // Verifies that logging into a form on a web page allows the user to save and
 // then clear a password.
 - (void)testClearPasswords {
 
   ios::ChromeBrowserState* browserState =
       chrome_test_util::GetOriginalBrowserState();
   PrefService* preferences = browserState->GetPrefs();
@@ skipping 325 matching lines @@
       performAction:grey_tap()];
   [[EarlGrey selectElementWithMatcher:PrivacyButton()]
       performAction:grey_tap()];
   [[EarlGrey selectElementWithMatcher:SendUsageDataButton()]
       performAction:grey_tap()];
   chrome_test_util::VerifyAccessibilityForCurrentScreen();
   [self closeSubSettingsMenu];
 }
 
 @end