Fix open_browser tests

rtv/terminal.py

@@ -18,8 +18,6 @@ from contextlib import contextmanager
 from tempfile import NamedTemporaryFile
 
 import six
-#pylint: disable=import-error
-from six.moves.urllib.parse import quote
 from kitchen.text.display import textual_width_chop
 
 from . import exceptions
@@ -509,9 +507,10 @@ class Terminal(object):
                         raise exceptions.BrowserError(
                             'Timeout waiting for browser to open')
                 finally:
-                    # Can't check the loader exception because the oauth module
-                    # supersedes this loader and we need to always kill the
-                    # process if escape is pressed
+                    # This will be hit on the browser timeout, but also if the
+                    # user presses the ESC key. We always want to kill the
+                    # webbrowser process if it hasn't opened the tab and
+                    # terminated by now.
                     try:
                         p.terminate()
                     except OSError:

tests/test_terminal.py

@@ -10,7 +10,7 @@ import six
 import pytest
 
 from rtv.docs import HELP, COMMENT_EDIT_FILE
-from rtv.exceptions import TemporaryFileError
+from rtv.exceptions import TemporaryFileError, BrowserError
 
 try:
     from unittest import mock
@@ -464,22 +464,38 @@ def test_open_link_subprocess(terminal):
         assert get_error()
 
 
-def test_open_browser(terminal):
-
-    url = 'http://www.test.com'
+def test_open_browser_display(terminal):
 
     terminal._display = True
-    with mock.patch('subprocess.Popen', autospec=True) as Popen:
-        Popen.return_value.poll.return_value = 0
-        terminal.open_browser(url)
-    assert Popen.called
+    with mock.patch('webbrowser.open_new_tab', autospec=True) as open_new_tab:
+        terminal.open_browser('http://www.test.com')
+
+    # open_new_tab() will be executed in the child process so we can't
+    # directly check if the was called from here or not.
+    # open_new_tab.assert_called_with('http://www.test.com')
+
+    # Shouldn't suspend curses
     assert not curses.endwin.called
     assert not curses.doupdate.called
 
+
+def test_open_browser_display_no_response(terminal):
+
+    terminal._display = True
+    with mock.patch('rtv.terminal.Process', autospec=True) as Process:
+        Process.return_value.is_alive.return_value = 1
+        terminal.open_browser('http://www.test.com')
+    assert isinstance(terminal.loader.exception, BrowserError)
+
+
+def test_open_browser_no_display(terminal):
+
     terminal._display = False
     with mock.patch('webbrowser.open_new_tab', autospec=True) as open_new_tab:
-        terminal.open_browser(url)
-    open_new_tab.assert_called_with(url)
+        terminal.open_browser('http://www.test.com')
+    open_new_tab.assert_called_with('http://www.test.com')
+
+    # Should suspend curses to give control of the terminal to the browser
     assert curses.endwin.called
     assert curses.doupdate.called