Refactor Assert[No]CookieExists to return a boolean

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

Index: ios/chrome/browser/ui/settings/settings_egtest.mm
diff --git a/ios/chrome/browser/ui/settings/settings_egtest.mm b/ios/chrome/browser/ui/settings/settings_egtest.mm
index ac39ef9c01bbe4f8c16dd7a15a3ac8a82c6d3cfd..3d9042389e38434e1d3ebbb1bbb13595de2e3782 100644
--- a/ios/chrome/browser/ui/settings/settings_egtest.mm
+++ b/ios/chrome/browser/ui/settings/settings_egtest.mm
@@ -9,6 +9,7 @@
 
 #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"
@@ -61,15 +62,8 @@
 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,
@@ -164,29 +158,6 @@
   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.
@@ -665,29 +636,42 @@ - (void)assertsMetricsPrefsForService:(MetricsServiceType)serviceType {
 #endif
 }
 
-#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, 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>(kCookie, kResponseWithSetCookie);
+      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 {
+  [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.
@@ -700,10 +684,14 @@ - (void)testClearCookies {
                                           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];
   }];
@@ -716,7 +704,11 @@ - (void)testClearCookies {
   [[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();
 }