diff --git a/extension/src/api.ts b/extension/src/api.ts index 99cea1f6..b464b0c3 100644 --- a/extension/src/api.ts +++ b/extension/src/api.ts @@ -19,8 +19,11 @@ type VisitsResponse = { // NOTE: boolean is legacy behaviour type VisitedBackendResponse = Array +type Endpoint = 'visits' | 'search' | 'search_around' | 'visited' + + // TODO ugh, why params: {} not working?? -export async function queryBackendCommon(params: any, endp: string): Promise { +export async function queryBackendCommon(params: any, endp: Endpoint): Promise { 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(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 diff --git a/extension/src/background.ts b/extension/src/background.ts index 99036a35..469a7887 100644 --- a/extension/src/background.ts +++ b/extension/src/background.ts @@ -38,7 +38,7 @@ function actions(): Array { // 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")) } }) } diff --git a/extension/src/display.ts b/extension/src/display.ts index 81e8151c..01f10bd0 100644 --- a/extension/src/display.ts +++ b/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( diff --git a/extension/src/notifications.ts b/extension/src/notifications.ts index 8469f096..5721b3d0 100644 --- a/extension/src/notifications.ts +++ b/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( 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") } diff --git a/extension/src/options_page.ts b/extension/src/options_page.ts index 7ae09e8e..710bf7e3 100644 --- a/extension/src/options_page.ts +++ b/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`)) }); })); diff --git a/tests/end2end_test.py b/tests/end2end_test.py index d84df359..5c6abdaf 100755 --- a/tests/end2end_test.py +++ b/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/"