Convert exceptions to errors, fix time recording

This makes a few changes that are a bit intermixed in the code:

1. Changes recording of start time and end time so that they record when the
underlying client process began and ended, not when the the actual tests
began (the revised behavior matches the spec).
2. Changes recording of s2c/c2s end_time values so that it actually waits for
the element it's waiting for before recording the time.
3. Changes two error cases so that instead of raising ValueError and killing
the process. One case was changed to a custom exception that kills the
process, as it should never occur. The other case was changed to a graceful
error that is recorded in results, but does not kill the process.
4. Removes the timeout handling logic from the result page parsing because
parsing the result page cannot time out (does not use WebDriverWait).

In addition, refactors html_driver a bit so that there is less mixing of
abstraction layers within functions.

Lastly, deletes the freezegun unit test as it was not providing additional
coverage over the final timing test and it's an additional unneeded
dependency.

client_wrapper/html5_driver.py

@@ -19,12 +19,26 @@
 import pytz
 from selenium import webdriver
 from selenium.webdriver.support import ui
-from selenium.webdriver.support import expected_conditions as EC
+from selenium.webdriver.support import expected_conditions
 from selenium.common import exceptions
 
 import names
 import results
 
+# TODO(mtlynch): Define all error strings as public constants so we're not
+# duplicating strings between production code and unit test code.
+ERROR_C2S_NEVER_STARTED = 'Timed out waiting for c2s test to begin.'
+ERROR_S2C_NEVER_STARTED = 'Timed out waiting for c2s test to begin.'
+ERROR_S2C_NEVER_ENDED = 'Timed out waiting for c2s test to end.'
+
+
+class Error(Exception):
+    pass
+
+
+class UnsupportedBrowserError(Error):
+    pass
+
 
 class NdtHtml5SeleniumDriver(object):
 
@@ -49,24 +63,16 @@ def perform_test(self):
         """
         result = results.NdtResult(start_time=None, end_time=None, errors=[])
         result.client = names.NDT_HTML5
+        result.start_time = datetime.datetime.now(pytz.utc)
 
         with contextlib.closing(_create_browser(self._browser)) as driver:
             result.browser = self._browser
             result.browser_version = driver.capabilities['version']
 
-            if not _load_url(driver, self._url, result):
-                return result
-
-            _click_start_button(driver, result)
-
-            if not _record_test_in_progress_values(result, driver,
-                                                   self._timeout):
-                return result
+            _complete_ui_flow(driver, self._url, self._timeout, result)
 
-            if not _populate_metric_values(result, driver):
-                return result
-
-            return result
+        result.end_time = datetime.datetime.now(pytz.utc)
+        return result
 
 
 def _create_browser(browser):
@@ -78,6 +84,9 @@ def _create_browser(browser):
     Returns:
         An instance of a Selenium webdriver browser class corresponding to
         the specified browser.
+
+    Raises:
+        UnsupportedBrowserError: When an invalid browser is specified.
     """
     if browser == names.FIREFOX:
         return webdriver.Firefox()
@@ -87,7 +96,7 @@ def _create_browser(browser):
         return webdriver.Edge()
     elif browser == names.SAFARI:
         return webdriver.Safari()
-    raise ValueError('Invalid browser specified: %s' % browser)
+    raise UnsupportedBrowserError('Invalid browser specified: %s' % browser)
 
 
 def _load_url(driver, url, result):
@@ -109,95 +118,93 @@ def _load_url(driver, url, result):
     return True
 
 
-def _click_start_button(driver, result):
-    """Clicks start test button and records start time.
+def _complete_ui_flow(driver, url, timeout, result):
+    """Performs the UI flow for the NDT HTML5 test and records results.
+
+    Args:
+        driver: An instance of a Selenium webdriver browser class.
+        url: URL to load to start the UI flow.
+        timeout: Maximum time (in seconds) to wait for any element to appear in
+            the flow.
+        result: NdtResult instance to populate with results from proceeding
+            through the UI flow.
+    """
+    if not _load_url(driver, url, result):
+        return
+
+    _click_start_button(driver)
+    result.c2s_result = results.NdtSingleTestResult()
+    result.s2c_result = results.NdtSingleTestResult()
+
+    if _wait_for_c2s_test_to_start(driver, timeout):
+        result.c2s_result.start_time = datetime.datetime.now(pytz.utc)
+    else:
+        result.errors.append(results.TestError(ERROR_C2S_NEVER_STARTED))
+
+    if _wait_for_s2c_test_to_start(driver, timeout):
+        result.c2s_result.end_time = datetime.datetime.now(pytz.utc)
+        result.s2c_result.start_time = datetime.datetime.now(pytz.utc)
+    else:
+        result.errors.append(results.TestError(ERROR_S2C_NEVER_STARTED))
+
+    if _wait_for_results_page_to_appear(driver, timeout):
+        result.s2c_result.end_time = datetime.datetime.now(pytz.utc)
+    else:
+        result.errors.append(results.TestError(ERROR_S2C_NEVER_ENDED))
 
-    Clicks the start test button for an NDT test and records the start time in
-    an NdtResult instance.
+    _populate_metric_values(result, driver)
+
+
+def _click_start_button(driver):
+    """Clicks the "Start Test" button in the web UI.
 
     Args:
         driver: An instance of a Selenium webdriver browser class.
-        result: An instance of an NdtResult class.
     """
     driver.find_element_by_id('websocketButton').click()
 
     start_button = driver.find_elements_by_xpath(
         "//*[contains(text(), 'Start Test')]")[0]
     start_button.click()
-    result.start_time = datetime.datetime.now(pytz.utc)
-
 
-def _record_test_in_progress_values(result, driver, timeout):
-    """Records values that are measured while the NDT test is in progress.
 
-    Measures s2c_start_time, c2s_end_time, and end_time, which are stored in
-    an instance of NdtResult. These times are measured while the NDT test is
-    in progress.
-
-    Args:
-        result: An instance of NdtResult.
-        driver: An instance of a Selenium webdriver browser class.
-        timeout: The number of seconds that the driver will wait for
-            each element to become visible before timing out.
+def _wait_for_c2s_test_to_start(driver, timeout):
+    # Wait until the 'Now Testing your upload speed' banner is displayed.
+    upload_speed_text = driver.find_elements_by_xpath(
+        "//*[contains(text(), 'your upload speed')]")[0]
+    return _wait_until_element_is_visible(driver, upload_speed_text, timeout)
 
-    Returns:
-        True if recording the measured values was successful, False if otherwise.
-    """
-    try:
-        # wait until 'Now Testing your upload speed' is displayed
-        upload_speed_text = driver.find_elements_by_xpath(
-            "//*[contains(text(), 'your upload speed')]")[0]
-        result.c2s_result = results.NdtSingleTestResult()
-        result.c2s_result.start_time = _record_time_when_element_displayed(
-            upload_speed_text,
-            driver,
-            timeout=timeout)
-        result.c2s_result.end_time = datetime.datetime.now(pytz.utc)
 
-        # wait until 'Now Testing your download speed' is displayed
-        download_speed_text = driver.find_elements_by_xpath(
-            "//*[contains(text(), 'your download speed')]")[0]
-        result.s2c_result = results.NdtSingleTestResult()
-        result.s2c_result.start_time = _record_time_when_element_displayed(
-            download_speed_text,
-            driver,
-            timeout=timeout)
+def _wait_for_s2c_test_to_start(driver, timeout):
+    # Wait until the 'Now Testing your download speed' banner is displayed.
+    download_speed_text = driver.find_elements_by_xpath(
+        "//*[contains(text(), 'your download speed')]")[0]
+    return _wait_until_element_is_visible(driver, download_speed_text, timeout)
 
-        # wait until the results page appears
-        results_text = driver.find_element_by_id('results')
-        result.s2c_result.end_time = datetime.datetime.now(pytz.utc)
-        result.end_time = _record_time_when_element_displayed(results_text,
-                                                              driver,
-                                                              timeout=timeout)
-    except exceptions.TimeoutException:
-        result.errors.append(results.TestError(
-            'Test did not complete within timeout period.'))
-        return False
-    return True
 
+def _wait_for_results_page_to_appear(driver, timeout):
+    results_text = driver.find_element_by_id('results')
+    return _wait_until_element_is_visible(driver, results_text, timeout)
 
-def _record_time_when_element_displayed(element, driver, timeout):
-    """Return the time when the specified element is displayed.
 
-    The Selenium WebDriver checks whether the specified element is visible. If
-    it becomes visible before the request has timed out, the timestamp of the
-    time when the element becomes visible is returned.
+def _wait_until_element_is_visible(driver, element, timeout):
+    """Waits until a DOM element is visible in within a given timeout.
 
     Args:
-        element: A selenium webdriver element.
         driver: An instance of a Selenium webdriver browser class.
-        timeout: The number of seconds that the driver will wait for
-            each element to become visible before timing out.
+        element: A selenium webdriver element.
+        timeout: The maximum time to wait (in seconds).
 
     Returns:
-        A datetime object with a timezone information attribute.
-
-    Raises:
-        TimeoutException: If the element does not become visible before the
-            timeout time passes.
+        True if the element became visible within the timeout.
     """
-    ui.WebDriverWait(driver, timeout=timeout).until(EC.visibility_of(element))
-    return datetime.datetime.now(pytz.utc)
+    try:
+        ui.WebDriverWait(
+            driver,
+            timeout=timeout).until(expected_conditions.visibility_of(element))
+    except exceptions.TimeoutException:
+        return False
+    return True
 
 
 def _populate_metric_values(result, driver):
@@ -210,36 +217,22 @@ def _populate_metric_values(result, driver):
     Args:
         result: An instance of NdtResult.
         driver: An instance of a Selenium webdriver browser class.
-
-    Returns:
-        True if populating metrics and checking their values was successful.
-            False if otherwise.
     """
-    try:
-        c2s_throughput = driver.find_element_by_id('upload-speed').text
-        c2s_throughput_units = driver.find_element_by_id(
-            'upload-speed-units').text
+    c2s_throughput = driver.find_element_by_id('upload-speed').text
+    c2s_throughput_units = driver.find_element_by_id('upload-speed-units').text
 
-        result.c2s_result.throughput = _parse_throughput(
-            result.errors, c2s_throughput, c2s_throughput_units,
-            'c2s throughput')
+    result.c2s_result.throughput = _parse_throughput(
+        result.errors, c2s_throughput, c2s_throughput_units, 'c2s throughput')
 
-        s2c_throughput = driver.find_element_by_id('download-speed').text
+    s2c_throughput = driver.find_element_by_id('download-speed').text
 
-        s2c_throughput_units = driver.find_element_by_id(
-            'download-speed-units').text
-        result.s2c_result.throughput = _parse_throughput(
-            result.errors, s2c_throughput, s2c_throughput_units,
-            's2c throughput')
+    s2c_throughput_units = driver.find_element_by_id(
+        'download-speed-units').text
+    result.s2c_result.throughput = _parse_throughput(
+        result.errors, s2c_throughput, s2c_throughput_units, 's2c throughput')
 
-        result.latency = driver.find_element_by_id('latency').text
-        result.latency = _validate_metric(result.errors, result.latency,
-                                          'latency')
-    except exceptions.TimeoutException:
-        result.errors.append(results.TestError(
-            'Test did not complete within timeout period.'))
-        return False
-    return True
+    result.latency = driver.find_element_by_id('latency').text
+    result.latency = _validate_metric(result.errors, result.latency, 'latency')
 
 
 def _parse_throughput(errors, throughput, throughput_units,
@@ -273,8 +266,8 @@ def _parse_throughput(errors, throughput, throughput_units,
         elif throughput_units == 'Mb/s':
             return throughput
         else:
-            raise ValueError('Invalid throughput unit specified: %s' %
-                             throughput_units)
+            errors.append(results.TestError(
+                'Invalid throughput unit specified: %s' % throughput_units))
     return None
 
 

client_wrapper/results.py

@@ -59,10 +59,13 @@ class NdtResult(object):
     """Represents the results of a complete NDT HTML5 client test.
 
     Attributes:
-        start_time: The datetime at which tests were initiated (i.e. the time
-            the driver pushed the 'Start Test' button).
-        end_time: The datetime at which the tests completed (i.e. the time the
-            results page loaded).
+        start_time: The datetime at which the NDT client was launched. This is
+            time at which the client wrapper begins running a particular client,
+            but is not necessarily the time at which the client itself initiated
+            a test.
+        end_time: The datetime at which the NDT client completed. This should be
+            equal to the end_time of the client's last test or the time of a
+            fatal error in the client.
         errors: A list of TestError objects representing any errors encountered
             during the tests (or an empty list if all tests were successful).
         c2s_result: The NdtSingleResult for the c2s (upload) test (or None if no

test_requirements.txt

@@ -1,2 +1 @@
 mock==1.3.0
-freezegun==0.3.6