[Fixit Fridays] Make webdriver helpers wait till the element exists, …

…and use those methods in tests (#4862)

Modified the base_utils find_elem and find_elems methods such that they
first have to wait for the element if it's not found. This allows us to
remove all that logic from within each test to check if the element
exists before trying to find it.

Also fixed how the XPath is sometimes used as the entire path multiple
times in the same test.

Moving forward, we should aim to always used find_elem, and find_elems
to retrieve elements for webdriver tests.
Will keep working towards moving over the old tests to using this
approach.

This also fixed the drought webdriver test which was previously skipped
for flakiness. I expect that this will allow us to unskip many other
tests as well.

server/routes/dev_place/utils.py

@@ -48,11 +48,12 @@
 ]
 
 # Place page categories
-OVERVIEW_CATEGORY = "Overview"
-ORDERED_CATEGORIES = [
-    OVERVIEW_CATEGORY, "Economics", "Health", "Equity", "Crime", "Education",
-    "Demographics", "Housing", "Environment", "Energy"
+ORDERED_TOPICS = [
+    "Economics", "Health", "Equity", "Crime", "Education", "Demographics",
+    "Housing", "Environment", "Energy"
 ]
+OVERVIEW_CATEGORY = "Overview"
+ORDERED_CATEGORIES = [OVERVIEW_CATEGORY] + ORDERED_TOPICS
 CATEGORIES = set(ORDERED_CATEGORIES)
 
 

server/webdriver/base_utils.py

@@ -13,11 +13,16 @@
 # limitations under the License.
 """Utilities used by the base webddriver test."""
 
+from typing import List
+
 from selenium import webdriver
 from selenium.webdriver.chrome.options import Options
+from selenium.webdriver.common.by import By
 from selenium.webdriver.support import expected_conditions as EC
 from selenium.webdriver.support.ui import WebDriverWait
 
+from server.webdriver import shared
+
 DEFAULT_HEIGHT = 1200
 DEFAULT_WIDTH = 1200
 
@@ -37,29 +42,81 @@ def create_driver(preferences=None):
   return driver
 
 
-def find_elem(parent, by: str, value: str):
+def find_parents(parents,
+                 by: str = By.CLASS_NAME,
+                 parent_path: List[str] = None
+                ) -> list[webdriver.remote.webelement.WebElement]:
   """
-  Finds an element within the parent element with the specified by string and value.
-  Returns None if not found.
+    Returns the final list of elements matching the 'by' in parent_path, waits for elements if needed.
+    Note that we only return the elements matching all the way to the last parent_path value.
+    For example:
+      <div id=block>
+        <div id=chart />
+        <div id=chart />
+      </div>
+      <div id=block>
+        <div id=chart />
+        <div id=chart />
+      </div>
+    For find_parents(driver, By.ID, ['block', 'chart']) --> returns all 4 chart elements within the blocks. 
+    """
+  if not parent_path:
+    return parents
+
+  elements_to_return = []
+  for value in parent_path:
+    this_level_elements = []
+    for par in parents:
+      this_level_elements.extend(find_elems(par, by, value))
+    elements_to_return = this_level_elements
+
+  return elements_to_return
+
+
+def find_elems(
+    parent: webdriver.remote.webelement.WebElement,
+    by: str = By.CLASS_NAME,
+    value: str = "",
+    path_to_elem: List[str] = None
+) -> list[webdriver.remote.webelement.WebElement]:
   """
-  try:
-    return parent.find_element(by, value)
-  except:
-    return None
+    Finds elements within the parent elements with the specified by string and value.
+    If not found, it will wait up to the timeout set, and then return None.
+    """
+  parents = find_parents([parent], by,
+                         path_to_elem) if path_to_elem else [parent]
 
+  elements = []
+  for par in parents:
+    wait_elem(par, by, value, shared.TIMEOUT)
+    found_elements = par.find_elements(by, value)
+    if found_elements:
+      elements.extend(found_elements)
+    else:
+      elems_or_none = wait_elem(par, by, value, shared.TIMEOUT)
+      if elems_or_none:
+        elements.append(elems_or_none)
+  return elements if elements else []
 
-def find_elems(parent, by: str, value: str):
+
+def find_elem(
+    parent: webdriver.remote.webelement.WebElement,
+    by: str = By.CLASS_NAME,
+    value: str = "",
+    path_to_elem: List[str] = None
+) -> webdriver.remote.webelement.WebElement | None:
   """
-  Finds elements within the parent element with the specified by string and value.
-  Returns None if not found.
+  Finds an element within the parent element with the specified by string and value.
+  If not found, it will wait up to the timeout set, and then return None.
   """
-  try:
-    return parent.find_elements(by, value)
-  except:
-    return None
+  elems = find_elems(parent, by, value, path_to_elem)
+  return elems[0] if elems else None
 
 
-def wait_elem(driver, by: str, value: str, timeout_seconds: float = 5):
+def wait_elem(driver,
+              by: str = By.CLASS_NAME,
+              value: str = "",
+              timeout_seconds: float = 5):
   """
   Waits for an element within the parent element with the specified by string and value.
   Uses a default timeout of 5 seconds.

server/webdriver/shared_tests/browser_test.py

@@ -17,6 +17,8 @@
 from selenium.webdriver.support.ui import WebDriverWait
 
 from server.webdriver import shared
+from server.webdriver.base_utils import find_elem
+from server.webdriver.base_utils import find_elems
 
 MTV_URL = '/browser/geoId/0649670'
 CA_POPULATION_URL = '/browser/geoId/06?statVar=Count_Person'
@@ -44,23 +46,16 @@ def test_page_landing(self):
     self.assertIn(title_text, self.driver.title)
 
     # Wait for title to be present
-    title_locator = (By.TAG_NAME, 'h1')
-    WebDriverWait(self.driver, self.TIMEOUT_SEC).until(
-        EC.text_to_be_present_in_element(title_locator, 'Knowledge Graph'))
-    title_element = self.driver.find_element(*title_locator)
     self.assertEqual(
-        "Knowledge Graph", title_element.text,
-        f"Expected title 'Knowledge Graph', but found: {title_element.text}")
+        find_elem(self.driver, by=By.TAG_NAME, value='h1').text,
+        "Knowledge Graph")
 
     # Assert intro is correct
-    description_locator = (
-        By.XPATH, "//h1[text()='Knowledge Graph']/following-sibling::p")
-    description_element = self.driver.find_element(*description_locator)
-    expected_description_start = 'The Data Commons Knowledge Graph is constructed'
     self.assertTrue(
-        description_element.text.startswith(expected_description_start),
-        f"Intro text does not start with expected text. Found: {description_element.text}"
-    )
+        find_elem(self.driver,
+                  by=By.XPATH,
+                  value="//h1[text()='Knowledge Graph']/following-sibling::p").
+        text.startswith("The Data Commons Knowledge Graph is constructed"))
 
   def test_page_serve_ca_population(self):
     """Test the browser page for California population can be loaded successfully."""
@@ -81,36 +76,32 @@ def test_page_serve_ca_population(self):
     self.assertEqual(title_text, self.driver.title)
 
     # Assert header is correct.
-    element_present = EC.presence_of_element_located((By.TAG_NAME, 'h1'))
-    WebDriverWait(self.driver, self.TIMEOUT_SEC).until(element_present)
-    statvar_title = self.driver.find_element(By.XPATH, '//*[@id="node"]/h1[1]')
-    self.assertEqual(statvar_title.text, 'Statistical Variable: Count_Person')
-    about_title = self.driver.find_element(By.XPATH, '//*[@id="node"]/h1[2]')
-    self.assertEqual(about_title.text, 'About: California')
+    node = find_elem(self.driver, by=By.XPATH, value='//*[@id="node"]')
+    self.assertEqual(
+        find_elem(node, by=By.XPATH, value='.//h1[1]').text,
+        'Statistical Variable: Count_Person')
+    self.assertEqual(
+        find_elem(node, by=By.XPATH, value='.//h1[2]').text,
+        'About: California')
 
     # Assert properties section shows dcid and typeOf values for the statistical variable
     # Count_Person.
-    element_present = EC.presence_of_element_located(
-        (By.XPATH, '//*[@id="node-content"]/div[1]/div/table'))
-    WebDriverWait(self.driver, self.TIMEOUT_SEC).until(element_present)
-    table = self.driver.find_element(
-        By.XPATH, '//*[@id="node-content"]/div[1]/div/table')
-    dcid_row = table.find_elements(By.XPATH, './/tbody/tr[2]/td')
+    table = find_elem(self.driver,
+                      by=By.XPATH,
+                      value='//*[@id="node-content"]/div[1]/div/table')
+    dcid_row = find_elems(table, by=By.XPATH, value='.//tbody/tr[2]/td')
     self.assertEqual(dcid_row[0].text, 'dcid')
     self.assertEqual(dcid_row[1].text, 'Count_Person')
-    type_of_row = table.find_elements(By.XPATH, './/tbody/tr[3]/td')
+    type_of_row = find_elems(table, by=By.XPATH, value='.//tbody/tr[3]/td')
     self.assertEqual(type_of_row[0].text, 'typeOf')
     self.assertEqual(type_of_row[1].text, 'StatisticalVariable')
     self.assertEqual(type_of_row[2].text, 'datacommons.org')
 
     # Assert observation charts loaded.
-    element_present = EC.presence_of_element_located(
-        (By.CLASS_NAME, 'observation-chart'))
-    WebDriverWait(self.driver, self.TIMEOUT_SEC).until(element_present)
-    observations_section = self.driver.find_element(
-        By.XPATH, '//*[@id="node-content"]/div[2]')
-    observations = observations_section.find_elements(By.CLASS_NAME, 'card')
-    self.assertTrue(len(observations) > 0)
+    observations_section = find_elem(self.driver,
+                                     by=By.XPATH,
+                                     value='//*[@id="node-content"]/div[2]')
+    self.assertGreater(len(find_elems(observations_section, value='card')), 0)
 
   def test_page_serve_austrobaileya(self):
     """Test the browser page for Austrobaileya scandens can be loaded successfully."""

server/webdriver/tests/browser_test.py

@@ -21,6 +21,7 @@
 from selenium.webdriver.support.ui import WebDriverWait
 
 from server.webdriver.base_dc_webdriver import BaseDcWebdriverTest
+from server.webdriver.base_utils import find_elem
 import server.webdriver.shared as shared
 from server.webdriver.shared_tests.browser_test import BrowserTestMixin
 
@@ -107,22 +108,23 @@ def test_observation_table_redirect(self):
     self.driver.get(self.url_ + CA_POPULATION_URL)
 
     # Wait for observation charts to be loaded.
-    element_present = EC.presence_of_element_located(
-        (By.CLASS_NAME, 'observation-chart'))
-    WebDriverWait(self.driver, self.TIMEOUT_SEC).until(element_present)
-    observations_section = self.driver.find_element(
-        By.XPATH, '//*[@id="node-content"]/div[2]')
+    self.assertIsNotNone(find_elem(self.driver, value='observation-chart'))
+    observations_section = find_elem(self.driver,
+                                     by=By.XPATH,
+                                     value='//*[@id="node-content"]/div[2]')
 
     # Switch to table view for the first chart
-    observation_section_chart_1 = observations_section.find_elements(
-        By.CLASS_NAME, 'card')[0]
-    table_view_button = observation_section_chart_1.find_element(
-        By.TAG_NAME, 'button')
+    observation_section_chart_1 = find_elem(observations_section, value='card')
+    table_view_button = find_elem(observation_section_chart_1,
+                                  by=By.TAG_NAME,
+                                  value='button')
     table_view_button.click()
 
     # Click the first row in the table view to open the browser page for that observation
-    table = observation_section_chart_1.find_element(By.TAG_NAME, 'table')
-    first_row = table.find_element(By.XPATH, './/tbody/tr[2]/td')
+    table = find_elem(observation_section_chart_1,
+                      by=By.TAG_NAME,
+                      value='table')
+    first_row = find_elem(table, by=By.XPATH, value='.//tbody/tr[2]/td')
     first_row.click()
 
     # Wait for the new page to open in a new tab
@@ -140,32 +142,33 @@ def test_observation_table_redirect(self):
     self.assertEqual(new_page_title, self.driver.title)
 
     # Assert header of the new page is correct.
-    element_present = EC.presence_of_element_located((By.TAG_NAME, 'h1'))
-    WebDriverWait(self.driver, self.TIMEOUT_SEC).until(element_present)
-    about_title = self.driver.find_element(By.XPATH, '//*[@id="node"]/h1')
-    self.assertEqual(about_title.text, 'About: dc/o/y54f4zvqrzf67')
-    dcid_subtitle = self.driver.find_element(By.XPATH, '//*[@id="node"]/h2[1]')
-    self.assertEqual(dcid_subtitle.text, 'dcid: dc/o/y54f4zvqrzf67')
-    typeOf_subtitle = self.driver.find_element(By.XPATH,
-                                               '//*[@id="node"]/h2[2]')
-    self.assertEqual(typeOf_subtitle.text, 'typeOf: StatVarObservation')
+    node = find_elem(self.driver, by=By.XPATH, value='//*[@id="node"]')
+    self.assertEqual(
+        find_elem(node, by=By.XPATH, value='.//h1').text,
+        'About: dc/o/y54f4zvqrzf67')  # about title.
+    self.assertEqual(
+        find_elem(node, by=By.XPATH, value='.//h2[1]').text,
+        'dcid: dc/o/y54f4zvqrzf67')  # dcid subtitle
+    self.assertEqual(
+        find_elem(node, by=By.XPATH, value='.//h2[2]').text,
+        'typeOf: StatVarObservation')  # typeOf_subtitle
 
   def test_observation_chart_redirect(self):
     """Test that the observation chart observation node links can redirect properly"""
     # Load California population browser page.
     self.driver.get(self.url_ + CA_POPULATION_URL)
-    element_present = EC.presence_of_element_located(
-        (By.XPATH, '//*[@id="node-content"]/div[1]/div/table'))
-    WebDriverWait(self.driver, self.TIMEOUT_SEC).until(element_present)
+    self.assertIsNotNone(
+        find_elem(self.driver,
+                  by=By.XPATH,
+                  value='//*[@id="node-content"]/div[1]/div/table'))
 
     # Click the point on the chart for the year 1850
-    element_present = EC.presence_of_element_located((By.XPATH, (
-        '//*[@id="node-content"]/div[2]/div/div[1]/div[2]/div/div[2]/div/*[name()="svg"]/'
-        + '*[name()="g"][4]/*[name()="g"]/*[name()="circle"][1]')))
-    WebDriverWait(self.driver, self.TIMEOUT_SEC).until(element_present)
-    point = self.driver.find_element(By.XPATH, (
+    point = find_elem(
+        self.driver,
+        by=By.XPATH,
+        value=
         '//*[@id="node-content"]/div[2]/div/div[1]/div[2]/div/div[2]/div/*[name()="svg"]/'
-        + '*[name()="g"][4]/*[name()="g"]/*[name()="circle"][1]'))
+        + '*[name()="g"][4]/*[name()="g"]/*[name()="circle"][1]')
     point.click()
 
     # Wait for the new page to open in a new tab
@@ -183,12 +186,13 @@ def test_observation_chart_redirect(self):
     self.assertEqual(new_page_title, self.driver.title)
 
     # Assert header of the new page is correct.
-    element_present = EC.presence_of_element_located((By.TAG_NAME, 'h1'))
-    WebDriverWait(self.driver, self.TIMEOUT_SEC).until(element_present)
-    about_title = self.driver.find_element(By.XPATH, '//*[@id="node"]/h1')
-    self.assertEqual(about_title.text, 'About: dc/o/y54f4zvqrzf67')
-    dcid_subtitle = self.driver.find_element(By.XPATH, '//*[@id="node"]/h2[1]')
-    self.assertEqual(dcid_subtitle.text, 'dcid: dc/o/y54f4zvqrzf67')
-    typeOf_subtitle = self.driver.find_element(By.XPATH,
-                                               '//*[@id="node"]/h2[2]')
-    self.assertEqual(typeOf_subtitle.text, 'typeOf: StatVarObservation')
+    node = find_elem(self.driver, by=By.XPATH, value='//*[@id="node"]')
+    self.assertEqual(
+        find_elem(node, by=By.XPATH, value='.//h1').text,
+        'About: dc/o/y54f4zvqrzf67')  # about title.
+    self.assertEqual(
+        find_elem(node, by=By.XPATH, value='.//h2[1]').text,
+        'dcid: dc/o/y54f4zvqrzf67')  # dcid subtitle
+    self.assertEqual(
+        find_elem(node, by=By.XPATH, value='.//h2[2]').text,
+        'typeOf: StatVarObservation')  # typeOf_subtitle

server/webdriver/tests/disaster_page_test.py

@@ -19,6 +19,9 @@
 from selenium.webdriver.support.ui import WebDriverWait
 
 from server.webdriver.base import WebdriverBaseTest
+from server.webdriver.base_utils import find_elem
+from server.webdriver.base_utils import find_elems
+from server.webdriver.base_utils import wait_elem
 
 
 class TestCharts(WebdriverBaseTest):
@@ -45,26 +48,23 @@ def test_server_and_page(self):
         EC.title_contains('Disasters Dashboard'))
 
     # Wait until the group of charts has loaded.
-    element_present = EC.presence_of_element_located(
-        (By.ID, 'subject-page-main-pane'))
-    WebDriverWait(self.driver, self.TIMEOUT_SEC).until(element_present)
+    self.assertIsNotNone(
+        wait_elem(self.driver, by=By.ID, value='subject-page-main-pane'))
 
     # Store a list of all the charts.
-    event_maps = self.driver.find_elements(By.CLASS_NAME,
-                                           'disaster-event-map-tile')
-    # Assert there are 5+ maps.
+    event_maps = find_elems(self.driver, value='disaster-event-map-tile')
     self.assertGreater(len(event_maps), 5)
 
     # Wait until the svg loads in the first article.
-    WebDriverWait(self.driver, self.TIMEOUT_SEC).until(
-        lambda d: event_maps[0].find_element(By.TAG_NAME, 'svg'))
+    self.assertIsNotNone(find_elem(event_maps[0], by=By.TAG_NAME, value='svg'))
 
     # Assert first article has svg with map geo region.
-    map_geo_region = event_maps[0].find_element(By.ID, 'map-geo-regions')
-    path = map_geo_region.find_elements(By.TAG_NAME, 'path')
+    map_geo_region = find_elem(event_maps[0], by=By.ID, value='map-geo-regions')
+    self.assertIsNotNone(map_geo_region)
+
+    path = find_elems(map_geo_region, by=By.TAG_NAME, value='path')
     self.assertEqual(len(path), 1)
 
-    # Assert first article has svg with points.
-    map_points_layers = event_maps[0].find_elements(By.CLASS_NAME,
-                                                    'map-points-layer')
-    self.assertGreater(len(map_points_layers), 2)
+    # Assert first article has svg with at least 2 points.
+    self.assertGreater(len(find_elems(event_maps[0], value='map-points-layer')),
+                       2)