Merge pull request #81 from ethanstoner/fix/task-clean-state

keep the rewards tab when a task fails, and put it back on the rewards page

src/rewards_tasks.py

@@ -18,6 +18,8 @@ import element_selectors
 
 VISUAL_SEARCH_IMAGE_PATH = os.path.abspath("visual_search.jpg")
 
+REWARDS_HOME_URL = "https://rewards.bing.com/"
+
 logger = logging.getLogger(__name__)
 
 
@@ -76,11 +78,16 @@ class RewardsTaskUtils:
 
 		self.driver.execute_cdp_cmd("Network.setExtraHTTPHeaders", {"headers": headers})
 
-		self.driver.get("https://rewards.bing.com/")
+		self.driver.get(REWARDS_HOME_URL)
 
 		self.tab_utils = tab_utils.TabUtils(driver)
 		self.tab_utils.ensure_focus()
 
+		# The tab the tasks work in. Recorded rather than looked up later,
+		# because "the current tab" stops meaning this one the moment a task
+		# opens a card in a new one.
+		self.main_window = driver.current_window_handle
+
 		self.mouse = mouse_trajectory.MouseUtils(driver)
 		self.keyboard = mimic_typing.KeyboardUtils(driver)
 		self.elements = element_selectors.ElementSelectionUtils(driver)
@@ -396,9 +403,57 @@ class RewardsTaskUtils:
 				)
 				self.wait_for_then_click(self.elements.get_clear_bing_search_query_button)
 
-		self.driver.get("https://rewards.bing.com/")
+		self.driver.get(REWARDS_HOME_URL)
 		self.tab_utils.ensure_focus()
 
+	def restore_main_tab(self):
+		"""Close the stray tabs, keeping the one the tasks work in.
+
+		close_all_other_tabs with no arguments keeps whatever tab is focused
+		right now. After a task that died on a Bing tab that is the Bing tab, so
+		the cleanup closed the Rewards tab and kept the search results. Naming
+		the tab to keep is the difference between tidying up and destroying the
+		only tab the next task can use.
+
+		If the main tab is gone, whatever is left is better than nothing: the
+		page fix below still has to run either way.
+		"""
+		try:
+			handles = self.driver.window_handles
+
+			if not handles:
+				return
+
+			keep = self.main_window if self.main_window in handles else handles[0]
+
+			self.tab_utils.close_all_other_tabs(exceptions=[keep])
+		except Exception as exc:
+			logger.warning(
+				"Could not tidy the open tabs: %s", log_utils.exception_summary(exc)
+			)
+
+	def return_to_rewards_home(self):
+		"""Put the browser back on the Rewards home page.
+
+		Only called when a task did not finish. Navigating after every task
+		would reload the page six times a run for no reason, and the tasks that
+		succeed already leave the browser somewhere their successor can work
+		from.
+		"""
+		try:
+			if self.driver.current_url.startswith(REWARDS_HOME_URL):
+				return
+
+			self.driver.get(REWARDS_HOME_URL)
+			self.tab_utils.ensure_focus()
+		except Exception as exc:
+			# Recovery is best effort. If even this fails the next task will
+			# report its own [SKIP], which is no worse than before.
+			logger.warning(
+				"Could not return to the Rewards home page: %s",
+				log_utils.exception_summary(exc)
+			)
+
 	def claim_bonus_points(self):
 		self.switch_to_dashboard()
 
@@ -426,9 +481,12 @@ class RewardsTaskUtils:
 			# The tags stay in the message rather than being folded into the
 			# level, they are the per-task outcome summary and reading a run
 			# means scanning for them.
+			completed = False
+
 			try:
 				step()
 				logger.info("[OK] %s", name)
+				completed = True
 			except Exception as exc:
 				tag, reason = task_failure_report(exc)
 
@@ -438,8 +496,10 @@ class RewardsTaskUtils:
 					exc_info=logger.isEnabledFor(logging.DEBUG)
 				)
 
-			# Leave a clean tab state behind for the next task.
+			# Leave a clean tab state behind for the next task. Both halves of
-			try:
+			# this matter, and they are separate failures: the right tab has to
-				self.tab_utils.close_all_other_tabs()
+			# survive, and it has to be showing the right page.
-			except Exception:
+			self.restore_main_tab()
-				pass
+
+			if not completed:
+				self.return_to_rewards_home()

tests/test_task_recovery.py

@@ -0,0 +1,274 @@
+"""Tests for getting back to a known page after a task fails.
+
+The bug (#63): a task that dies partway through leaves the browser wherever it
+stopped, which for the search and visual search tasks is a Bing page rather than
+the Rewards site. Every later task begins by looking for controls that only
+exist on rewards.bing.com, so one failure made all of them report [SKIP] for a
+UI that was present and working the whole time.
+
+The stand-ins here are local rather than in fakes.py: that module fakes the DOM
+lookups the selectors do, and what these need is a driver that remembers where
+it navigated. No browser, so they run anywhere.
+
+	python -m unittest discover -s tests
+"""
+
+import logging
+import os
+import sys
+import unittest
+
+sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "src"))
+
+from selenium.common.exceptions import NoSuchElementException, TimeoutException, WebDriverException
+
+import rewards_tasks
+
+HOME = rewards_tasks.REWARDS_HOME_URL
+BING_RESULTS = "https://www.bing.com/search?q=weather"
+
+
+MAIN_TAB = "main-tab"
+BING_TAB = "bing-tab"
+
+
+class RecordingDriver:
+	def __init__(self, url=HOME, get_raises=None, handles=None, current_handle=MAIN_TAB):
+		self.current_url = url
+		self.visited = []
+		self.get_raises = get_raises
+		self.window_handles = list(handles) if handles is not None else [MAIN_TAB]
+		self.current_window_handle = current_handle
+
+	def get(self, url):
+		if self.get_raises is not None:
+			raise self.get_raises
+
+		self.visited.append(url)
+		self.current_url = url
+
+
+class RecordingTabUtils:
+	def __init__(self):
+		self.closes = 0
+		self.focuses = 0
+		self.kept = []
+
+	def close_all_other_tabs(self, exceptions=None):
+		self.closes += 1
+		self.kept.append(exceptions)
+
+	def ensure_focus(self):
+		self.focuses += 1
+
+
+class StubTasks(rewards_tasks.RewardsTaskUtils):
+	"""RewardsTaskUtils with the six tasks replaced, and nothing else stubbed.
+
+	complete_all_tasks and return_to_rewards_home are the real ones, which is
+	the whole point.
+	"""
+
+	def __init__(self, driver, failures=None):
+		self.driver = driver
+		self.tab_utils = RecordingTabUtils()
+		self.main_window = MAIN_TAB
+		self.failures = failures or {}
+		self.ran = []
+
+	def _task(self, name, strands_browser_at=None):
+		self.ran.append(name)
+
+		if name not in self.failures:
+			# A task that finishes switches back to the Rewards tab itself, so
+			# only a failure part way through leaves the browser elsewhere.
+			return
+
+		if strands_browser_at is not None:
+			self.driver.current_url = strands_browser_at
+
+		raise self.failures[name]
+
+	def complete_bing_daily_set(self):
+		self._task("Bing daily set")
+
+	def complete_explore_on_bing_tasks(self):
+		self._task("Explore on Bing", strands_browser_at=BING_RESULTS)
+
+	def complete_visual_search(self):
+		self._task("Visual search", strands_browser_at=BING_RESULTS)
+
+	def complete_misc_cards(self):
+		self._task("Misc cards")
+
+	def complete_required_searches(self):
+		self._task("Required searches")
+
+	def claim_bonus_points(self):
+		self._task("Bonus points")
+
+
+ALL_TASKS = [
+	"Bing daily set", "Explore on Bing", "Visual search",
+	"Misc cards", "Required searches", "Bonus points",
+]
+
+
+class FailedTaskRecoveryTests(unittest.TestCase):
+	def test_a_failure_on_a_bing_page_navigates_back(self):
+		driver = RecordingDriver()
+		tasks = StubTasks(driver, {"Visual search": TimeoutException("no file input")})
+
+		with self.assertLogs(rewards_tasks.logger, level=logging.WARNING):
+			tasks.complete_all_tasks()
+
+		self.assertEqual(driver.visited, [HOME])
+		self.assertEqual(driver.current_url, HOME)
+
+	def test_the_later_tasks_still_run(self):
+		# The point of the fix: one failure used to cost every task after it.
+		driver = RecordingDriver()
+		tasks = StubTasks(driver, {"Explore on Bing": TimeoutException("slow panel")})
+
+		with self.assertLogs(rewards_tasks.logger, level=logging.WARNING):
+			tasks.complete_all_tasks()
+
+		self.assertEqual(tasks.ran, ALL_TASKS)
+
+	def test_a_skip_recovers_too(self):
+		# NoSuchElementException is the [SKIP] path rather than [FAIL], and it
+		# strands the browser just the same.
+		driver = RecordingDriver()
+		tasks = StubTasks(driver, {"Visual search": NoSuchElementException("no sidebar")})
+
+		with self.assertLogs(rewards_tasks.logger, level=logging.WARNING):
+			tasks.complete_all_tasks()
+
+		self.assertEqual(driver.visited, [HOME])
+
+	def test_a_clean_run_never_navigates(self):
+		# Six extra page loads a run would be a real cost for no benefit.
+		driver = RecordingDriver()
+		tasks = StubTasks(driver)
+
+		tasks.complete_all_tasks()
+
+		self.assertEqual(driver.visited, [])
+		self.assertEqual(tasks.ran, ALL_TASKS)
+
+	def test_no_redundant_navigation_when_already_home(self):
+		# Plenty of failures happen on the Rewards page itself, e.g. a panel
+		# that never renders. Reloading it would only cost time.
+		driver = RecordingDriver()
+		tasks = StubTasks(driver, {"Misc cards": TimeoutException("no cards")})
+
+		with self.assertLogs(rewards_tasks.logger, level=logging.WARNING):
+			tasks.complete_all_tasks()
+
+		self.assertEqual(driver.visited, [])
+
+	def test_tabs_are_still_tidied_after_a_failure(self):
+		driver = RecordingDriver()
+		tasks = StubTasks(driver, {"Visual search": TimeoutException("no file input")})
+
+		with self.assertLogs(rewards_tasks.logger, level=logging.WARNING):
+			tasks.complete_all_tasks()
+
+		self.assertEqual(tasks.tab_utils.closes, len(ALL_TASKS))
+
+	def test_the_rewards_tab_is_the_one_kept(self):
+		# The actual bug: close_all_other_tabs() with no argument keeps whatever
+		# is focused, and after a task dies on a Bing tab that is the Bing tab,
+		# so the cleanup closed the Rewards tab and kept the search results.
+		driver = RecordingDriver(handles=[MAIN_TAB, BING_TAB], current_handle=BING_TAB)
+		tasks = StubTasks(driver, {"Visual search": TimeoutException("no file input")})
+
+		with self.assertLogs(rewards_tasks.logger, level=logging.WARNING):
+			tasks.complete_all_tasks()
+
+		self.assertTrue(all(kept == [MAIN_TAB] for kept in tasks.tab_utils.kept))
+
+
+class RestoreMainTabTests(unittest.TestCase):
+	def test_names_the_main_tab_rather_than_the_focused_one(self):
+		driver = RecordingDriver(handles=[MAIN_TAB, BING_TAB], current_handle=BING_TAB)
+		tasks = StubTasks(driver)
+
+		tasks.restore_main_tab()
+
+		self.assertEqual(tasks.tab_utils.kept, [[MAIN_TAB]])
+
+	def test_falls_back_when_the_main_tab_is_gone(self):
+		# A task can close it, and a stale handle would only raise.
+		driver = RecordingDriver(handles=[BING_TAB], current_handle=BING_TAB)
+		tasks = StubTasks(driver)
+
+		tasks.restore_main_tab()
+
+		self.assertEqual(tasks.tab_utils.kept, [[BING_TAB]])
+
+	def test_no_windows_left_is_not_an_error(self):
+		driver = RecordingDriver(handles=[], current_handle=MAIN_TAB)
+		tasks = StubTasks(driver)
+
+		tasks.restore_main_tab()
+
+		self.assertEqual(tasks.tab_utils.kept, [])
+
+	def test_a_failure_to_tidy_is_survivable(self):
+		driver = RecordingDriver(handles=[MAIN_TAB])
+		tasks = StubTasks(driver)
+
+		def boom(exceptions=None):
+			raise WebDriverException("browser gone")
+
+		tasks.tab_utils.close_all_other_tabs = boom
+
+		with self.assertLogs(rewards_tasks.logger, level=logging.WARNING) as logged:
+			tasks.restore_main_tab()
+
+		self.assertTrue(any("Could not tidy" in line for line in logged.output))
+
+
+class ReturnToRewardsHomeTests(unittest.TestCase):
+	def test_navigates_and_refocuses(self):
+		driver = RecordingDriver(url=BING_RESULTS)
+		tasks = StubTasks(driver)
+
+		tasks.return_to_rewards_home()
+
+		self.assertEqual(driver.visited, [HOME])
+		self.assertEqual(tasks.tab_utils.focuses, 1)
+
+	def test_already_home_is_left_alone(self):
+		driver = RecordingDriver(url=HOME + "?foo=1")
+		tasks = StubTasks(driver)
+
+		tasks.return_to_rewards_home()
+
+		self.assertEqual(driver.visited, [])
+		self.assertEqual(tasks.tab_utils.focuses, 0)
+
+	def test_a_driver_that_cannot_navigate_is_survivable(self):
+		# Recovery is best effort. Raising here would replace the real failure
+		# with the tidy-up's own, and take the rest of the run down with it.
+		driver = RecordingDriver(url=BING_RESULTS, get_raises=WebDriverException("browser gone"))
+		tasks = StubTasks(driver)
+
+		with self.assertLogs(rewards_tasks.logger, level=logging.WARNING) as logged:
+			tasks.return_to_rewards_home()
+
+		self.assertTrue(any("Could not return" in line for line in logged.output))
+
+	def test_a_failing_recovery_does_not_end_the_run(self):
+		driver = RecordingDriver(get_raises=WebDriverException("browser gone"))
+		tasks = StubTasks(driver, {"Explore on Bing": TimeoutException("slow panel")})
+
+		with self.assertLogs(rewards_tasks.logger, level=logging.WARNING):
+			tasks.complete_all_tasks()
+
+		self.assertEqual(tasks.ran, ALL_TASKS)
+
+
+if __name__ == "__main__":
+	unittest.main()