Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1882603003:
[Chromedriver] Add testcase for testing alert on new window. (Closed)
|
Created:
4 years, 8 months ago by gmanikpure Modified:
3 years, 8 months ago Reviewers:
samuong CC:
chromium-reviews, samuong+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Chromedriver] Add testcase for testing alert on new window.
And disable it by default until the issue is fixed.
BUG=chromedriver:833
Review-Url: https://codereview.chromium.org/1882603003
Cr-Commit-Position: refs/heads/master@{#459971}
Committed: https://chromium.googlesource.com/chromium/src/+/1512539632abf713b55f5a16fe7b709fb1f4a57d
Patch Set 1 #
Total comments: 6
Patch Set 2 : modifications #Patch Set 3 : disable this test on webview shell. #Patch Set 4 : Reland - Disable test in _NEGATIVE_FILTER #
Messages
Total messages: 23 (7 generated)
gmanikpure@chromium.org changed reviewers: + samuong@chromium.org
Hi Sam, Could you please have a look whenever time permits? Thanks, Gauri
https://codereview.chromium.org/1882603003/diff/1/chrome/test/chromedriver/te... File chrome/test/chromedriver/test/run_py_tests.py (right): https://codereview.chromium.org/1882603003/diff/1/chrome/test/chromedriver/te... chrome/test/chromedriver/test/run_py_tests.py:728: self._driver.Load(self._http_server.GetUrl() + '/newwindowWithAlert') i'd suggest just loading about:blank, and then setting document.body.innerHTML = '<a href=....' using ExecuteScript https://codereview.chromium.org/1882603003/diff/1/chrome/test/data/chromedriv... File chrome/test/data/chromedriver/alert_onload.html (right): https://codereview.chromium.org/1882603003/diff/1/chrome/test/data/chromedriv... chrome/test/data/chromedriver/alert_onload.html:1: <html> don't forget the <!DOCTYPE html> at the top https://codereview.chromium.org/1882603003/diff/1/chrome/test/data/chromedriv... chrome/test/data/chromedriver/alert_onload.html:3: <script> alert('This is a javascript alert.'); </script> nit: don't put spaces before the alert statement
https://codereview.chromium.org/1882603003/diff/1/chrome/test/chromedriver/te... File chrome/test/chromedriver/test/run_py_tests.py (right): https://codereview.chromium.org/1882603003/diff/1/chrome/test/chromedriver/te... chrome/test/chromedriver/test/run_py_tests.py:728: self._driver.Load(self._http_server.GetUrl() + '/newwindowWithAlert') On 2017/03/27 18:22:06, samuong wrote: > i'd suggest just loading about:blank, and then setting document.body.innerHTML = > '<a href=....' using ExecuteScript I get 'element not visible error' at line730 when I use below: self._driver.Load(self.GetHttpUrlForFile('/chromedriver/empty.html')) script = (""" document.body.innerHTML="<a href='%s' target='_blank'></a>";""" % self.GetHttpUrlForFile('/chromedriver/alert_onload.html')) Can you please let me know if I am missing anything?
What about using window.open instead?
ExecuteScript('window.open(%s)' %
self.GetHttpUrlForFile('/chromedriver/alert_onload.html'))
On 2017/03/27 20:41:22, samuong wrote:
> What about using window.open instead?
>
> ExecuteScript('window.open(%s)' %
> self.GetHttpUrlForFile('/chromedriver/alert_onload.html'))
Thank you, that works!
Please take a look.
https://codereview.chromium.org/1882603003/diff/1/chrome/test/data/chromedriv... File chrome/test/data/chromedriver/alert_onload.html (right): https://codereview.chromium.org/1882603003/diff/1/chrome/test/data/chromedriv... chrome/test/data/chromedriver/alert_onload.html:1: <html> On 2017/03/27 18:22:07, samuong wrote: > don't forget the <!DOCTYPE html> at the top Done. https://codereview.chromium.org/1882603003/diff/1/chrome/test/data/chromedriv... chrome/test/data/chromedriver/alert_onload.html:3: <script> alert('This is a javascript alert.'); </script> On 2017/03/27 18:22:06, samuong wrote: > nit: don't put spaces before the alert statement Done.
lgtm, but put the bug number in the BUG= line rather than the title
Description was changed from ========== [Chromedriver] Adding testcase for Issue833 And disabled it by default until the issue is fixed. BUG= ========== to ========== [Chromedriver] Adding testcase for Issue833 And disabled it by default until the issue is fixed. BUG=chromedriver:833 ==========
Description was changed from ========== [Chromedriver] Adding testcase for Issue833 And disabled it by default until the issue is fixed. BUG=chromedriver:833 ========== to ========== [Chromedriver] Adding testcase for testing alert on new window. And disable it by default until the issue is fixed. BUG=chromedriver:833 ==========
On 2017/03/27 21:08:26, samuong wrote: > lgtm, but put the bug number in the BUG= line rather than the title Done.
Oh one more thing, put newlines after the summary line and before the BUG= line: https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list...
Description was changed from ========== [Chromedriver] Adding testcase for testing alert on new window. And disable it by default until the issue is fixed. BUG=chromedriver:833 ========== to ========== [Chromedriver] Add testcase for testing alert on new window. And disable it by default until the issue is fixed. BUG=chromedriver:833 ==========
On 2017/03/27 21:29:12, samuong wrote: > Oh one more thing, put newlines after the summary line and before the BUG= line: > > https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list... Thank you for pointing to the doc. I will make note of it.
thanks, lgtm
The CQ bit was checked by gmanikpure@chromium.org
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": 1490650432960680,
"parent_rev": "8a121558528bacde6c6f5fd95429a1b50cfba876", "commit_rev":
"1512539632abf713b55f5a16fe7b709fb1f4a57d"}
Message was sent while issue was closed.
Description was changed from ========== [Chromedriver] Add testcase for testing alert on new window. And disable it by default until the issue is fixed. BUG=chromedriver:833 ========== to ========== [Chromedriver] Add testcase for testing alert on new window. And disable it by default until the issue is fixed. BUG=chromedriver:833 Review-Url: https://codereview.chromium.org/1882603003 Cr-Commit-Position: refs/heads/master@{#459971} Committed: https://chromium.googlesource.com/chromium/src/+/1512539632abf713b55f5a16fe7b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/1512539632abf713b55f5a16fe7b...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2780723004/ by samuong@chromium.org. The reason for reverting is: This seems to be causing waterfall failures (I think the filter should be added to _NEGATIVE_FILTER, not the Android-specific filter?).
Message was sent while issue was closed.
On 2017/03/29 00:01:10, samuong wrote: > A revert of this CL (patchset #3 id:40001) has been created in > https://codereview.chromium.org/2780723004/ by mailto:samuong@chromium.org. > > The reason for reverting is: This seems to be causing waterfall failures (I > think the filter should be added to _NEGATIVE_FILTER, not the Android-specific > filter?). Sorry, patch set1 had the test in _NEGATIVE_FILTER. Unfortunately, missed to include it in later patch sets after rebasing to the latest checkout. I will make the required change and send it for review.
Message was sent while issue was closed.
lgtm for the reland patchset |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
