reuse the same browser when running webdriver tests

tools/testing/run_selenium.py

Index: tools/testing/run_selenium.py
diff --git a/tools/testing/run_selenium.py b/tools/testing/run_selenium.py
index 93f2c1b723a8c90afd47e294d4925bf8c15d1582..391fd7bdb95befb61f822c695f930ced29f2fb1c 100755
--- a/tools/testing/run_selenium.py
+++ b/tools/testing/run_selenium.py
@@ -6,7 +6,10 @@
 #
 
 """Script to actually open a browser and perform the test, and reports back with
-the result.
+the result. It uses Selenium WebDriver for running the tests.

[Emily Fortuna, 2012/02/21 18:19:35]

Selenium RC and Selenium Webdriver (Selenium Webdriver is used for everything
but Safari, which uses plain-jane Selenium, also called Selenium Remote
Control).

[Jennifer Messerly, 2012/02/21 18:58:43]

Done.

+
+If started without arguments, this script runs a batch of in-browser tests in
+the same browser process. Batching gives faster throughput and makes tests less subject to browser starting flakiness, issues with too many browser processes running, etc.

[Emily Fortuna, 2012/02/21 18:19:35]

line > 80 char.
Also maybe give an example how the two different cases are invoked. Explain how
the browser is specified by reading from stdin in the batch case, for example.

[Jennifer Messerly, 2012/02/21 18:58:43]

Done.

 """
 
 import os
@@ -18,6 +21,9 @@ import shutil
 import socket
 import sys
 import time
+import signal
+
+TIMEOUT_ERROR_MSG = 'FAIL (timeout)'
 
 def perf_test_done(driver):
   """Checks if the performance test has completed."""
@@ -35,10 +41,13 @@ def perf_test_done_helper(source):
   return 'Score:' in source
 
 def run_test_in_browser(browser, html_out, timeout, is_perf):
-  """Run the desired test in the browser using Selenium 2.0 WebDriver syntax, 
+  """Run the desired test in the browser using Selenium 2.0 WebDriver syntax,
   and wait for the test to complete. This is the newer syntax, that currently
   supports Firefox, Chrome, IE, Opera (and some mobile browsers)."""
-  browser.get("file://" + html_out) 
+  if isinstance(browser, selenium.selenium):
+    return run_test_in_browser_selenium1(browser, html_out, timeout, is_perf)

[Emily Fortuna, 2012/02/21 18:19:35]

I know I came up with the name originally  here, but maybe call the method
run_tests_in_browser_selenium_rc instead of selenium1.

[Jennifer Messerly, 2012/02/21 18:58:43]

Done.

+
+  browser.get("file://" + html_out)
   source = ''
   try:
     if is_perf:
@@ -50,16 +59,7 @@ def run_test_in_browser(browser, html_out, timeout, is_perf):
           ('FAIL' in driver.page_source))
     source = browser.page_source
   except selenium.common.exceptions.TimeoutException:
-    source = 'FAIL (timeout)'
-  finally: 
-    # A timeout exception is thrown if nothing happens within the time limit.
-    if browser != 'chrome':
-      browser.close()
-    try:
-      browser.quit()
-    except selenium.common.exceptions.WebDriverException:
-      #TODO(efortuna): figure out why this crashes.... and avoid?
-      pass
+    source = TIMEOUT_ERROR_MSG
   return source
 
 def run_test_in_browser_selenium1(sel, html_out, timeout, is_perf):
@@ -68,7 +68,7 @@ def run_test_in_browser_selenium1(sel, html_out, timeout, is_perf):
   supported on Selenium 2.0."""
   sel.open('file://' + html_out)
   source = sel.get_html_source()
-  def end_condition(source): 
+  def end_condition(source):
     return 'PASS' in source or 'FAIL' in source
   if is_perf:
     end_condition = perf_test_done_helper
@@ -79,66 +79,78 @@ def run_test_in_browser_selenium1(sel, html_out, timeout, is_perf):
     time.sleep(sec)
     elapsed += sec
     source = sel.get_html_source()
-  sel.stop()
   return source
 
-def parse_args():
+def parse_args(args=None):
   parser = optparse.OptionParser()
-  parser.add_option('--out', dest='out', 
-      help = 'The path for html output file that we will running our test from', 
-      action = 'store', default = '') 
-  parser.add_option('--browser', dest='browser', 
-      help = 'The browser type (default = chrome)', 
+  parser.add_option('--out', dest='out',
+      help = 'The path for html output file that we will running our test from',
+      action = 'store', default = '')
+  parser.add_option('--browser', dest='browser',
+      help = 'The browser type (default = chrome)',
       action = 'store', default = 'chrome')
-  # TODO(efortuna): Put this back up to be more than the default timeout in 
-  # test.dart. Right now it needs to be less than 60 so that when test.dart 
+  # TODO(efortuna): Put this back up to be more than the default timeout in
+  # test.dart. Right now it needs to be less than 60 so that when test.dart
   # times out, this script also closes the browser windows.
-  parser.add_option('--timeout', dest = 'timeout', 
-      help = 'Amount of time (seconds) to wait before timeout', type = 'int', 
+  parser.add_option('--timeout', dest = 'timeout',
+      help = 'Amount of time (seconds) to wait before timeout', type = 'int',
       action = 'store', default=58)
-  parser.add_option('--perf', dest = 'is_perf', 
-      help = 'Add this flag if we are running a browser performance test', 
+  parser.add_option('--perf', dest = 'is_perf',
+      help = 'Add this flag if we are running a browser performance test',
       action = 'store_true', default=False)
-  args, ignored = parser.parse_args()
+  args, ignored = parser.parse_args(args=args)
   return args.out, args.browser, args.timeout, args.is_perf
 
-def Main():
-  # Note: you need ChromeDriver *in your path* to run Chrome, in addition to 
-  # installing Chrome.
-  browser = None
-  html_out, browser, timeout, is_perf = parse_args()
-
+def start_browser(browser, html_out):

[Emily Fortuna, 2012/02/21 18:19:35]

Maybe call this get_browser instead of start_browser since we're returning the
newly created browser runner?

[Jennifer Messerly, 2012/02/21 18:58:43]

I worry about calling it "get" in that it sounds less side-effecty. It could be
get_and_start_browser? But since most functions "get" something, I usually drop
the "get"

(might be because I'm used to APIs like this:
http://msdn.microsoft.com/en-us/library/system.diagnostics.process.start.aspx)

   if browser == 'chrome':
-    browser = selenium.webdriver.Chrome()
+    # Note: you need ChromeDriver *in your path* to run Chrome, in addition to
+    # installing Chrome. Also note that the build bot runs have a different path
+    # from a normal user -- check the build logs.
+    return selenium.webdriver.Chrome()
   elif browser == 'ff':
     profile = selenium.webdriver.firefox.firefox_profile.FirefoxProfile()
     profile.set_preference('dom.max_script_run_time', 0)
     profile.set_preference('dom.max_chrome_script_run_time', 0)
-    browser = selenium.webdriver.Firefox(firefox_profile=profile)
+    return selenium.webdriver.Firefox(firefox_profile=profile)
   elif browser == 'ie' and platform.system() == 'Windows':
-    browser = selenium.webdriver.Ie()
+    return selenium.webdriver.Ie()
   elif browser == 'safari' and platform.system() == 'Darwin':
-    # TODO(efortuna): Ensure our preferences (no pop-up blocking) file is the 
-    # same (Safari auto-deletes when it has too many "crashes," or in our case, 
+    # TODO(efortuna): Ensure our preferences (no pop-up blocking) file is the
+    # same (Safari auto-deletes when it has too many "crashes," or in our case,
     # timeouts). Come up with a less hacky way to do this.
-    shutil.copy(os.path.dirname(__file__) + '/com.apple.Safari.plist', 
-        '/Library/Preferences/com.apple.Safari.plist')
+    # !!!!

[Emily Fortuna, 2012/02/21 18:19:35]

Do we have a workaround for this yet? We probably shouldn't comment out the two
lines below until we do. :-/

[Jennifer Messerly, 2012/02/21 18:58:43]

Oops! Thanks for catching that. Fixed--now checks if the file exists and copies
it if it does.

+    #shutil.copy(os.path.dirname(__file__) + '/com.apple.Safari.plist',
+    #    '/Library/Preferences/com.apple.Safari.plist')
     sel = selenium.selenium('localhost', 4444, "*safari", 'file://' + html_out)
     try:
       sel.start()
+      return sel
     except socket.error:
       print 'ERROR: Could not connect to Selenium RC server. Are you running' +\
           ' java -jar selenium-server-standalone-2.15.0.jar? If not, start ' + \
           'it before running this test.'
-      return 1
+      sys.exit(1)
   else:
     raise Exception('Incompatible browser and platform combination.')
-  source = ''
-  if browser == 'safari':
-    source = run_test_in_browser_selenium1(sel, html_out, timeout, is_perf)
-  else:
-    source = run_test_in_browser(browser, html_out, timeout, is_perf)
 
+def close_browser(browser):
+  if browser is None:
+    return
+  if isinstance(browser, selenium.selenium):
+    browser.stop()
+    return
+
+  # A timeout exception is thrown if nothing happens within the time limit.
+  print '!!! trying to close browser !!!'

[Emily Fortuna, 2012/02/21 18:19:35]

Do we always want to print this? Perhaps a more informative error message saying
that a timeout exception has occurred?

[Jennifer Messerly, 2012/02/21 18:58:43]

Oops. removed.

+  if browser != 'chrome':
+    browser.close()
+  try:
+    browser.quit()
+  except selenium.common.exceptions.WebDriverException:
+    # TODO(efortuna): figure out why this crashes.... and avoid?

[Emily Fortuna, 2012/02/21 18:19:35]

nit: Yes, this was my mistake, but let's make that a capital "F" at the start of
the sentence, per style guidelines.

[Jennifer Messerly, 2012/02/21 18:58:43]

Done.

+    pass
+
+def report_results(is_perf, source):
   if is_perf:
     # We're running a performance test.
     print source
@@ -152,16 +164,80 @@ def Main():
     if 'FAIL' not in source and 'PASS' in source:
       print 'Content-Type: text/plain\nPASS'
       return 0
-    else: 
+    else:
       #The hacky way to get document.getElementById('body').innerHTML for this
       # webpage, without the JavaScript.
       #TODO(efortuna): Access these elements in a nicer way using DOM parser.
       index = source.find('<body>')
       index += len('<body>')
       end_index = source.find('<script')
-      print source[index : end_index]
+      print unicode(source[index : end_index]).encode("utf-8")

[Emily Fortuna, 2012/02/21 18:19:35]

good catch here.

[Jennifer Messerly, 2012/02/21 18:58:43]

Done.

       return 1
 
+def run_batch_tests():

[Emily Fortuna, 2012/02/21 18:19:35]

Add some comments saying what's going on in this method.

[Jennifer Messerly, 2012/02/21 18:58:43]

Done.

+  print '>>> BATCH START'
+  browser = None
+  current_browser_name = None
+
+  # test.dart doesn't give us a chance to shut down gracefully, so handle
+  # SIGTERM instead. TODO(jmesserly): make this more robust

[Emily Fortuna, 2012/02/21 18:19:35]

FYI: I've filed a feature request about this here:
https://code.google.com/p/dart/issues/detail?id=1756
Maybe add issue # in comment here?

[Jennifer Messerly, 2012/02/21 18:58:43]

Done.

+  def sigterm(number, frame):
+    close_browser(browser)
+  signal.signal(signal.SIGTERM, sigterm)
+
+  try:
+    while True:
+      line = sys.stdin.readline()
+      if line == '--terminate\n':
+        break
+
+      html_out, browser_name, timeout, is_perf = parse_args(line.split())
+
+      # Sanity checks that test.dart is passing flags we can handle.
+      if is_perf:
+        print 'Batch test runner not compatible with perf testing'
+        return 1
+      if browser and current_browser_name != browser_name:
+        print('Batch test runner got multiple browsers: %s and %s'
+            % (current_browser_name, browser_name))
+        return 1
+
+      # Start the browser on the first run
+      if browser is None:
+        current_browser_name = browser_name
+        browser = start_browser(browser_name, html_out)
+
+      source = run_test_in_browser(browser, html_out, timeout, is_perf)
+
+      # print one of:
+      # >>> TEST {PASS, FAIL, OK, CRASH, FAIL, TIMEOUT}
+      status = report_results(is_perf, source)
+      if status == 0:
+        print '>>> TEST PASS'

[Emily Fortuna, 2012/02/21 18:19:35]

I believe you need to print out the following "magic string" if a test passed:
'Content-Type: text/plain\nPASS'
(see line 164 for example). 

[Jennifer Messerly, 2012/02/21 18:58:43]

This is printed still by report_results. But I'm not sure if we need to keep
that, since ">>> TEST PASS" is the magic string used by the BatchRunnerProcess?

[Emily Fortuna, 2012/02/21 19:35:33]

Oh, I see. Seems fine then.

+      elif source == TIMEOUT_ERROR_MSG:
+        print '>>> TEST TIMEOUT'
+      else:
+        print '>>> TEST FAIL'
+      sys.stdout.flush()
+  finally:
+    close_browser(browser)
+
+
+def main(args):
+  # Run in batch mode if the --batch flag is passed.
+  # TODO(jmesserly): reconcile with the existing args parsing
+  if '--batch' in args:
+    return run_batch_tests()
+
+  # Run a single test
+  html_out, browser_name, timeout, is_perf = parse_args()
+  browser = start_browser(browser_name, html_out)
+
+  try:
+    output = run_test_in_browser(browser, html_out, timeout, is_perf)
+    return report_results(is_perf, output)
+  finally:
+    close_browser(browser)
 
 if __name__ == "__main__":
-  sys.exit(Main())
+  sys.exit(main(sys.argv))