extension: minor tweaks for error handling and add more tests

extension/src/api.ts

@@ -19,8 +19,11 @@ type VisitsResponse = {
 // NOTE: boolean is legacy behaviour
 type VisitedBackendResponse = Array<JsonObject | null | boolean>
 
+type Endpoint = 'visits' | 'search' | 'search_around' | 'visited'
+
+
 // TODO ugh, why params: {} not working??
-export async function queryBackendCommon<R>(params: any, endp: string): Promise<R | Error> {
+export async function queryBackendCommon<R>(params: any, endp: Endpoint): Promise<R | Error> {
     const opts = await getOptions()
     if (opts.host == '') { // use 'dummy' backend
         // the user only wants to use browser visits?
@@ -49,36 +52,31 @@ export async function queryBackendCommon<R>(params: any, endp: string): Promise<
     const endpoint = `${opts.host}/${endp}`
     params['client_version'] = browser.runtime.getManifest().version
 
-    function with_stack(e: Error): Error {
-        const stack = []
-        if (e.stack) {
-            stack.push(e.stack)
-        }
-        stack.push(`while requesting ${endpoint}`)
-        stack.push("check extension options, make sure you set backend or disable it (set to empty string)")
-        e.stack = stack.join('\n')
-        console.error(e, e.stack) // hopefully worth loging, meant to happen rarely
-        return e
+    const extra_error = "Check if backend is running, or disable it in extension preferences."
+
+    let response: Response
+    try {
+        response = await fetch(
+            endpoint,
+            {
+                method: 'POST', // todo use GET?
+                headers: {
+                    'Content-Type' : 'application/json',
+                    'Authorization': "Basic " + btoa(opts.token),
+                },
+                body: JSON.stringify(params)
+            },
+        )
+    } catch (err) {
+        console.error(err)
+        throw new Error(`${endpoint}: unavailable (${(err as Error).toString()}).\n${extra_error}`)
     }
 
-    // TODO cors mode?
-    return fetch(endpoint, {
-        method: 'POST', // todo use GET?
-        headers: {
-            'Content-Type' : 'application/json',
-            'Authorization': "Basic " + btoa(opts.token),
-        },
-        body: JSON.stringify(params)
-    }).then(response => {
-        // right, fetch API doesn't reject on HTTP error status...
-        const ok = response.ok
-        if (!ok) {
-            return with_stack(new Error(`${response.statusText} (${response.status}`))
-        }
-        return response.json()
-    }).catch((err: Error) => {
-        return with_stack(err)
-    })
+    // note: fetch API doesn't reject on HTTP error
+    if (!response.ok) {  // http error
+        throw new Error(`${endpoint}: HTTP error ${response.status} ${response.statusText}.\n${extra_error}`)
+    }
+    return response.json()
 }
 
 // eslint-disable-next-line no-unused-vars

extension/src/background.ts

@@ -38,7 +38,7 @@ function actions(): Array<Action> {
         // see https://developer.chrome.com/docs/extensions/mv2/background_pages/#listeners
         isMobile().then(mobile => {
             if (mobile) {
-                notifyError("Expected pageAction to be present!")
+                notifyError(new Error("Expected pageAction to be present!"))
             }
         })
     }
@@ -928,7 +928,7 @@ function hasContextMenus(): boolean {
     if (browser.contextMenus == undefined) {
         isMobile().then(mobile => {
             if (!mobile) {
-                notifyError("error: chrome.contextMenus should be available")
+                notifyError(new Error("chrome.contextMenus should be available"))
             }
         })
         return false
@@ -1026,7 +1026,7 @@ function initBackground(): void {
     } else {
         isMobile().then(mobile => {
             if (!mobile) {
-                notifyError("error: chrome.commands should be available")
+                notifyError(new Error("chrome.commands should be available"))
             }
         })
     }

extension/src/display.ts

@@ -82,7 +82,8 @@ export class Binder {
         const item = child(parent, 'li', ['error'])
         const ec   = child(item  , 'code')
         // todo not sure if need any other info?
-        tchild(ec, `${error.name}: ${error.message}\n${error.stack}`)
+        // sigh.. in chrome stack includes name and message already.. but not in firefox
+        tchild(ec, `${error}\n${error.stack}`)
     }
 
     async render(

extension/src/notifications.ts

@@ -18,27 +18,22 @@ export function desktopNotify(message: string, priority: number=0) {
     });
 }
 
-function errmsg(obj: any): string {
-    let msg = null;
-    if (obj instanceof XMLHttpRequest) {
-        msg = `while requesting ${obj.responseURL}`;
-    } else {
-        msg = obj;
-    }
-    return `ERROR: ${msg}`;
+
+function error2string(err: Error): string {
+    // sigh.. in chrome stack includes name and message already.. but not in firefox
+    return `ERROR: ${err}\n${err.stack}`
 }
 
-export function notifyError(obj: any, context: string='') {
-    const message = errmsg(obj);
-    console.error('%o, context=%s', obj, context);
-    desktopNotify(message)
+
+export function notifyError(err: Error) {
+    console.error(err)
+    desktopNotify(error2string(err))
 }
 
 
-export function alertError(obj: any) {
-    const message = errmsg(obj);
-    console.error('%o', obj);
-    alert(message);
+export function alertError(err: Error) {
+    console.error(err)
+    alert(error2string(err))
 }
 
 
@@ -52,7 +47,7 @@ export function defensify<Args extends DefensifyArgs>(
         console.error('function "%s" %s failed: %o', fname, name, err)
         getOptions().then((opts: Options) => {
             if (opts.verbose_errors_on) {
-                notifyError(err, 'defensify')
+                notifyError(err)
             } else {
                 console.warn("error notification is suppressed by 'verbose_errors' option")
             }

extension/src/options_page.ts

@@ -279,7 +279,7 @@ document.getElementById('backend_status_id')!.addEventListener('click', defensif
         // TODO ugh. need to reject if ok is false...
         const resj = await res.json()
         alert(`Success! ${JSON.stringify(resj)}`)
-    }, err => {
-        alertError(`${err}. See https://github.com/karlicoss/promnesia/blob/master/doc/TROUBLESHOOTING.org`);
+    }, (err: Error) => {
+        alertError(new Error(`${err}\n${err.stack}\n\nSee https://github.com/karlicoss/promnesia/blob/master/doc/TROUBLESHOOTING.org`))
     });
 }));

tests/end2end_test.py

@@ -6,17 +6,12 @@
 from datetime import datetime
 import os
 from pathlib import Path
+import socket
 from time import sleep
 from typing import Iterator, TypeVar, Callable
 
 import pytest
 
-if __name__ == '__main__':
-    # TODO ugh need to figure out PATH
-    # python3 -m pytest -s tests/server_test.py::test_query
-    pytest.main(['-s', __file__])
-
-
 from selenium.webdriver import Remote as Driver
 from selenium.webdriver.common.keys import Keys
 from selenium.webdriver.common.by import By
@@ -156,8 +151,8 @@ def driver(tmp_path: Path, browser: Browser) -> Iterator[Driver]:
 
 @dataclass
 class Backend:
-    # directory with database and configs
-    backend_dir: Path
+    backend_dir: Path  # directory with database and configs
+    port: str
 
 
 @pytest.fixture
@@ -168,7 +163,7 @@ def backend(tmp_path: Path, addon: Addon) -> Iterator[Backend]:
         # this bit (up to yield) takes about 1.5s -- I guess it's the 1s sleep in configure_extension
         addon.configure(host=LOCALHOST, port=srv.port)
         addon.helper.driver.get('about:blank')  # not sure if necessary
-        yield Backend(backend_dir=backend_dir)
+        yield Backend(backend_dir=backend_dir, port=srv.port)
 
 
 @browsers()
@@ -244,6 +239,33 @@ def test_sidebar_position(addon: Addon, driver: Driver) -> None:
     confirm("sidebar: should be displayed below")
 
 
+@browsers()
+def test_bad_port(addon: Addon, driver: Driver) -> None:
+    """
+    Check that we get error notification instead of silently failing if the endpoint is wrong
+    """
+    addon.configure(host=LOCALHOST, port='12346')
+
+    driver.get('https://example.com')
+
+    sidebar = addon.sidebar
+
+    sidebar.open()
+
+    # TODO would be nice to check that regular visits also work..
+    with sidebar.ctx():
+        visits = sidebar.visits
+
+        [err] = visits
+
+        assert err.get_attribute('class') == 'error'
+        assert 'http://localhost:12346/visits: unavailable' in err.text
+
+    # TODO check 'mark visited' too?
+    # although at the moment due to error handling in sources.visited it's not really showing error
+    # if there is a single visit in the results
+
+
 @browsers()
 def test_blacklist_custom(addon: Addon, driver: Driver) -> None:
     addon.configure(port='12345', blacklist=('stackoverflow.com',))
@@ -305,6 +327,11 @@ def test_add_to_blacklist_context_menu(addon: Addon, driver: Driver) -> None:
 # todo might be nice to run soft asserts for this test?
 @browsers()
 def test_visits(addon: Addon, driver: Driver, backend: Backend) -> None:
+    # TODO separate this out into a parameterized test?
+    # also check that custom hostname works too
+    hostname = socket.gethostname()  # typically returns proper hostname instead of 'localhost'
+    addon.configure(host=f'http://{hostname}', port=backend.port)
+
     from promnesia.tests.sources.test_hypothesis import index_hypothesis
 
     test_url = "http://www.e-flux.com/journal/53/59883/the-black-stack/"