From 4edeaeacb2594f0ff20147549cff5c119d10d4a3 Mon Sep 17 00:00:00 2001
From: bloodearnest <simon.davy@thedatalab.org>
Date: Mon, 21 Oct 2024 17:49:30 +0100
Subject: [PATCH] Test and fix the open_browser error handling

This wasn't covered before, and had some issues. This adds coverate for
timeout and error scenarios, and fixes an error found by these tests.
---
 opensafely/utils.py   |  3 ++-
 tests/test_rstudio.py |  2 +-
 tests/test_utils.py   | 41 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/opensafely/utils.py b/opensafely/utils.py
index 46b1284..2bae8fd 100644
--- a/opensafely/utils.py
+++ b/opensafely/utils.py
@@ -176,11 +176,12 @@ def open_browser(url, timeout=60.0):
         # wait for port to be open
         debug("open_browser: waiting for port")
         start = time.time()
+        response = None
         while time.time() - start < timeout:
             try:
                 response = requests.get(url, timeout=1)
             except Exception:
-                pass
+                time.sleep(0.5)
             else:
                 break
 
diff --git a/tests/test_rstudio.py b/tests/test_rstudio.py
index 77c7149..c2de83b 100644
--- a/tests/test_rstudio.py
+++ b/tests/test_rstudio.py
@@ -19,7 +19,7 @@ def test_rstudio(run, tmp_path, monkeypatch, gitconfig_exists):
     # windows
     monkeypatch.setitem(os.environ, "USERPROFILE", str(home))
 
-    # mock the webbrowser.open call
+    # mock the open_browser call
     mock_open_browser = mock.Mock(spec=utils.open_browser)
     monkeypatch.setattr(utils, "open_browser", mock_open_browser)
 
diff --git a/tests/test_utils.py b/tests/test_utils.py
index 4c3ee9d..1b639fc 100644
--- a/tests/test_utils.py
+++ b/tests/test_utils.py
@@ -125,3 +125,44 @@ def test_run_docker_interactive_tty(run, no_user, monkeypatch):
         ],
     )
     utils.run_docker([], "ehrql:v1", [], interactive=True)
+
+
+def test_open_browser_timeout(monkeypatch, capsys):
+    monkeypatch.setitem(os.environ, "DEBUG", "TRUE")
+
+    mock_open = mock.Mock(spec=utils.webbrowser.open)
+    monkeypatch.setattr(utils.webbrowser, "open", mock_open)
+
+    port = utils.get_free_port()
+    url = f"http://localhost:{port}/"
+    utils.open_browser(url, timeout=0.01)
+
+    assert not mock_open.called
+    _, err = capsys.readouterr()
+    assert f"Could not connect to {url}" in err
+
+
+def test_open_browser_error(monkeypatch, capsys):
+    # turn on debug logging
+    monkeypatch.setitem(os.environ, "DEBUG", "TRUE")
+
+    # return successful response from poll
+    mock_get = mock.Mock(spec=utils.requests.get)
+    response = utils.requests.Response()
+    response.status_code = 200
+    mock_get.return_value = response
+    monkeypatch.setattr(utils.requests, "get", mock_get)
+
+    # raise exception when calling webbrowser.open
+    mock_open = mock.Mock(spec=utils.webbrowser.open)
+    mock_open.side_effect = Exception("TEST ERROR")
+    monkeypatch.setattr(utils.webbrowser, "open", mock_open)
+
+    port = utils.get_free_port()
+    url = f"http://localhost:{port}/"
+    utils.open_browser(url, timeout=0.01)
+
+    mock_open.assert_called_with(url, new=2)
+    out, err = capsys.readouterr()
+    assert out == ""
+    assert "TEST ERROR" in err