Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2170243002:
[ios] Implemented Meta Tags EarlGrey tests. (Closed)
|
Created:
4 years, 5 months ago by Eugene But (OOO till 7-30) Modified:
4 years, 5 months ago Reviewers:
baxley CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@rename-tests Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ios] Implemented Meta Tags EarlGrey tests.
BUG=630411
Committed: https://crrev.com/2b43f1201efe386b7c28c536184b2ec472282f8c
Cr-Commit-Position: refs/heads/master@{#407328}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed review comments #
Total comments: 2
Patch Set 3 : Merged #Patch Set 4 : Added missing dependency. #
Messages
Total messages: 21 (9 generated)
eugenebut@chromium.org changed reviewers: + baxley@chromium.org
https://codereview.chromium.org/2170243002/diff/1/ios/web/ios_web_shell_tests... File ios/web/ios_web_shell_tests.gyp (right): https://codereview.chromium.org/2170243002/diff/1/ios/web/ios_web_shell_tests... ios/web/ios_web_shell_tests.gyp:64: 'shell/test/meta_tags_egtest.mm', Can you add this to ios/web/shell/test/BUILD.gn (this just landed). I'll take care of doing it for page_state_egtest.mm since that can happen now https://codereview.chromium.org/2170243002/diff/1/ios/web/shell/test/meta_tag... File ios/web/shell/test/meta_tags_egtest.mm (right): https://codereview.chromium.org/2170243002/diff/1/ios/web/shell/test/meta_tag... ios/web/shell/test/meta_tags_egtest.mm:40: @interface MetaTagsTest : ShellBaseTestCase Similar to comment in https://codereview.chromium.org/2173543002/ Should this be ShellMetaTagsTest? Or do you think it's better to drop the shell. Most of the files in ios/web/shell start with shell. https://codereview.chromium.org/2170243002/diff/1/ios/web/shell/test/meta_tag... ios/web/shell/test/meta_tags_egtest.mm:72: assertWithMatcher:grey_notNil()]; This has a 4 second timeout. In the case where the delay is 3 seconds, do you think that this is cutting it close? If I'm misunderstanding the meta refresh time, let me know.
gclient sync -j1 pulling source from dependent CL for some reason :( https://codereview.chromium.org/2170243002/diff/1/ios/web/ios_web_shell_tests... File ios/web/ios_web_shell_tests.gyp (right): https://codereview.chromium.org/2170243002/diff/1/ios/web/ios_web_shell_tests... ios/web/ios_web_shell_tests.gyp:64: 'shell/test/meta_tags_egtest.mm', On 2016/07/22 15:53:26, baxley wrote: > Can you add this to ios/web/shell/test/BUILD.gn (this just landed). > > I'll take care of doing it for page_state_egtest.mm since that can happen now Done. https://codereview.chromium.org/2170243002/diff/1/ios/web/shell/test/meta_tag... File ios/web/shell/test/meta_tags_egtest.mm (right): https://codereview.chromium.org/2170243002/diff/1/ios/web/shell/test/meta_tag... ios/web/shell/test/meta_tags_egtest.mm:40: @interface MetaTagsTest : ShellBaseTestCase On 2016/07/22 15:53:26, baxley wrote: > Similar to comment in https://codereview.chromium.org/2173543002/ > > Should this be ShellMetaTagsTest? Or do you think it's better to drop the shell. > Most of the files in ios/web/shell start with shell. In ios/web/shell files Shell prefix is used for shell specific implementation classes. F.e. WebClient has ShellWebClient and ChromeWebClient implementations. So that pattern does not apply for EG tests. https://codereview.chromium.org/2170243002/diff/1/ios/web/shell/test/meta_tag... ios/web/shell/test/meta_tags_egtest.mm:72: assertWithMatcher:grey_notNil()]; On 2016/07/22 15:53:26, baxley wrote: > This has a 4 second timeout. In the case where the delay is 3 seconds, do you > think that this is cutting it close? If I'm misunderstanding the meta refresh > time, let me know. Good point. |runMetaRefreshTestWithRefreshInterval:| should still work if interval is 5 seconds and I should not steal time from addressFieldText.
lgtm https://codereview.chromium.org/2170243002/diff/20001/ios/web/shell/test/page... File ios/web/shell/test/page_state_egtest.mm (left): https://codereview.chromium.org/2170243002/diff/20001/ios/web/shell/test/page... ios/web/shell/test/page_state_egtest.mm:56: super-tiny nit: Should we make this the same in the other tests files? I don't know if the style rule is to have a blank line or not in an empty @interface, so either is fine with me.
Thanks! https://codereview.chromium.org/2170243002/diff/20001/ios/web/shell/test/page... File ios/web/shell/test/page_state_egtest.mm (left): https://codereview.chromium.org/2170243002/diff/20001/ios/web/shell/test/page... ios/web/shell/test/page_state_egtest.mm:56: On 2016/07/22 18:36:18, baxley wrote: > super-tiny nit: Should we make this the same in the other tests files? I don't > know if the style rule is to have a blank line or not in an empty @interface, so > either is fine with me. There is no rule for this, probably did by accident. Change everywhere in a separate CL.
The CQ bit was checked by eugenebut@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from baxley@chromium.org Link to the patchset: https://codereview.chromium.org/2170243002/#ps40001 (title: "Merged")
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
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by eugenebut@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from baxley@chromium.org Link to the patchset: https://codereview.chromium.org/2170243002/#ps60001 (title: "Added missing dependency.")
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by eugenebut@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [ios] Implemented Meta Tags EarlGrey tests. BUG=630411 ========== to ========== [ios] Implemented Meta Tags EarlGrey tests. BUG=630411 Committed: https://crrev.com/2b43f1201efe386b7c28c536184b2ec472282f8c Cr-Commit-Position: refs/heads/master@{#407328} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/2b43f1201efe386b7c28c536184b2ec472282f8c Cr-Commit-Position: refs/heads/master@{#407328} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
