Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9114056:
Automated chrome driver tests for about:plugins functionality tests (Closed)
|
Created:
8 years, 11 months ago by vivianz Modified:
8 years, 10 months ago CC:
chromium-reviews Visibility:
Public. |
DescriptionAutomated chrome driver tests for about:plugins functionality tests
BUG=109663, 95586
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=119978
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 26
Patch Set 4 : '' #
Total comments: 4
Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 13
Messages
Total messages: 10 (0 generated)
lgtm once trybots are green
http://codereview.chromium.org/9114056/diff/18001/functional/PYAUTO_TESTS File functional/PYAUTO_TESTS (right): http://codereview.chromium.org/9114056/diff/18001/functional/PYAUTO_TESTS#new... functional/PYAUTO_TESTS:428: 'aboutplugins_ui', your first test can run on all builds, cant it? http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.py File functional/aboutplugins_ui.py (right): http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:1: import os rename this file to about_plugins_ui.py http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:6: import test_utils you don't use this; remove? http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:18: raw_input('Interact with the browser and hit <enter> to list plugins...') this method is more useful for the plugins test; how about change it to be more useful for us: instead of while True: ... just do self.NavigateToURL('chrome://plugins/') driver = self.NewWebDriver() import pdb pdb.set_trace() http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:41: When PDF Viewer plugin is enabled, navigate the site should not trigger navigating to the site http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:42: PDF file downloade and if PDF viewer plugin is disabled, PDF file should downloade? http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:43: be viewable in browser. if plugin is disable, the pdf should be viewable in the browser? i think you wrote it backwards http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:62: driver = self.NewWebDriver() don't create a new webdriver here; pass it in as a argument to this func http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:76: '//*[@jscontent="name"][text()="Remoting Viewer"]' + don't use jscontent, use class instead http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:77: '//ancestor::*[@class="plugin-text"]//a[text()="Disable"]')) == 1)) this isn't sufficient to determine if the details are shown or not, since the Disable link is also present in compact mode http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:107: # Need to sleep off 100ms for the detail-info expansion webkit transition. ? http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:110: self._IsEnabled('Chrome PDF Viewer'))) i don't think this is correct http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:131: driver = self.NewWebDriver() don't make a new driver
http://codereview.chromium.org/9114056/diff/18001/functional/PYAUTO_TESTS File functional/PYAUTO_TESTS (right): http://codereview.chromium.org/9114056/diff/18001/functional/PYAUTO_TESTS#new... functional/PYAUTO_TESTS:428: 'aboutplugins_ui', On 2012/01/20 17:16:28, kkania wrote: > your first test can run on all builds, cant it? Done. http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.py File functional/aboutplugins_ui.py (right): http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:1: import os On 2012/01/20 17:16:28, kkania wrote: > rename this file to about_plugins_ui.py Done. http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:6: import test_utils On 2012/01/20 17:16:28, kkania wrote: > you don't use this; remove? Done. http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:18: raw_input('Interact with the browser and hit <enter> to list plugins...') On 2012/01/20 17:16:28, kkania wrote: > this method is more useful for the plugins test; how about change it to be more > useful for us: > > instead of > while True: > ... > > just do > self.NavigateToURL('chrome://plugins/') > driver = self.NewWebDriver() > import pdb > pdb.set_trace() Done. http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:41: When PDF Viewer plugin is enabled, navigate the site should not trigger On 2012/01/20 17:16:28, kkania wrote: > navigating to the site Done. http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:42: PDF file downloade and if PDF viewer plugin is disabled, PDF file should On 2012/01/20 17:16:28, kkania wrote: > downloade? fixed http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:43: be viewable in browser. On 2012/01/20 17:16:28, kkania wrote: > if plugin is disable, the pdf should be viewable in the browser? i think you > wrote it backwards my bad :) fixed http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:62: driver = self.NewWebDriver() On 2012/01/20 17:16:28, kkania wrote: > don't create a new webdriver here; pass it in as a argument to this func Done. http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:76: '//*[@jscontent="name"][text()="Remoting Viewer"]' + On 2012/01/20 17:16:28, kkania wrote: > don't use jscontent, use class instead have to use jscontent, that is the only unique label there for detail info. http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:77: '//ancestor::*[@class="plugin-text"]//a[text()="Disable"]')) == 1)) On 2012/01/20 17:16:28, kkania wrote: > this isn't sufficient to determine if the details are shown or not, since the > Disable link is also present in compact mode change to locate the detail plugin path for Remote Viewer plugin. http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:107: # Need to sleep off 100ms for the detail-info expansion webkit transition. On 2012/01/20 17:16:28, kkania wrote: > ? legacy comment deleted. http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:110: self._IsEnabled('Chrome PDF Viewer'))) On 2012/01/20 17:16:28, kkania wrote: > i don't think this is correct change to assertTrue wait until chrome PDF viewer is not enabled. http://codereview.chromium.org/9114056/diff/18001/functional/aboutplugins_ui.... functional/aboutplugins_ui.py:131: driver = self.NewWebDriver() On 2012/01/20 17:16:28, kkania wrote: > don't make a new driver Done. removed.
did the trybots pass? http://codereview.chromium.org/9114056/diff/22001/functional/PYAUTO_TESTS File functional/PYAUTO_TESTS (right): http://codereview.chromium.org/9114056/diff/22001/functional/PYAUTO_TESTS#new... functional/PYAUTO_TESTS:553: # No Web Driver test for ChromeOS . at end http://codereview.chromium.org/9114056/diff/22001/functional/PYAUTO_TESTS#new... functional/PYAUTO_TESTS:554: '-aboutplugins_ui', i think you also need to put this in the continuous chromeos section, or chromeos is going to run the 1 test you list up top
http://codereview.chromium.org/9114056/diff/22001/functional/PYAUTO_TESTS File functional/PYAUTO_TESTS (right): http://codereview.chromium.org/9114056/diff/22001/functional/PYAUTO_TESTS#new... functional/PYAUTO_TESTS:553: # No Web Driver test for ChromeOS On 2012/01/30 19:26:01, kkania wrote: > . at end Done. http://codereview.chromium.org/9114056/diff/22001/functional/PYAUTO_TESTS#new... functional/PYAUTO_TESTS:554: '-aboutplugins_ui', On 2012/01/30 19:26:01, kkania wrote: > i think you also need to put this in the continuous chromeos section, or > chromeos is going to run the 1 test you list up top Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vivianz@chromium.org/9114056/30001
Change committed as 119978
http://codereview.chromium.org/9114056/diff/30001/functional/PYAUTO_TESTS File functional/PYAUTO_TESTS (right): http://codereview.chromium.org/9114056/diff/30001/functional/PYAUTO_TESTS#new... functional/PYAUTO_TESTS:294: '-aboutplugins_ui', this is wrong and has caused pyauto tests to fail on chromeos It should be: about_plugins_ui
I've fixed the chromeos problem in http://src.chromium.org/viewvc/chrome?view=rev&revision=120303 Please prepare a followup CL to address the comments I've made. http://codereview.chromium.org/9114056/diff/30001/functional/PYAUTO_TESTS File functional/PYAUTO_TESTS (right): http://codereview.chromium.org/9114056/diff/30001/functional/PYAUTO_TESTS#new... functional/PYAUTO_TESTS:294: '-aboutplugins_ui', You've added only one test in line 37. Why attempt to exclude all here? http://codereview.chromium.org/9114056/diff/30001/functional/PYAUTO_TESTS#new... functional/PYAUTO_TESTS:294: '-aboutplugins_ui', I've fixed it in http://src.chromium.org/viewvc/chrome?view=rev&revision=120303 http://codereview.chromium.org/9114056/diff/30001/functional/PYAUTO_TESTS#new... functional/PYAUTO_TESTS:438: 'about_plugins_ui', FULL = CONTINUOUS + extras You've already specified one test in CONTINUOUS. So now FULL suite will run testAboutPluginDetailInfo twice!! Not what you want. It's better to: include 'about_plugins_ui' in CONTINUOUS, and specifically disable that one test there, and re-add it in FULL But why did you pick PDF as a part of this UI test? http://codereview.chromium.org/9114056/diff/30001/functional/PYAUTO_TESTS#new... functional/PYAUTO_TESTS:562: '-aboutplugins_ui', FWIW, webdriver works on chromeos now. So this should not be necessary. http://codereview.chromium.org/9114056/diff/30001/functional/about_plugins_ui.py File functional/about_plugins_ui.py (right): http://codereview.chromium.org/9114056/diff/30001/functional/about_plugins_ui... functional/about_plugins_ui.py:14: """TestCase for chrome://plugins.""" UI tests for chrome://plugins http://codereview.chromium.org/9114056/diff/30001/functional/about_plugins_ui... functional/about_plugins_ui.py:32: if os.path.exists(path): os.remove(path) move os.remove to next line. OR os.path.exists(path) and os.remove(path) http://codereview.chromium.org/9114056/diff/30001/functional/about_plugins_ui... functional/about_plugins_ui.py:34: if os.path.exists(crdownload): os.remove(crdownload) same here http://codereview.chromium.org/9114056/diff/30001/functional/about_plugins_ui... functional/about_plugins_ui.py:63: if is_pdf_enabled == False: if not is_pdf_enabled: http://codereview.chromium.org/9114056/diff/30001/functional/about_plugins_ui... functional/about_plugins_ui.py:64: self.assertTrue(self.WaitUntil(lambda: Why are you doing functional verification in a test for ui? http://codereview.chromium.org/9114056/diff/30001/functional/about_plugins_ui... functional/about_plugins_ui.py:67: self.assertTrue(self.WaitUntil(lambda: len( what does this check for? http://codereview.chromium.org/9114056/diff/30001/functional/about_plugins_ui... functional/about_plugins_ui.py:82: def testAboutPluginEnableAndDisablePDFPlugin(self): Couldn't you have picked some other plugin so that you don't have to muck with continuous vs FULL suite? http://codereview.chromium.org/9114056/diff/30001/functional/about_plugins_ui... functional/about_plugins_ui.py:83: """Verify enable and disable plugins from about:plugins page.""" If you really want to verify functionality, move the test to plugins.py rather than _ui.py Do only UI verification here, no functional verification, especially not if you have to make it so elaborate. |
