Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2734963004:
Refactor Assert[No]CookieExists to return a boolean (Closed)
|
Created:
3 years, 9 months ago by liaoyuke Modified:
3 years, 9 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, marq+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor Assert[No]CookieExists to return a boolean
Instead of asserting inside helper functions, this CL refactors
Assert[NO]CookieExists functions to return a boolean value and have the
test asserts explicitly.
This not only makes the intention of the functions more clear but also
allows more flexible handling of the error, such as wrap it inside a
wait condition or customize error message etc.
BUG=693580
Review-Url: https://codereview.chromium.org/2734963004
Cr-Commit-Position: refs/heads/master@{#456868}
Committed: https://chromium.googlesource.com/chromium/src/+/e5dbb7a7fe2cf1b9086f1f7a88082e998705086f
Patch Set 1 : Refactor Assert[No]CookieExists to return a boolean #
Total comments: 4
Patch Set 2 : Addressed feedback #
Total comments: 10
Patch Set 3 : Move cookie helpers to a shared location #
Total comments: 10
Patch Set 4 : Addressed comments #Patch Set 5 : Update comments #
Total comments: 18
Patch Set 6 : Eugene's comments #
Total comments: 4
Patch Set 7 : Addressed comments #Patch Set 8 : Rebase #
Messages
Total messages: 30 (15 generated)
Patchset #1 (id:1) has been deleted
liaoyuke@chromium.org changed reviewers: + baxley@chromium.org
Hey Mike, Please take a look. Thank you very much!
I just have one regex question. I'm happy with the naming and structure of these new methods. https://codereview.chromium.org/2734963004/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/settings/settings_egtest.mm (right): https://codereview.chromium.org/2734963004/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/settings_egtest.mm:180: // Returns true if |cookie| is the only cookie exists in current web state. will this return true if it's the "only" cookie? Is there a way to have multiple cookies and it return true? https://codereview.chromium.org/2734963004/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/settings_egtest.mm:191: error:&error]; Sorry, I'm not great at reading regex. Could you clarify why this would return false for the case where there are two cookies, and we're checking for the first one?
Patchset #2 (id:40001) has been deleted
PTAL. Thanks! https://codereview.chromium.org/2734963004/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/settings/settings_egtest.mm (right): https://codereview.chromium.org/2734963004/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/settings_egtest.mm:180: // Returns true if |cookie| is the only cookie exists in current web state. On 2017/03/08 16:20:50, baxley wrote: > will this return true if it's the "only" cookie? Is there a way to have multiple > cookies and it return true? Resolved per offline discussion. https://codereview.chromium.org/2734963004/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/settings_egtest.mm:191: error:&error]; On 2017/03/08 16:20:50, baxley wrote: > Sorry, I'm not great at reading regex. Could you clarify why this would return > false for the case where there are two cookies, and we're checking for the first > one? Resolved per offline discussion.
PTAL. Thanks!
A few naming and comment nits, but overall seems fine. Once addressed, LGTM (you'll need an OWNERS review) https://codereview.chromium.org/2734963004/diff/60001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/settings/settings_egtest.mm (right): https://codereview.chromium.org/2734963004/diff/60001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/settings_egtest.mm:168: // Returns true if there is no cookie in current web state. nit: s/current/the current https://codereview.chromium.org/2734963004/diff/60001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/settings_egtest.mm:169: bool IsCookieEmpty() { Would IsCookiePresent() be more clear? We're not checking for an empty "cookie", are we checking for there being no cookies? https://codereview.chromium.org/2734963004/diff/60001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/settings_egtest.mm:183: // Returns true if |cookie| is the only cookie exists in current web state. Returns true if |cookie| is the only cookie that exists in the current web state. i.e. s/cookie exists/cookie that exists/ and s/current/the current https://codereview.chromium.org/2734963004/diff/60001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/settings_egtest.mm:184: bool IsCookieEquals(const char cookie[]) { Sorry, I don't have great suggestions, but IsCookieEquals doesn't make it clear what two cookies we're comparing. IsCookiePresent(cookie)? IsOneCookiePresent(cookie)? https://codereview.chromium.org/2734963004/diff/60001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/settings_egtest.mm:196: return [resultCookie isEqualToString:expectedCookie]; Do you see any value in having a helper method to do the comparison, since it looks like the above 11 lines of code are the same (with the exception of building the expectedCookie here). Just a thought, I leave it up to you, I don't want to over engineer this or drag it out longer than necessary.
Patchset #3 (id:80001) has been deleted
PTAL. Thank you very much! https://codereview.chromium.org/2734963004/diff/60001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/settings/settings_egtest.mm (right): https://codereview.chromium.org/2734963004/diff/60001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/settings_egtest.mm:168: // Returns true if there is no cookie in current web state. On 2017/03/10 21:58:17, baxley wrote: > nit: s/current/the current Done. https://codereview.chromium.org/2734963004/diff/60001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/settings_egtest.mm:169: bool IsCookieEmpty() { On 2017/03/10 21:58:17, baxley wrote: > Would IsCookiePresent() be more clear? We're not checking for an empty "cookie", > are we checking for there being no cookies? Acknowledged. https://codereview.chromium.org/2734963004/diff/60001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/settings_egtest.mm:183: // Returns true if |cookie| is the only cookie exists in current web state. On 2017/03/10 21:58:17, baxley wrote: > Returns true if |cookie| is the only cookie that exists in the current web > state. > > i.e. s/cookie exists/cookie that exists/ and s/current/the current Acknowledged. https://codereview.chromium.org/2734963004/diff/60001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/settings_egtest.mm:184: bool IsCookieEquals(const char cookie[]) { On 2017/03/10 21:58:17, baxley wrote: > Sorry, I don't have great suggestions, but IsCookieEquals doesn't make it clear > what two cookies we're comparing. > > IsCookiePresent(cookie)? IsOneCookiePresent(cookie)? Acknowledged. https://codereview.chromium.org/2734963004/diff/60001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/settings_egtest.mm:196: return [resultCookie isEqualToString:expectedCookie]; On 2017/03/10 21:58:17, baxley wrote: > Do you see any value in having a helper method to do the comparison, since it > looks like the above 11 lines of code are the same (with the exception of > building the expectedCookie here). Just a thought, I leave it up to you, I don't > want to over engineer this or drag it out longer than necessary. Done.
I think the overall approach is good. I have a few questions about the number of assertions we do per check. Would it be possible to just do one check (verify cookies.count)? https://codereview.chromium.org/2734963004/diff/100001/ios/chrome/browser/net... File ios/chrome/browser/net/cookies_egtest.mm (right): https://codereview.chromium.org/2734963004/diff/100001/ios/chrome/browser/net... ios/chrome/browser/net/cookies_egtest.mm:108: @"Only one cookie should be found in normal mode."); Do we need to check all three things for every case? The purpose of this test is to verify if a cookie exists, would it be okay to just do the last two checks? For some of the cases with no cookies, could we just do one check (cookie.count == 0)? https://codereview.chromium.org/2734963004/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/settings_egtest.mm (right): https://codereview.chromium.org/2734963004/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:66: NSString* const kCookieValue = @"b"; nit: would this be better for debugging if it had more descriptive names? @"key", @"value" ? @"key1", @"value2"? https://codereview.chromium.org/2734963004/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:641: - (void)setUpSimpleHttpServerWithSetCookies { 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. https://codereview.chromium.org/2734963004/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:674: GREYAssertEqual(0U, cookies.count, @"No cookie should be found."); 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). https://codereview.chromium.org/2734963004/diff/100001/ios/chrome/test/earl_g... File ios/chrome/test/earl_grey/chrome_earl_grey.mm (right): https://codereview.chromium.org/2734963004/diff/100001/ios/chrome/test/earl_g... ios/chrome/test/earl_grey/chrome_earl_grey.mm:98: return cookies; Sorry if I missed it, but the comment says it returns nil if it fails to get cookies. Doesn't it return something like @{} ?
liaoyuke@chromium.org changed reviewers: + eugenebut@chromium.org
Hey Eugene, Please take a look for owner's approval. Thanks, Yuke https://codereview.chromium.org/2734963004/diff/100001/ios/chrome/browser/net... File ios/chrome/browser/net/cookies_egtest.mm (right): https://codereview.chromium.org/2734963004/diff/100001/ios/chrome/browser/net... ios/chrome/browser/net/cookies_egtest.mm:108: @"Only one cookie should be found in normal mode."); On 2017/03/13 21:29:04, baxley wrote: > Do we need to check all three things for every case? The purpose of this test is > to verify if a cookie exists, would it be okay to just do the last two checks? > > For some of the cases with no cookies, could we just do one check (cookie.count > == 0)? Done. https://codereview.chromium.org/2734963004/diff/100001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/settings_egtest.mm (right): https://codereview.chromium.org/2734963004/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:66: NSString* const kCookieValue = @"b"; On 2017/03/13 21:29:04, baxley wrote: > nit: would this be better for debugging if it had more descriptive names? > @"key", @"value" ? @"key1", @"value2"? Done. https://codereview.chromium.org/2734963004/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:641: - (void)setUpSimpleHttpServerWithSetCookies { On 2017/03/13 21:29:04, baxley wrote: > 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. Makes sense. But ideally, I think we should separate the cookie test into a file and have this method inside the setUp function. https://codereview.chromium.org/2734963004/diff/100001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:674: GREYAssertEqual(0U, cookies.count, @"No cookie should be found."); On 2017/03/13 21:29:04, baxley wrote: > 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). Acknowledged. https://codereview.chromium.org/2734963004/diff/100001/ios/chrome/test/earl_g... File ios/chrome/test/earl_grey/chrome_earl_grey.mm (right): https://codereview.chromium.org/2734963004/diff/100001/ios/chrome/test/earl_g... ios/chrome/test/earl_grey/chrome_earl_grey.mm:98: return cookies; On 2017/03/13 21:29:04, baxley wrote: > Sorry if I missed it, but the comment says it returns nil if it fails to get > cookies. Doesn't it return something like @{} ? Done.
https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/settings_egtest.mm (right): https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:65: NSString* const kCookieKey = @"key"; s/kCookieKey/kCookieName and s/key/name https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:649: NSString* cookieForUrl = cookieForURL https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/test/earl_g... File ios/chrome/test/earl_grey/chrome_earl_grey.h (right): https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/test/earl_g... ios/chrome/test/earl_grey/chrome_earl_grey.h:39: // Returns cookies as key value pairs, and it fails the test if there are errors How about "Returns cookies as key value pairs, where key is a cookie name and value is a cookie value."? https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/test/earl_g... File ios/chrome/test/earl_grey/chrome_earl_grey.mm (right): https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/test/earl_g... ios/chrome/test/earl_grey/chrome_earl_grey.mm:77: " document.documentElement.innerHTML = 'No cookies!';" It is quite unexpected side effect that |cookies| method changes the page. https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/test/earl_g... ios/chrome/test/earl_grey/chrome_earl_grey.mm:91: NSArray* stringCookies = base::mac::ObjCCastStrict<NSArray>(result); nit: s/stringCookie/nameValuePairs ? https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/test/earl_g... ios/chrome/test/earl_grey/chrome_earl_grey.mm:93: for (NSString* stringCookie in stringCookies) { nit: s/stringCookie/nameValuePair ? https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/test/earl_g... ios/chrome/test/earl_grey/chrome_earl_grey.mm:94: NSArray* cookieKeyValue = [stringCookie componentsSeparatedByString:@"="]; s/cookieKeyValue/cookieNameValue https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/test/earl_g... ios/chrome/test/earl_grey/chrome_earl_grey.mm:95: NSString* cookieKey = cookieKeyValue[0]; s/cookieKey/cookieName https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/test/earl_g... ios/chrome/test/earl_grey/chrome_earl_grey.mm:95: NSString* cookieKey = cookieKeyValue[0]; Do you want to assert that array length is 2? Otherwise this will crash.
PTAL. Thank you very much! https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/settings/settings_egtest.mm (right): https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:65: NSString* const kCookieKey = @"key"; On 2017/03/14 00:43:38, Eugene But wrote: > s/kCookieKey/kCookieName and s/key/name Done. https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/settings/settings_egtest.mm:649: NSString* cookieForUrl = On 2017/03/14 00:43:38, Eugene But wrote: > cookieForURL Done. https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/test/earl_g... File ios/chrome/test/earl_grey/chrome_earl_grey.h (right): https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/test/earl_g... ios/chrome/test/earl_grey/chrome_earl_grey.h:39: // Returns cookies as key value pairs, and it fails the test if there are errors On 2017/03/14 00:43:38, Eugene But wrote: > How about "Returns cookies as key value pairs, where key is a cookie name and > value is a cookie value."? Done. https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/test/earl_g... File ios/chrome/test/earl_grey/chrome_earl_grey.mm (right): https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/test/earl_g... ios/chrome/test/earl_grey/chrome_earl_grey.mm:77: " document.documentElement.innerHTML = 'No cookies!';" On 2017/03/14 00:43:39, Eugene But wrote: > It is quite unexpected side effect that |cookies| method changes the page. I talked to baxley@ about this issue, and we thought it's fine because the text can be used for better visualization and debugging. But I think you are right, the side effect is surprising, and we should avoid doing this. https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/test/earl_g... ios/chrome/test/earl_grey/chrome_earl_grey.mm:91: NSArray* stringCookies = base::mac::ObjCCastStrict<NSArray>(result); On 2017/03/14 00:43:39, Eugene But wrote: > nit: s/stringCookie/nameValuePairs ? Done. https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/test/earl_g... ios/chrome/test/earl_grey/chrome_earl_grey.mm:93: for (NSString* stringCookie in stringCookies) { On 2017/03/14 00:43:39, Eugene But wrote: > nit: s/stringCookie/nameValuePair ? Done. https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/test/earl_g... ios/chrome/test/earl_grey/chrome_earl_grey.mm:94: NSArray* cookieKeyValue = [stringCookie componentsSeparatedByString:@"="]; On 2017/03/14 00:43:39, Eugene But wrote: > s/cookieKeyValue/cookieNameValue Done. https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/test/earl_g... ios/chrome/test/earl_grey/chrome_earl_grey.mm:95: NSString* cookieKey = cookieKeyValue[0]; On 2017/03/14 00:43:39, Eugene But wrote: > s/cookieKey/cookieName Done. https://codereview.chromium.org/2734963004/diff/140001/ios/chrome/test/earl_g... ios/chrome/test/earl_grey/chrome_earl_grey.mm:95: NSString* cookieKey = cookieKeyValue[0]; On 2017/03/14 00:43:39, Eugene But wrote: > Do you want to assert that array length is 2? Otherwise this will crash. Done.
lgtm with changes to the script https://codereview.chromium.org/2734963004/diff/160001/ios/chrome/test/earl_g... File ios/chrome/test/earl_grey/chrome_earl_grey.mm (right): https://codereview.chromium.org/2734963004/diff/160001/ios/chrome/test/earl_g... ios/chrome/test/earl_grey/chrome_earl_grey.mm:74: @"(function getCookies() {" @"document.cookie ? document.cookie.split(/;\\s*/) : [];" https://codereview.chromium.org/2734963004/diff/160001/ios/chrome/test/earl_g... ios/chrome/test/earl_grey/chrome_earl_grey.mm:90: GREYAssertEqual(2, cookieNameValue.count, @"Cookie has invalid format."); s/2/2U ?
The CQ bit was checked by liaoyuke@chromium.org to run a CQ dry run
https://codereview.chromium.org/2734963004/diff/160001/ios/chrome/test/earl_g... File ios/chrome/test/earl_grey/chrome_earl_grey.mm (right): https://codereview.chromium.org/2734963004/diff/160001/ios/chrome/test/earl_g... ios/chrome/test/earl_grey/chrome_earl_grey.mm:74: @"(function getCookies() {" On 2017/03/14 17:32:02, Eugene But wrote: > @"document.cookie ? document.cookie.split(/;\\s*/) : [];" Done. https://codereview.chromium.org/2734963004/diff/160001/ios/chrome/test/earl_g... ios/chrome/test/earl_grey/chrome_earl_grey.mm:90: GREYAssertEqual(2, cookieNameValue.count, @"Cookie has invalid format."); On 2017/03/14 17:32:02, Eugene But wrote: > s/2/2U ? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you very much for reviewing!
The CQ bit was checked by liaoyuke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by liaoyuke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from baxley@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2734963004/#ps200001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1489530646935250,
"parent_rev": "939b97d2936323c01181ddabe38a4e3780da242c", "commit_rev":
"e5dbb7a7fe2cf1b9086f1f7a88082e998705086f"}
Message was sent while issue was closed.
Description was changed from ========== Refactor Assert[No]CookieExists to return a boolean Instead of asserting inside helper functions, this CL refactors Assert[NO]CookieExists functions to return a boolean value and have the test asserts explicitly. This not only makes the intention of the functions more clear but also allows more flexible handling of the error, such as wrap it inside a wait condition or customize error message etc. BUG=693580 ========== to ========== Refactor Assert[No]CookieExists to return a boolean Instead of asserting inside helper functions, this CL refactors Assert[NO]CookieExists functions to return a boolean value and have the test asserts explicitly. This not only makes the intention of the functions more clear but also allows more flexible handling of the error, such as wrap it inside a wait condition or customize error message etc. BUG=693580 Review-Url: https://codereview.chromium.org/2734963004 Cr-Commit-Position: refs/heads/master@{#456868} Committed: https://chromium.googlesource.com/chromium/src/+/e5dbb7a7fe2cf1b9086f1f7a8808... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:200001) as https://chromium.googlesource.com/chromium/src/+/e5dbb7a7fe2cf1b9086f1f7a8808... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
