Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2952343002:
EG Tests for Sad Tab View (Closed)
|
Created:
3 years, 6 months ago by PL Modified:
3 years, 5 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, baxley+watch_chromium.org, noyau+watch_chromium.org, marq+watch_chromium.org, huangml+watch_chromium.org, liaoyuke+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionEG Tests for Sad Tab View
BUG=736162
TEST=None, this is a test.
Review-Url: https://codereview.chromium.org/2952343002
Cr-Commit-Position: refs/heads/master@{#482846}
Committed: https://chromium.googlesource.com/chromium/src/+/ccdc193a45479001424ed276a0a384ce7d7cf26e
Patch Set 1 #
Total comments: 14
Patch Set 2 : Review feedback #
Total comments: 4
Patch Set 3 : Typo #
Messages
Total messages: 18 (9 generated)
peterlaurens@chromium.org changed reviewers: + baxley@chromium.org, kkhorimoto@chromium.org
Hello! This CL adds EG tests to the new Sad Tab view. Now that the chrome://crash URL is supported reliable EG tests for Sad Tab are possible on iOS. kkhorimoto@ for Sad Tab changes baxley@ for EG Test changes (I appreciate you're out until Monday, but this can wait until then!) Thanks! - Peter
lgtm with some formatting suggestions + comment clarification https://codereview.chromium.org/2952343002/diff/1/ios/chrome/browser/ui/sad_t... File ios/chrome/browser/ui/sad_tab/sad_tab_view_egtest.mm (right): https://codereview.chromium.org/2952343002/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view_egtest.mm:33: // N.B. There is a mechanism which changes the Sad Tab UI if a crash URL is What does N.B. mean? https://codereview.chromium.org/2952343002/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view_egtest.mm:39: NSString* reloadSadTabTitleString = optional: You'd save a line by inlining the l10n_util call when instantiating the matcher instead of creating this variable that's only used once. Same with other strings below.
https://codereview.chromium.org/2952343002/diff/1/ios/chrome/browser/ui/sad_t... File ios/chrome/browser/ui/sad_tab/sad_tab_view_egtest.mm (right): https://codereview.chromium.org/2952343002/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view_egtest.mm:33: // N.B. There is a mechanism which changes the Sad Tab UI if a crash URL is On 2017/06/23 21:54:46, kkhorimoto_ wrote: > What does N.B. mean? N.B. = Note Bene = Take careful note of the following. I can remove this if it's obtuse. I wanted to call out that ideally I would want to split up this test into smaller atomic bits, but there is a fundamental reason why we need to go through the flow in one go. https://codereview.chromium.org/2952343002/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view_egtest.mm:39: NSString* reloadSadTabTitleString = On 2017/06/23 21:54:46, kkhorimoto_ wrote: > optional: You'd save a line by inlining the l10n_util call when instantiating > the matcher instead of creating this variable that's only used once. Same with > other strings below. Yeah, I think you're right. Originally I thought this would be cleaner, but my variable names are quite long so the amount of space saved ion the 'matcher' lines is low. I'll make this change, thanks!
Just a few questions and nits. My biggest question is if we need to add ContainsText as a helper for other tests. Since we've had 400+ tests that don't need it (unless they already have implemented it locally, in which case your change makes sense). https://codereview.chromium.org/2952343002/diff/1/ios/chrome/browser/ui/sad_t... File ios/chrome/browser/ui/sad_tab/sad_tab_view_egtest.mm (right): https://codereview.chromium.org/2952343002/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view_egtest.mm:36: // as visiting Sad Tab may not be idempotent. Great comment! it cleared up my question of why this isn't split up into smaller tests. https://codereview.chromium.org/2952343002/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view_egtest.mm:52: chrome_test_util::ContainsText(incognitoHelpString); Optional nit: If you want to make the test shorter, you can move the matchers and strings into the anonymous namespace in this file. You can also remove "Matcher" from the name, since it's not typically used in our C-style matcher methods. I know it's only one test that uses this, but I know some test authors have done it in the past. I don't feel super strongly, up to you. https://codereview.chromium.org/2952343002/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view_egtest.mm:72: // Use chome_test_util::LoadURL() directly to avoid ChomeEarlGray helper nit: s/LoadURL/LoadUrl s/ChromeEarlGray/ChromeEarlGrey https://codereview.chromium.org/2952343002/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view_egtest.mm:73: // methods which expect to wait for web content. Another great comment! answering my question about LoadURL() https://codereview.chromium.org/2952343002/diff/1/ios/chrome/test/earl_grey/c... File ios/chrome/test/earl_grey/chrome_matchers.h (right): https://codereview.chromium.org/2952343002/diff/1/ios/chrome/test/earl_grey/c... ios/chrome/test/earl_grey/chrome_matchers.h:130: // objects. The text comparison is performed with NSString's containsText: Is it worthwhile exposing the implementation detail of the method used? Or would it be better to just say that it looks for a substring of text in UILabel, ... ? https://codereview.chromium.org/2952343002/diff/1/ios/chrome/test/earl_grey/c... ios/chrome/test/earl_grey/chrome_matchers.h:132: id<GREYMatcher> ContainsText(NSString* text); Can you think of many other use cases where this is needed over matcherForText in GREYMatchers.h? I think that one is preferable, when it works. I'd rather not have two slightly different helper methods in different locations, making it more difficult to know which to use, unless the need for ContainsText expands beyond the test you added. There may already be examples doing something similar within tests, in which case this method is a good idea.
Thanks for your guidance! baxley@ - thanks for your suggestions, new patch uploaded, PTAL! - Peter https://codereview.chromium.org/2952343002/diff/1/ios/chrome/browser/ui/sad_t... File ios/chrome/browser/ui/sad_tab/sad_tab_view_egtest.mm (right): https://codereview.chromium.org/2952343002/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view_egtest.mm:52: chrome_test_util::ContainsText(incognitoHelpString); On 2017/06/26 08:25:36, baxley wrote: > Optional nit: If you want to make the test shorter, you can move the matchers > and strings into the anonymous namespace in this file. You can also remove > "Matcher" from the name, since it's not typically used in our C-style matcher > methods. > > I know it's only one test that uses this, but I know some test authors have done > it in the past. I don't feel super strongly, up to you. That's interesting, I've put up a change that does this in the new patch, let me know what you think! https://codereview.chromium.org/2952343002/diff/1/ios/chrome/browser/ui/sad_t... ios/chrome/browser/ui/sad_tab/sad_tab_view_egtest.mm:72: // Use chome_test_util::LoadURL() directly to avoid ChomeEarlGray helper On 2017/06/26 08:25:37, baxley wrote: > nit: s/LoadURL/LoadUrl > s/ChromeEarlGray/ChromeEarlGrey Good catch, done! Thanks! https://codereview.chromium.org/2952343002/diff/1/ios/chrome/test/earl_grey/c... File ios/chrome/test/earl_grey/chrome_matchers.h (right): https://codereview.chromium.org/2952343002/diff/1/ios/chrome/test/earl_grey/c... ios/chrome/test/earl_grey/chrome_matchers.h:130: // objects. The text comparison is performed with NSString's containsText: On 2017/06/26 08:25:37, baxley wrote: > Is it worthwhile exposing the implementation detail of the method used? Or would > it be better to just say that it looks for a substring of text in UILabel, ... ? Done! https://codereview.chromium.org/2952343002/diff/1/ios/chrome/test/earl_grey/c... ios/chrome/test/earl_grey/chrome_matchers.h:132: id<GREYMatcher> ContainsText(NSString* text); On 2017/06/26 08:25:37, baxley wrote: > Can you think of many other use cases where this is needed over matcherForText > in GREYMatchers.h? I think that one is preferable, when it works. > > I'd rather not have two slightly different helper methods in different > locations, making it more difficult to know which to use, unless the need for > ContainsText expands beyond the test you added. There may already be examples > doing something similar within tests, in which case this method is a good idea. I was a bit surprised this was the first time this would be needed, I had a look through to see if there were similar tests using something like this but in a brief look through - I didn't see any but there are a lot I did not check. I'm happy to move this into the test class so it isn't presented to everyone, but I can imagine it being useful in the future. Happy to take your guidance and move it into the test class, I've done that in the new patch so you can see how it might look :-).
question about dispatch_once, and a nit. otherwise, LGTM https://codereview.chromium.org/2952343002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/sad_tab/sad_tab_view_egtest.mm (right): https://codereview.chromium.org/2952343002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view_egtest.mm:53: return matcher; Question more than comment, why is dispatch_once needed here? In our other matchers, we typically just return the matcher. For example... return grey_text(l10n_util::GetNSString(IDS_SAD_TAB_MESSAGE); I'm asking in case there is a bug we've missed, or I'm missing how this is different from other matchers. https://codereview.chromium.org/2952343002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view_egtest.mm:108: // Use chome_test_util::LoadURL() directly to avoid ChomeEarlGrey helper sorry for the super-nit... is it chrome_test_util::LoadUrl()
Thanks! https://codereview.chromium.org/2952343002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/sad_tab/sad_tab_view_egtest.mm (right): https://codereview.chromium.org/2952343002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view_egtest.mm:53: return matcher; On 2017/06/27 06:02:11, baxley wrote: > Question more than comment, why is dispatch_once needed here? In our other > matchers, we typically just return the matcher. > > For example... > return grey_text(l10n_util::GetNSString(IDS_SAD_TAB_MESSAGE); > > I'm asking in case there is a bug we've missed, or I'm missing how this is > different from other matchers. Good question! If you look at the GREYMatchers implementation, the convenience methods that return matchers (e.g. matcherForText:) create and allocate a new matcher every call. That's unnecessary, so here a dispatch_once ensures we re-use the matcher for the multiple times it's needed. dispatch_once is very quick and thread-safe (even though those things aren't super high priorities in the case of an egtest). https://codereview.chromium.org/2952343002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/sad_tab/sad_tab_view_egtest.mm:108: // Use chome_test_util::LoadURL() directly to avoid ChomeEarlGrey helper On 2017/06/27 06:02:11, baxley wrote: > sorry for the super-nit... > is it chrome_test_util::LoadUrl() Yes! I love the super-nit don't worry, helps me look like I can spell :-).
The CQ bit was checked by peterlaurens@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 peterlaurens@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kkhorimoto@chromium.org, baxley@chromium.org Link to the patchset: https://codereview.chromium.org/2952343002/#ps40001 (title: "Typo")
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": 40001, "attempt_start_ts": 1498614019482920,
"parent_rev": "475ce8ef500505927a1458c6ec1a4971eccdb691", "commit_rev":
"ccdc193a45479001424ed276a0a384ce7d7cf26e"}
Message was sent while issue was closed.
Description was changed from ========== EG Tests for Sad Tab View BUG=736162 TEST=None, this is a test. ========== to ========== EG Tests for Sad Tab View BUG=736162 TEST=None, this is a test. Review-Url: https://codereview.chromium.org/2952343002 Cr-Commit-Position: refs/heads/master@{#482846} Committed: https://chromium.googlesource.com/chromium/src/+/ccdc193a45479001424ed276a0a3... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/ccdc193a45479001424ed276a0a3... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
