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

@@ -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
 

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)
 

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