From e748e2d08886091a2fab63e746af8e469f09d2fe Mon Sep 17 00:00:00 2001 From: Fabrice Meyer Date: Sun, 6 Sep 2026 11:52:41 -0300 Subject: [PATCH 01/17] Bring the test rig into the repository The tests that found the last round of bugs lived on my laptop, which is the wrong place for them: every one of them exists because something looked right on the wire and was wrong on the screen - changes buffered forever behind a missing flag, a document written to a file nobody reads, a session ended under an editor that was still open - and none of that is visible to a test that asserts on status codes. So they come in as they are: a disposable Nextcloud with this working copy mounted live, and headless browsers driven over CDP that ask the editor's own document model what it displays and read the saved file to see what was kept. What changed on the way in: * provision.sh builds the instance from nothing - the connector at a pinned version, a second user to co-author with, and a document of each format uploaded and shared with them. The documents are the blank templates the bundled server ships for "new file", so there are no binary fixtures here and the fixtures are by definition ones this document server can open. What it creates goes to .rig-env, which the tests read, so no instance's file ids are hardcoded in a test. * It also points the connector at this app explicitly. Left to auto-configuration from occ, the url comes from overwrite.cli.url, which is not necessarily the host the browser uses - and an editor iframe on a different origin than the page cannot be framed by it, nor read by a test. * The parts every test needs - the CDP driver, and occ/database/appdata/saved file access - are lib/driver.py and lib/harness.py instead of one test importing another. * rig.sh is the single entry point, so CI and a laptop run the same commands. smoke_test.py is new: every format opens with no JS exception or failed request, takes an edit, and that edit is in the file after a flush. It is the test that would have caught most of what broke while this app was brought up to a current document server, none of which needed anything as specific as the tests that came after it. Signed-off-by: Fabrice Meyer Co-Authored-By: Claude Opus 5 (1M context) --- tests/rig/.gitignore | 2 + tests/rig/README.md | 76 ++++++++ tests/rig/autosave_test.py | 161 +++++++++++++++++ tests/rig/chat_test.py | 158 +++++++++++++++++ tests/rig/coedit_test.py | 111 ++++++++++++ tests/rig/docker-compose.yml | 51 ++++++ tests/rig/flush_live_test.py | 127 ++++++++++++++ tests/rig/leave_test.py | 142 +++++++++++++++ tests/rig/lib/config.py | 82 +++++++++ tests/rig/lib/driver.py | 331 +++++++++++++++++++++++++++++++++++ tests/rig/lib/harness.py | 162 +++++++++++++++++ tests/rig/probe.py | 55 ++++++ tests/rig/provision.sh | 119 +++++++++++++ tests/rig/rig.sh | 81 +++++++++ tests/rig/run_all.py | 85 +++++++++ tests/rig/smoke_test.py | 159 +++++++++++++++++ tests/rig/twotab_test.py | 117 +++++++++++++ 17 files changed, 2019 insertions(+) create mode 100644 tests/rig/.gitignore create mode 100644 tests/rig/README.md create mode 100755 tests/rig/autosave_test.py create mode 100755 tests/rig/chat_test.py create mode 100755 tests/rig/coedit_test.py create mode 100644 tests/rig/docker-compose.yml create mode 100755 tests/rig/flush_live_test.py create mode 100755 tests/rig/leave_test.py create mode 100644 tests/rig/lib/config.py create mode 100644 tests/rig/lib/driver.py create mode 100644 tests/rig/lib/harness.py create mode 100755 tests/rig/probe.py create mode 100755 tests/rig/provision.sh create mode 100755 tests/rig/rig.sh create mode 100755 tests/rig/run_all.py create mode 100755 tests/rig/smoke_test.py create mode 100755 tests/rig/twotab_test.py diff --git a/tests/rig/.gitignore b/tests/rig/.gitignore new file mode 100644 index 0000000..46718ce --- /dev/null +++ b/tests/rig/.gitignore @@ -0,0 +1,2 @@ +.rig-env +__pycache__/ diff --git a/tests/rig/README.md b/tests/rig/README.md new file mode 100644 index 0000000..fdda1b6 --- /dev/null +++ b/tests/rig/README.md @@ -0,0 +1,76 @@ +# The test rig + +A disposable Nextcloud with this app mounted live, and a set of regression tests +that drive real editors in headless chromium. + +They drive editors because that is where the bugs were. This app is one half of +a conversation with sdkjs: what matters is not whether a message was sent but +what the other end does with it, and every one of the tests here exists because +something looked right on the wire and was wrong on the screen — changes +buffered forever behind a missing flag, a document written to a file nobody +reads, a session ended under an editor that was still open. So each test asks +the editor's own document model what it displays, or reads the saved file, and +never settles for "the request returned 200". + +## Running it + +Needs docker, python3 with `websockets` and `requests`, chromium, and the +document server tree in place: + + make # in the app directory, once: downloads the document server + pip install websockets requests + + cd tests/rig + ./rig.sh up # containers, and wait for Nextcloud + ./rig.sh provision # connector, users, one document per format + ./rig.sh test # the whole suite + ./rig.sh test autosave # or just one, see ./run_all.py --list + ./rig.sh logs # this app's lines from nextcloud.log + ./rig.sh down # throw it all away + +Knobs, all environment variables: `RIG_PROJECT` (compose project, so a second +rig can run alongside), `RIG_PORT`, `RIG_NC_IMAGE`, `RIG_CONNECTOR_VERSION`, +`APP_SOURCE` (which working copy to mount). `provision.sh` writes what it +created — users, documents, file ids — to `.rig-env`, which the tests read, so +nothing about a particular instance is hardcoded in a test. + +## What each test covers + +| test | covers | +| --- | --- | +| `smoke` | every format opens with no JS exception or failed request, takes an edit, and that edit is in the file after a flush. Catches the whole class of "the editor does not come up": an unrendered `api.js`, stylesheets killed by the CSP nonce, fonts the converter cannot find, appdata the file cache never heard about | +| `flush-live` | flushing a document somebody is still editing writes the file and leaves the change list and the document folder alone. The change list is the only record of what was typed — `Editor.bin` stays at the version the document was opened at — so consuming it mid-session strands the document | +| `autosave` | edits reach the file while the document is open, with no cron and no flush; a save inside the interval does not re-assemble the document; `autosave_interval 0` turns it off | +| `leave` | closing the editor and closing the tab both write the document out and dispose of it, one participant leaving does not end the document for the other, and reopening shows all of it | +| `twotab` | two tabs of *one* browser — which share a cookie jar, a Nextcloud session and a connection pool, as two separate browsers do not — see each other type; and one of them dropped to view mode (`asc_coAuthoringDisconnect`, which is what sdkjs does on a licence verdict or a rights change) does not take the document away from the other | +| `coedit` | two users in one document each see what the other types, in a text document and in a spreadsheet, with one socket session per browser — a second one means the transport reconnected mid-edit | +| `chat` | the chat panel carries messages both ways, and a late joiner reads the backlog | + +## Things that will mislead you + +- **The sample documents keep what earlier runs typed into them.** Every test + suffixes its markers with a per-run number for that reason. A test that looks + for a fixed marker will pass on a leftover from an hour ago. +- **A rig with background jobs in AJAX mode runs them whenever a browser makes a + request**, and the Cleanup job writes documents that are being edited. Any + assertion that a file did *not* change has to allow for that; `autosave_test` + checks whether the job ran in the window it is measuring. +- **The `wss://…transport=websocket` 400 in the console is deliberate.** PHP + cannot upgrade, and the 400 is what makes the client fall back to long + polling. +- An app config value written with `occ` takes a few seconds to be visible to + web requests, which is what `harness.APP_CONFIG_PROPAGATION` waits out. +- Deleting appdata behind Nextcloud's back leaves stale file cache rows and the + next conversion fails with `Could not create path .../Editor.bin`; + `harness.reset()` follows it with `files:scan-app-data` for that reason. + +## Layout + + rig.sh up / provision / reset / test / logs / down + provision.sh connector, second user, a document per format, .rig-env + docker-compose.yml Nextcloud + MariaDB, this working copy mounted as an app + run_all.py the suite runner, and what each test covers + lib/config.py where the rig is and what is in it + lib/harness.py occ, the database, the appdata, the saved files + lib/driver.py a headless chromium per session, over CDP + probe.py poke at one live editor by hand, for when a test fails diff --git a/tests/rig/autosave_test.py b/tests/rig/autosave_test.py new file mode 100755 index 0000000..f989342 --- /dev/null +++ b/tests/rig/autosave_test.py @@ -0,0 +1,161 @@ +#!/usr/bin/env python3 +"""Check that edits reach the Nextcloud file without cron and without a flush. + +Issue #100: the file was only written once the last participant had left *and* +the Cleanup background job had run. Everything typed before that lived in this +server's database, so a browser closed at the wrong moment - or an instance +whose cron is slow or misconfigured - lost the work, silently. + +This drives one real editor and checks the file on disk at each step: + + 1. type, and the first save writes the file straight away + 2. type again inside the autosave interval - the file must not change, or the + converter is being run on every keystroke batch + 3. type again after the interval - both markers must be in the file now + 4. close the editor - everything must be in the file, the document folder + gone and the change list empty, with no cron and no `documentserver:flush` + +Nothing here runs occ except to read state and to set the interval. +""" +import argparse, asyncio, os, subprocess, sys, time + +sys.path.insert(0, os.path.join(os.path.dirname(os.path.abspath(__file__)), 'lib')) +import config +import harness +from driver import Session, DISMISS_JS, TEXT_JS + + +CLOSE_JS = r"""(() => { + try { + // what the editor's own close button goes through in the Nextcloud + // connector: OCA.Onlyoffice.onRequestClose + const ed = (window.OCA && OCA.Onlyoffice && OCA.Onlyoffice.docEditor) || window.docEditor; + if (ed && ed.destroyEditor) { + ed.destroyEditor(); + return 'destroyEditor()'; + } + const f = document.querySelector('iframe'); + const w = f && f.contentWindow; + const api = w && (w.Asc && w.Asc.editor); + if (api && api.asc_coAuthoringDisconnect) { + api.asc_coAuthoringDisconnect(); + return 'asc_coAuthoringDisconnect()'; + } + return 'no way to close found'; + } catch (e) { return 'error: ' + e; } +})()""" + + +async def main(): + p = argparse.ArgumentParser() + p.add_argument('--base', default=config.BASE) + p.add_argument('--a', default=config.ADMIN) + p.add_argument('--kind', default='docx', choices=sorted(config.DOCUMENTS)) + p.add_argument('--fileid', default=None) + p.add_argument('--file', default=None) + p.add_argument('--member', default=None) + p.add_argument('--chromium', default=config.CHROMIUM) + p.add_argument('--tag', default='AUTO') + p.add_argument('--interval', type=int, default=45) + args = p.parse_args() + document = config.document(args.kind) + args.fileid = args.fileid or document['fileid'] + args.file = args.file or document['name'] + args.member = args.member or document['member'] + + ua, pa = args.a.split(':', 1) + # unique per run: the sample file keeps what earlier runs typed into it, + # and a marker left over from last time looks exactly like one that was + # just written + t = args.tag + str(int(time.time()) % 100000) + ok = True + + print('==> resetting rig state') + harness.reset() + print(' autosave interval ->', args.interval, ':', + harness.set_app_config('autosave_interval', args.interval) or 'set') + # an app config value written through occ takes a moment to be visible to + # web requests (local APCu) + time.sleep(harness.APP_CONFIG_PROPAGATION) + + A = Session('A', 9222, ua, pa, args.base, args.fileid, args.chromium, (420, 400)) + try: + await A.start() + await A.login_and_open() + await A.drain(25) + await A.eval(DISMISS_JS) + + print(f'\n==> [1] type {t}1, nothing else') + await A.type(f'{t}1 ', row_offset=0) + await A.drain(12) + got = harness.file_markers(args.file, args.member, + [f'{t}1']) + print(' file:', got, '| doc folders:', harness.doc_folders(), '| changes:', harness.changes(), + '\n snapshot state:', harness.snapshot_state(), '| now:', int(time.time())) + if not got[f'{t}1']: + ok = False + print(' FAIL - the first save did not reach the file') + + # Well inside the interval: the editor keeps saving on its own while a + # session is open, and a check too close to the far end of the interval + # catches one of those rather than the one this step typed. + print(f'\n==> [2] type {t}2 straight away, inside the {args.interval}s interval') + job_before = harness.job_last_run() + await A.type(f'{t}2 ', row_offset=1) + await A.drain(6) + got = harness.file_markers(args.file, args.member, + [f'{t}1', f'{t}2']) + print(' file:', got, '\n snapshot state:', harness.snapshot_state(), '| now:', int(time.time())) + if got[f'{t}2']: + if harness.job_last_run() != job_before: + print(' (the Cleanup job ran in this window and wrote it; ' + 'the interval was not exercised this run)') + else: + ok = False + print(' FAIL - the interval is not being respected, every save assembles the document') + + print(f'\n==> [3] wait out the interval, then type {t}3') + await A.drain(args.interval) + await A.type(f'{t}3 ', row_offset=2) + await A.drain(15) + got = harness.file_markers(args.file, args.member, + [f'{t}1', f'{t}2', f'{t}3']) + print(' file:', got) + if not all(got.values()): + ok = False + print(' FAIL - the autosave did not write the newer edits') + + print(f'\n==> [4] type {t}4 and close the editor') + await A.type(f'{t}4 ', row_offset=3) + await A.drain(8) + print(' closing:', await A.eval(CLOSE_JS)) + # the beacon is only acted on once the session's poll has provably + # stopped, which takes Channel::SEEN_INTERVAL + 3 seconds + await A.drain(20) + finally: + await A.stop() + + got = harness.file_markers(args.file, args.member, + [f'{t}1', f'{t}2', f'{t}3', f'{t}4']) + print(' close command seen by the server:', harness.app_log('type=close') > 0) + print(' file:', got) + print(' doc folders:', harness.doc_folders(), '| changes:', harness.changes(), '| sessions:', harness.sessions()) + if not all(got.values()): + ok = False + print(' FAIL - closing the editor did not write everything to the file') + if harness.doc_folders() or harness.changes(): + ok = False + print(' FAIL - the document was written but its session was not ended') + + print('\n===== server log =====') + print(subprocess.run(['docker', 'exec', DOCKER_APP, 'sh', '-c', + "grep -o '\"message\":\"[^\"]*documentserver[^\"]*\"' " + "/var/www/html/data/nextcloud.log | tail -25"], + capture_output=True, text=True).stdout) + + print('\n VERDICT:', 'saving during editing works' if ok else 'FAIL') + return 0 if ok else 1 + + +if __name__ == '__main__': + sys.exit(asyncio.run(main())) diff --git a/tests/rig/chat_test.py b/tests/rig/chat_test.py new file mode 100755 index 0000000..20c191e --- /dev/null +++ b/tests/rig/chat_test.py @@ -0,0 +1,158 @@ +#!/usr/bin/env python3 +"""Check the editor's chat panel across two live sessions, plus the backlog. + +Sends a message from each of two sessions and reports what each one was told, +then asks for the history again to see whether someone opening the document +later would find the conversation. The panel does not echo what you type, so a +session that does not get its own message back has broken chat even if the +other side sees it. +""" +import argparse, asyncio, json, os, sys +sys.path.insert(0, os.path.join(os.path.dirname(os.path.abspath(__file__)), 'lib')) +import config +import harness +from driver import Session, DISMISS_JS + +# Chat arrives through CoAuthoringApi.onMessage(messages, clear); record it +# rather than reading the Backbone collection the panel renders from. +RECORD_JS = r"""(() => { + try { + const w = document.querySelector('iframe').contentWindow; + const inner = w.Asc.editor.CoAuthoringApi._CoAuthoringApi; + w.__rigChat = []; + const orig = inner.onMessage; + inner.onMessage = function (messages, clear) { + (messages || []).forEach(m => w.__rigChat.push( + {from: m.useridoriginal, name: m.username, text: m.message, clear: !!clear})); + if (orig) orig.call(this, messages, clear); + }; + return 'recording'; + } catch (e) { return 'err: ' + e; } +})()""" + +SEND_JS = r"""(() => { + try { + const w = document.querySelector('iframe').contentWindow; + w.Asc.editor.asc_coAuthoringChatSendMessage(%s); + return 'sent'; + } catch (e) { return 'err: ' + e; } +})()""" + +READ_JS = r"""(() => { + const w = document.querySelector('iframe').contentWindow; + return JSON.stringify(w.__rigChat || 'no recorder'); +})()""" + +# What the panel itself holds. A session that opened after the conversation has +# no recorder in place, so this is the only way to see whether its backlog +# request was answered. +PANEL_JS = r"""(() => { + try { + const w = document.querySelector('iframe').contentWindow; + for (const n of ['DE', 'SSE', 'PE', 'PDFE']) { + const app = w[n]; + if (app && app.getCollection) { + const c = app.getCollection('Common.Collections.ChatMessages'); + if (c) return JSON.stringify(c.map(m => m.get('message'))); + } + } + return JSON.stringify('no chat collection'); + } catch (e) { return JSON.stringify('err: ' + e); } +})()""" + +ASK_HISTORY_JS = r"""(() => { + try { + const w = document.querySelector('iframe').contentWindow; + w.__rigChat = []; + w.Asc.editor.CoAuthoringApi.getMessages(); + return 'asked'; + } catch (e) { return 'err: ' + e; } +})()""" + + +async def main(): + p = argparse.ArgumentParser() + p.add_argument('--base', default=config.BASE) + p.add_argument('--a', default=config.ADMIN) + p.add_argument('--b', default=config.USER2) + p.add_argument('--kind', default='docx', choices=sorted(config.DOCUMENTS)) + p.add_argument('--fileid', default=None) + p.add_argument('--chromium', default=config.CHROMIUM) + p.add_argument('--tag', default='') + args = p.parse_args() + args.fileid = args.fileid or config.document(args.kind)['fileid'] + + ua, pa = args.a.split(':', 1) + ub, pb = args.b.split(':', 1) + A = Session('A', 9222, ua, pa, args.base, args.fileid, args.chromium) + B = Session('B', 9333, ub, pb, args.base, args.fileid, args.chromium) + + msg_a = f'{args.tag}hello-from-A' + msg_b = f'{args.tag}hi-from-B' + + verdict = {'ok': False} + try: + await A.start(); await B.start() + print(f'==> opening file {args.fileid} as {ua} (A) and {ub} (B)') + await A.login_and_open(); await A.drain(25) + await B.login_and_open(); await B.drain(25) + await A.eval(DISMISS_JS); await B.eval(DISMISS_JS) + print(' A recorder:', await A.eval(RECORD_JS)) + print(' B recorder:', await B.eval(RECORD_JS)) + + print(f'==> A sends {msg_a!r}:', await A.eval(SEND_JS % json.dumps(msg_a))) + await A.drain(10); await B.drain(10) + print(f'==> B sends {msg_b!r}:', await B.eval(SEND_JS % json.dumps(msg_b))) + await B.drain(10); await A.drain(10) + + got = {} + for s in (A, B): + got[s.name] = json.loads(await s.eval(READ_JS)) + print(f'\n {s.name} received: {got[s.name]}') + + ok = True + for s in (A, B): + texts = [m['text'] for m in got[s.name]] + missing = [m for m in (msg_a, msg_b) if m not in texts] + if missing: + ok = False + print(f' {s.name}: MISSING {missing}') + else: + print(f' {s.name}: both messages present') + + print('\n==> asking for the backlog again, as a late joiner would') + await A.eval(ASK_HISTORY_JS) + await A.drain(10) + history = json.loads(await A.eval(READ_JS)) + print(' backlog:', [m['text'] for m in history]) + if not all(m in [h['text'] for h in history] for m in (msg_a, msg_b)): + ok = False + print(' backlog: INCOMPLETE') + else: + print(' backlog: complete') + + print('\n==> a third session opens the document now') + C = Session('C', 9555, ua, pa, args.base, args.fileid, args.chromium) + try: + await C.start() + await C.login_and_open(); await C.drain(25) + panel = json.loads(await C.eval(PANEL_JS)) + print(' C chat panel holds:', panel) + if not isinstance(panel, list) or not all(m in panel for m in (msg_a, msg_b)): + ok = False + print(' C: INCOMPLETE - a late joiner does not see the conversation') + else: + print(' C: sees the whole conversation') + finally: + await C.stop() + + print('\n VERDICT:', 'chat OK' if ok else 'FAIL') + verdict['ok'] = ok + finally: + await A.stop(); await B.stop() + + return 0 if verdict['ok'] else 1 + + +if __name__ == '__main__': + sys.exit(asyncio.run(main())) diff --git a/tests/rig/coedit_test.py b/tests/rig/coedit_test.py new file mode 100755 index 0000000..d32bcbb --- /dev/null +++ b/tests/rig/coedit_test.py @@ -0,0 +1,111 @@ +#!/usr/bin/env python3 +"""Open one document in two live sessions and report what each session receives. + +Drives two headless chromium instances over CDP, logs a different user into +each, opens the same file in both, types in each, and then reads the Engine.IO +long-poll response bodies to see which socket messages actually reached whom. +""" +import argparse, asyncio, os, re, sys + +sys.path.insert(0, os.path.join(os.path.dirname(os.path.abspath(__file__)), 'lib')) +import config +from driver import Session, COAUTH_JS, DISMISS_JS, PROBE_JS, TEXT_JS + + +async def main(): + p = argparse.ArgumentParser() + p.add_argument('--base', default=config.BASE) + p.add_argument('--a', default=config.ADMIN) + p.add_argument('--b', default=config.USER2) + p.add_argument('--kind', default='docx', choices=sorted(config.DOCUMENTS)) + p.add_argument('--fileid', default=None) + p.add_argument('--out', default='/tmp/coedit') + p.add_argument('--chromium', default=config.CHROMIUM) + p.add_argument('--rounds', type=int, default=1) + p.add_argument('--tag', default='', + help='prefix for the typed markers, so a saved file can be checked ' + 'for this run rather than a previous one') + args = p.parse_args() + os.makedirs(args.out, exist_ok=True) + if not args.fileid: + args.fileid = config.document(args.kind)['fileid'] + + ua, pa = args.a.split(':', 1) + ub, pb = args.b.split(':', 1) + A = Session('A', 9222, ua, pa, args.base, args.fileid, args.chromium, (420, 400), (20, 60)) + B = Session('B', 9333, ub, pb, args.base, args.fileid, args.chromium, (420, 470), (20, 66)) + + verdict = {'ok': False} + try: + await A.start(); await B.start() + print(f'==> opening file {args.fileid} as {ua} (A) and {ub} (B)') + await A.login_and_open(); await A.drain(25) + await B.login_and_open(); await B.drain(25) + print(' A where:', await A.where()) + print(' B where:', await B.where()) + print(' A state:', await A.eval(PROBE_JS)) + print(' B state:', await B.eval(PROBE_JS)) + + print(' A overlays:', await A.eval(DISMISS_JS)) + print(' B overlays:', await B.eval(DISMISS_JS)) + await A.send('Input.dispatchKeyEvent', type='rawKeyDown', key='Escape', + code='Escape', windowsVirtualKeyCode=27, nativeVirtualKeyCode=27) + await B.send('Input.dispatchKeyEvent', type='rawKeyDown', key='Escape', + code='Escape', windowsVirtualKeyCode=27, nativeVirtualKeyCode=27) + await A.drain(2); await B.drain(2) + + markers = [] + for r in range(1, args.rounds + 1): + print(f'==> round {r}: A types') + await A.type(f'{args.tag}A{r} ', row_offset=r - 1) + await A.drain(12); await B.drain(12) + print(f'==> round {r}: B types') + await B.type(f'{args.tag}B{r} ', row_offset=r - 1) + await B.drain(12); await A.drain(12) + markers += [f'{args.tag}A{r}', f'{args.tag}B{r}'] + print(f' round {r} A co-auth:', await A.eval(COAUTH_JS)) + print(f' round {r} B co-auth:', await B.eval(COAUTH_JS)) + + print('\n===== LIVE PROPAGATION =====') + text = {} + for s_ in (A, B): + text[s_.name] = await s_.eval(TEXT_JS) + print(f' {s_.name} sees: {(text[s_.name] or "")[:220]!r}') + ok = True + for s_ in (A, B): + missing = [m for m in markers if m not in (text[s_.name] or '')] + if missing: + ok = False + print(f' {s_.name}: missing markers {missing}' if missing + else f' {s_.name}: all {len(markers)} markers present') + print(' VERDICT:', 'live co-authoring OK' if ok else 'FAIL - changes did not propagate') + verdict['ok'] = ok + + print(' A state:', await A.eval(PROBE_JS)) + print(' B state:', await B.eval(PROBE_JS)) + for s in (A, B): + types, raw = await s.messages() + sids = sorted(set(re.findall(r'"type":"auth","result":1,"sessionId":"([0-9a-f]+)"', ' '.join(raw)))) + print(f'\n>>> session {s.name} ({s.user}): {len(sids)} socket session(s) established ' + f'-> {sids} (more than one means it reconnected mid-edit)') + print(f'\n===== session {s.name} ({s.user}) received =====') + for t, n in sorted(types.items(), key=lambda kv: -kv[1]): + print(f' {n:3d} {t}') + for r in raw: + if any(k in r for k in ('connectState', 'saveChanges', 'savePartChanges', 'cursor', 'participant')): + print(' msg:', r[:200]) + sent = [p for p in s.posts if 'saveChanges' in p] + print(f' sent {len(sent)} saveChanges command(s)') + for p in sent[:6]: + m = re.search(r'"startSaveChanges":(\w+).*?"endSaveChanges":(\w+)', p) + start, end = (m.group(1), m.group(2)) if m else ('?', '?') + print(f' sent: start={start} end={end} bytes={len(p)}') + await s.shot(os.path.join(args.out, f'{s.name}.png')) + finally: + await A.stop(); await B.stop() + + return 0 if verdict['ok'] else 1 + + +if __name__ == '__main__': + sys.exit(asyncio.run(main())) diff --git a/tests/rig/docker-compose.yml b/tests/rig/docker-compose.yml new file mode 100644 index 0000000..9c4eb3e --- /dev/null +++ b/tests/rig/docker-compose.yml @@ -0,0 +1,51 @@ +# A disposable Nextcloud with this app mounted live, for the browser tests. +# +# The project name decides the container names the scripts address, so it is +# pinned rather than taken from the directory: rig-app-1 and rig-db-1 whatever +# the checkout is called. Set RIG_PROJECT to run a second one alongside. +name: ${RIG_PROJECT:-rig} + +services: + db: + image: ${RIG_DB_IMAGE:-mariadb:11.8} + command: --transaction-isolation=READ-COMMITTED --log-bin=binlog --binlog-format=ROW + environment: + MARIADB_ROOT_PASSWORD: rigroot + MARIADB_DATABASE: ${RIG_DB_NAME:-nextcloud} + MARIADB_USER: ${RIG_DB_USER:-nextcloud} + MARIADB_PASSWORD: ${RIG_DB_PASSWORD:-rigpass} + volumes: + - db:/var/lib/mysql + healthcheck: + test: ["CMD", "healthcheck.sh", "--connect", "--innodb_initialized"] + interval: 5s + timeout: 5s + retries: 20 + + app: + image: ${RIG_NC_IMAGE:-nextcloud:34.0.3-apache} + depends_on: + db: + condition: service_healthy + ports: + - "127.0.0.1:${RIG_PORT:-8099}:80" + environment: + MYSQL_HOST: db + MYSQL_DATABASE: ${RIG_DB_NAME:-nextcloud} + MYSQL_USER: ${RIG_DB_USER:-nextcloud} + MYSQL_PASSWORD: ${RIG_DB_PASSWORD:-rigpass} + NEXTCLOUD_ADMIN_USER: ${RIG_ADMIN_USER:-admin} + NEXTCLOUD_ADMIN_PASSWORD: ${RIG_ADMIN_PASSWORD:-rigadmin} + NEXTCLOUD_TRUSTED_DOMAINS: "localhost 127.0.0.1" + OVERWRITECLIURL: "http://localhost:${RIG_PORT:-8099}" + PHP_MEMORY_LIMIT: 1G + PHP_UPLOAD_LIMIT: 512M + volumes: + - nc:/var/www/html + # the working copy under test, mounted live, so an edit takes effect + # without rebuilding anything + - ${APP_SOURCE:-../..}:/var/www/html/custom_apps/documentserver_community + +volumes: + db: + nc: diff --git a/tests/rig/flush_live_test.py b/tests/rig/flush_live_test.py new file mode 100755 index 0000000..ce86381 --- /dev/null +++ b/tests/rig/flush_live_test.py @@ -0,0 +1,127 @@ +#!/usr/bin/env python3 +"""Flushing a document somebody is still editing must not empty its change list. + +`occ documentserver:flush` (and the Cleanup job, and the Save button) used to +run the same code whether or not anybody was in the document: apply the changes +to the file and then delete them. But the change list is the only record of +what was typed - Editor.bin stays at the version the document was opened at - so +consuming it mid-session means the next flush writes the file from what is left, +losing everything from before. A session opened afterwards gets the same empty +list and shows the document as it was before the edits. + + 1. type, then flush while the session is live + 2. the file has the text, and the change list and document are still there + 3. type again, flush again with everybody gone + 4. the file must hold both edits - the second flush must not have dropped the + first one +""" +import argparse, asyncio, os, subprocess, sys, time + +sys.path.insert(0, os.path.join(os.path.dirname(os.path.abspath(__file__)), 'lib')) +import config +import harness +from driver import Session, DISMISS_JS, TEXT_JS + + +async def main(): + p = argparse.ArgumentParser() + p.add_argument('--base', default=config.BASE) + p.add_argument('--a', default=config.ADMIN) + p.add_argument('--b', default=config.USER2) + p.add_argument('--kind', default='docx', choices=sorted(config.DOCUMENTS)) + p.add_argument('--fileid', default=None) + p.add_argument('--file', default=None) + p.add_argument('--member', default=None) + p.add_argument('--chromium', default=config.CHROMIUM) + p.add_argument('--tag', default='FLUSH') + args = p.parse_args() + document = config.document(args.kind) + args.fileid = args.fileid or document['fileid'] + args.file = args.file or document['name'] + args.member = args.member or document['member'] + + ua, pa = args.a.split(':', 1) + ub, pb = args.b.split(':', 1) + # unique per run: the sample file keeps what earlier runs typed into it, + # and a marker left over from last time looks exactly like one that was + # just written + t = args.tag + str(int(time.time()) % 100000) + ok = True + + print('==> resetting rig state') + harness.reset() + # off, so that what the file holds is the flush's doing and nothing else + harness.set_app_config('autosave_interval', 0) + time.sleep(harness.APP_CONFIG_PROPAGATION) + + A = Session('A', 9222, ua, pa, args.base, args.fileid, args.chromium, (420, 400)) + try: + await A.start() + await A.login_and_open(); await A.drain(25) + await A.eval(DISMISS_JS) + + print(f'\n==> [1] type {t}1, then flush with the session still open') + await A.type(f'{t}1 ') + await A.drain(10) + before = harness.changes() + print(' file before the flush:', + harness.file_markers(args.file, args.member, + [f'{t}1'])) + print(' flush: exit=%d %s' % harness.flush()) + await A.drain(5) + + got = harness.file_markers(args.file, args.member, + [f'{t}1']) + print(' file:', got, '| changes:', harness.changes(), '(was', str(before) + ')', + '| doc folders:', harness.doc_folders(), '| sessions:', harness.sessions()) + if not got[f'{t}1']: + ok = False + print(' FAIL - the flush did not write the document') + if harness.changes() < before: + ok = False + print(' FAIL - the flush consumed the change list of a live session') + if harness.doc_folders() == 0: + ok = False + print(' FAIL - the flush disposed of a document that is still open') + + print('\n==> [2] a second session joins: it must see the first one\'s work') + B = Session('B', 9333, ub, pb, args.base, args.fileid, args.chromium, (420, 470)) + try: + await B.start() + await B.login_and_open() + await B.drain(25) + text = str(await B.eval(TEXT_JS)) + print(' joiner sees the marker:', f'{t}1' in text) + if f'{t}1' not in text: + ok = False + print(' FAIL - the joiner replayed an empty change list against the ' + 'baseline and got the document as it was before the edits') + finally: + await B.stop() + + print(f'\n==> [3] the session is still usable: type {t}2') + await A.type(f'{t}2 ') + await A.drain(10) + print(' changes:', harness.changes(), '| sessions:', harness.sessions()) + finally: + await A.stop() + + print('\n==> [4] everybody gone, flush again') + harness.drop_sessions() + print(' flush: exit=%d %s' % harness.flush()) + got = harness.file_markers(args.file, args.member, + [f'{t}1', f'{t}2']) + print(' file:', got, '| doc folders:', harness.doc_folders(), '| changes:', harness.changes()) + if not all(got.values()): + ok = False + print(' FAIL - the final flush lost an edit') + if harness.doc_folders() or harness.changes(): + ok = False + print(' FAIL - the document was not disposed of once everybody had left') + + print('\n VERDICT:', 'flushing a live document is safe' if ok else 'FAIL') + return 0 if ok else 1 + + +if __name__ == '__main__': + sys.exit(asyncio.run(main())) diff --git a/tests/rig/leave_test.py b/tests/rig/leave_test.py new file mode 100755 index 0000000..06e273f --- /dev/null +++ b/tests/rig/leave_test.py @@ -0,0 +1,142 @@ +#!/usr/bin/env python3 +"""Check what happens to the document when people leave it. + +Two ways of leaving, both of which used to leave the file unwritten until the +Cleanup background job came round (#100): + + * closing the editor - destroyEditor() takes the iframe out of the DOM + * closing the tab - the whole page unloads + +and one thing that must *not* happen: the first of two participants leaving +must not end the document for the one still typing into it. + + 1. A and B open the document and both type + 2. A closes the editor: the document stays open, B keeps editing + 3. B types again - which must still reach the file + 4. B closes the tab: everything is in the file, the document is disposed of + +No cron and no `documentserver:flush` anywhere in here. +""" +import argparse, asyncio, os, subprocess, sys, time + +sys.path.insert(0, os.path.join(os.path.dirname(os.path.abspath(__file__)), 'lib')) +import config +import harness +from driver import Session, DISMISS_JS, TEXT_JS + +CLOSE_EDITOR_JS = """(() => { + const ed = (window.OCA && OCA.Onlyoffice && OCA.Onlyoffice.docEditor) || window.docEditor; + if (ed && ed.destroyEditor) { ed.destroyEditor(); return 'destroyEditor()'; } + return 'no editor to close'; +})()""" + + +async def main(): + p = argparse.ArgumentParser() + p.add_argument('--base', default=config.BASE) + p.add_argument('--a', default=config.ADMIN) + p.add_argument('--b', default=config.USER2) + p.add_argument('--kind', default='docx', choices=sorted(config.DOCUMENTS)) + p.add_argument('--fileid', default=None) + p.add_argument('--file', default=None) + p.add_argument('--member', default=None) + p.add_argument('--chromium', default=config.CHROMIUM) + p.add_argument('--tag', default='LEAVE') + p.add_argument('--interval', type=int, default=25) + args = p.parse_args() + document = config.document(args.kind) + args.fileid = args.fileid or document['fileid'] + args.file = args.file or document['name'] + args.member = args.member or document['member'] + + ua, pa = args.a.split(':', 1) + ub, pb = args.b.split(':', 1) + # unique per run: the sample file keeps what earlier runs typed into it, + # and a marker left over from last time looks exactly like one that was + # just written + t = args.tag + str(int(time.time()) % 100000) + ok = True + + print('==> resetting rig state') + harness.reset() + harness.set_app_config('autosave_interval', args.interval) + time.sleep(harness.APP_CONFIG_PROPAGATION) + + A = Session('A', 9222, ua, pa, args.base, args.fileid, args.chromium, (420, 400)) + B = Session('B', 9333, ub, pb, args.base, args.fileid, args.chromium, (420, 470)) + try: + await A.start(); await B.start() + await A.login_and_open(); await A.drain(25) + await B.login_and_open(); await B.drain(25) + await A.eval(DISMISS_JS); await B.eval(DISMISS_JS) + + print(f'\n==> [1] both type') + await A.type(f'{t}-A1 '); await A.drain(10); await B.drain(10) + await B.type(f'{t}-B1 '); await B.drain(10); await A.drain(10) + print(' sessions:', harness.sessions()) + + print(f'\n==> [2] A closes the editor while B is still in the document') + print(' A closing:', await A.eval(CLOSE_EDITOR_JS)) + await B.drain(20) + left = harness.sessions() + print(' sessions:', left, '| doc folders:', harness.doc_folders(), '| changes:', harness.changes()) + if left != 1: + ok = False + print(' FAIL - A leaving should have left exactly B behind') + if harness.doc_folders() == 0 or harness.changes() == 0: + ok = False + print(' FAIL - the document was disposed of while B is still editing it') + + print(f'\n==> [3] B keeps typing, and it still reaches the file') + await B.drain(args.interval) + await B.type(f'{t}-B2 ') + await B.drain(15) + got = harness.file_markers(args.file, args.member, + [f'{t}-A1', f'{t}-B1', f'{t}-B2']) + print(' file:', got) + if not all(got.values()): + ok = False + print(' FAIL - B lost the document server when A left') + + print(f'\n==> [4] B types once more and closes the tab') + await B.type(f'{t}-B3 ') + await B.drain(8) + await B.send('Page.navigate', url=args.base + '/apps/files') + await B.drain(22) + finally: + await A.stop(); await B.stop() + + got = harness.file_markers(args.file, args.member, + [f'{t}-A1', f'{t}-B1', f'{t}-B2', f'{t}-B3']) + print(' file:', got) + print(' doc folders:', harness.doc_folders(), '| changes:', harness.changes(), '| sessions:', harness.sessions()) + if not all(got.values()): + ok = False + print(' FAIL - closing the tab did not write everything to the file') + if harness.doc_folders() or harness.changes(): + ok = False + print(' FAIL - the last participant left but the document was not disposed of') + + print('\n==> [5] reopen the document: the editor must show what was typed') + C = Session('C', 9444, ua, pa, args.base, args.fileid, args.chromium, (420, 400)) + try: + await C.start() + await C.login_and_open() + await C.drain(25) + text = await C.eval(TEXT_JS) + missing = [m for m in (f'{t}-A1', f'{t}-B1', f'{t}-B2', f'{t}-B3') + if m not in str(text)] + print(' markers missing from the reopened document:', missing or 'none') + if missing: + ok = False + print(' FAIL - reopening the document did not show the saved work') + finally: + await C.stop() + + print('\n VERDICT:', 'leaving a document saves it' if ok else 'FAIL') + return 0 if ok else 1 + + + +if __name__ == '__main__': + sys.exit(asyncio.run(main())) diff --git a/tests/rig/lib/config.py b/tests/rig/lib/config.py new file mode 100644 index 0000000..43410d1 --- /dev/null +++ b/tests/rig/lib/config.py @@ -0,0 +1,82 @@ +"""Where the rig is and what is in it. + +Everything is overridable from the environment, and `provision.sh` writes what +it created into `.rig-env` next to this package, so the tests do not carry a +particular instance's file ids around in their defaults. +""" +import os + +HERE = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +ENV_FILE = os.path.join(HERE, '.rig-env') + + +def _load_env_file(): + """Read .rig-env into the environment, without overriding what is set.""" + try: + with open(ENV_FILE) as f: + lines = f.readlines() + except OSError: + return + for line in lines: + line = line.strip() + if not line or line.startswith('#') or '=' not in line: + continue + key, value = line.split('=', 1) + os.environ.setdefault(key.strip(), value.strip().strip('"\'')) + + +_load_env_file() + + +def _env(name, default): + value = os.environ.get(name) + return value if value else default + + +BASE = _env('RIG_BASE', 'http://127.0.0.1:8099') +PROJECT = _env('RIG_PROJECT', 'rig') +APP_CONTAINER = _env('RIG_APP_CONTAINER', f'{PROJECT}-app-1') +DB_CONTAINER = _env('RIG_DB_CONTAINER', f'{PROJECT}-db-1') + +DB_NAME = _env('RIG_DB_NAME', 'nextcloud') +DB_USER = _env('RIG_DB_USER', 'nextcloud') +DB_PASSWORD = _env('RIG_DB_PASSWORD', 'rigpass') + +ADMIN = _env('RIG_ADMIN', 'admin:rigadmin') +USER2 = _env('RIG_USER2', 'bob:Kq7-rigTest-3910') + +CHROMIUM = _env('RIG_CHROMIUM', '/usr/bin/chromium') + +# The documents provisioning uploaded, and where the text typed into each of +# them ends up inside the saved file. +DOCUMENTS = { + 'docx': { + 'name': _env('RIG_FILE_DOCX', 'sample.docx'), + 'fileid': _env('RIG_FILEID_DOCX', ''), + 'member': 'word/document.xml', + }, + 'xlsx': { + 'name': _env('RIG_FILE_XLSX', 'sample.xlsx'), + 'fileid': _env('RIG_FILEID_XLSX', ''), + 'member': 'xl/sharedStrings.xml', + }, + 'pptx': { + 'name': _env('RIG_FILE_PPTX', 'sample.pptx'), + 'fileid': _env('RIG_FILEID_PPTX', ''), + 'member': 'ppt/slides/slide1.xml', + }, +} + + +def document(kind='docx'): + doc = DOCUMENTS[kind] + if not doc['fileid']: + raise SystemExit( + f"no file id for the {kind} document: run ./rig.sh provision first, " + f"or set RIG_FILEID_{kind.upper()}") + return doc + + +def credentials(pair): + user, password = pair.split(':', 1) + return user, password diff --git a/tests/rig/lib/driver.py b/tests/rig/lib/driver.py new file mode 100644 index 0000000..bb897c6 --- /dev/null +++ b/tests/rig/lib/driver.py @@ -0,0 +1,331 @@ +"""Driving the editor: a headless chromium per session, over CDP. + +The editor renders to a canvas, so nothing about what a session displays can be +read out of the DOM. Every question about what a user actually sees is asked of +the editor's own document model instead, which is what TEXT_JS and COAUTH_JS are +for. +""" +import asyncio, base64, json, os, re, shutil, subprocess, sys, tempfile, time +import urllib.request +import websockets + +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) +import config + + +LOGIN_JS = r"""(() => { + const u = document.querySelector('input[name=user], input#user, input[autocomplete="username"]'); + const p = document.querySelector('input[name=password], input#password, input[type=password]'); + if (!u || !p) return 'form not found'; + const set = (el, v) => { el.focus(); el.value = v; + el.dispatchEvent(new Event('input', {bubbles: true})); + el.dispatchEvent(new Event('change', {bubbles: true})); }; + set(u, %s); set(p, %s); + const form = u.closest('form'); + const btn = form && (form.querySelector('button[type=submit]') || form.querySelector('input[type=submit]')); + if (btn) { btn.click(); return 'submitted'; } + if (form) { form.requestSubmit ? form.requestSubmit() : form.submit(); return 'submitted'; } + return 'no submit control'; +})()""" + +DISMISS_JS = r"""(() => { + // Nextcloud can put a modal (first-run wizard, "what's new") over the page, + // which would swallow the clicks meant for the document. + let closed = 0; + document.querySelectorAll('.modal-mask, .modal-container, [role="dialog"]').forEach(d => { + d.querySelectorAll('button, a').forEach(b => { + const label = ((b.getAttribute('aria-label') || '') + ' ' + (b.className || '')).toLowerCase(); + if (/close|fermer/.test(label)) { b.click(); closed++; } + }); + }); + return closed + ' closed, ' + document.querySelectorAll('.modal-mask').length + ' overlay(s) left'; +})()""" + +COAUTH_JS = r"""(() => { + // What the co-authoring client thinks its state is. lastOtherSaveTime is only + // set once a change from another user has been applied, so it is the signal + // that live co-editing works; a non-empty chunk buffer is the signal that + // remote changes arrived but are being held back (missing endSaveChanges). + try { + const w = document.querySelector('iframe').contentWindow; + // Asc.editor.CoAuthoringApi is the CDocsCoApi facade; the state lives on + // the DocsCoApi it wraps, and reading the facade silently yields undefined. + const outer = w.Asc && w.Asc.editor && w.Asc.editor.CoAuthoringApi; + const co = outer && outer._CoAuthoringApi; + if (!co) return JSON.stringify({error: 'no CoAuthoringApi'}); + return JSON.stringify({ + isCoAuthoring: co.isCoAuthoring, + countEditUsers: co._countEditUsers, + participants: Object.keys(co._participants || {}), + lastOtherSaveTime: co.lastOtherSaveTime, + lastOwnSaveTime: co.lastOwnSaveTime, + bufferedChunks: (co._saveChangesChunks || []).length, + bufferedOther: (co._authOtherChanges || []).length, + changesIndex: co.changesIndex, + serverChangesSize: outer.get_serverChangesSize(), + state: co._state, + }); + } catch (e) { return JSON.stringify({error: String(e)}); } +})()""" + +TEXT_JS = r"""(() => { + // The page renders to a canvas, so the only way to see what a session + // currently displays is to ask its document model. Each editor keeps a + // different one, and select-all is no use: GetSelectedText returns null + // unless the document has focus, which a headless tab does not reliably get. + try { + const w = document.querySelector('iframe').contentWindow; + const api = w.Asc && w.Asc.editor; + if (!api) return 'no editor'; + + if (api.wbModel) { // spreadsheet + const out = []; + api.wbModel.aWorksheets.forEach(ws => { + ws.getRange3(0, 0, 200, 30)._foreachNoEmpty(c => { + const v = c.getValueWithoutFormat(); + // Addresses, not just values: when two sessions edit one document + // the interesting question is usually which cell a value landed in. + if (v) out.push(c.getName() + '=' + v); + }); + }); + return out.join(' | '); + } + + const ld = api.WordControl && api.WordControl.m_oLogicDocument; + if (!ld) return 'no logic document'; + + const paras = content => content.GetAllParagraphs({All: true}).map(p => p.GetText()).join(' '); + + if (ld.Slides) { // presentation + return ld.Slides.map(sl => (sl.cSld.spTree || []).map(sp => + (sp.txBody && sp.txBody.content) ? paras(sp.txBody.content) : '' + ).filter(Boolean).join(' ')).join(' | '); + } + + return paras(ld); // text document + } catch (e) { return 'err: ' + e; } +})()""" + +PLACE_CELL_JS = r"""(() => { + // Spreadsheets need the caret placed by address, not by pixel: this sheet + // covers its top-left cells with charts and slicers, so a click there + // selects a drawing and the keystrokes go into it (or nowhere) instead of + // into a cell. Returns false for the other editors, which take the click. + try { + const w = document.querySelector('iframe').contentWindow; + const api = w.Asc && w.Asc.editor; + if (!api || !api.wb) return false; + api.wb.getWorksheet().setSelection(new w.Asc.Range(%d, %d, %d, %d)); + return true; + } catch (e) { return 'err: ' + e; } +})()""" + +PROBE_JS = r"""(() => { + const frame = document.querySelector('iframe'); + let inner = null; + try { + const d = frame && frame.contentDocument; + if (d) { + const err = d.querySelector('#id-error-mask-title, .error-mask-title'); + inner = { + errorTitle: err && err.innerText, + hasLoadMask: !!d.querySelector('.asc-loadmask, #loadmask'), + // the co-editing users button shows the participant count + users: (d.querySelector('#tlb-box-users, .btn-users, #left-btn-support') || {}).innerText || null, + status: (d.querySelector('#status-label-zoom, .status-label') || {}).innerText || null, + }; + } + } catch (e) { inner = {crossOrigin: String(e)}; } + return JSON.stringify(inner); +})()""" + + +class Session: + def __init__(self, name, port, user, password, base, fileid, chromium, + click=(420, 400), cell=(20, 60)): + self.name, self.port, self.user, self.password = name, port, user, password + self.base, self.fileid, self.chromium = base, fileid, chromium + # Where this session puts its caret. The two sessions must not land on + # the same spot: in a spreadsheet that means both typing into one cell, + # and the second value simply replaces the first, which looks exactly + # like the first one never propagated. + self.click = click + # Target cell (col, row), zero-based, for the spreadsheet editor. Far + # enough out to be empty in the sample workbook. + self.cell = cell + self.events, self.id, self.polls, self.posts = [], 0, {}, [] + + async def send(self, method, **params): + self.id += 1 + await self.ws.send(json.dumps({'id': self.id, 'method': method, 'params': params})) + while True: + msg = json.loads(await self.ws.recv()) + if msg.get('id') == self.id: + if 'error' in msg: + raise RuntimeError(f"{self.name} {method}: {msg['error']}") + return msg.get('result', {}) + self.note(msg) + + def note(self, msg): + self.events.append(msg) + if msg.get('method') == 'Network.requestWillBeSent': + req = msg['params'].get('request', {}) + if 'EIO=4' in req.get('url', '') and req.get('postData'): + self.posts.append(req['postData']) + if msg.get('method') == 'Network.responseReceived': + url = msg['params'].get('response', {}).get('url', '') + if 'EIO=4' in url and 'transport=polling' in url: + self.polls[msg['params']['requestId']] = url + + async def drain(self, seconds): + deadline = time.time() + seconds + while time.time() < deadline: + try: + msg = json.loads(await asyncio.wait_for(self.ws.recv(), + timeout=max(0.2, deadline - time.time()))) + self.note(msg) + except (asyncio.TimeoutError, TimeoutError): + break + except Exception: + break + + async def start(self): + self.profile = tempfile.mkdtemp(prefix=f'cdp-{self.name}-') + self.proc = subprocess.Popen([ + self.chromium, '--headless=new', '--no-sandbox', '--disable-gpu', + '--disable-dev-shm-usage', '--window-size=1400,900', + f'--remote-debugging-port={self.port}', f'--user-data-dir={self.profile}', + '--no-first-run', '--no-default-browser-check', 'about:blank', + ], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + ws_url = None + for _ in range(60): + try: + targets = json.load(urllib.request.urlopen(f'http://127.0.0.1:{self.port}/json/list')) + pages = [t for t in targets if t['type'] == 'page'] + if pages: + ws_url = pages[0]['webSocketDebuggerUrl'] + break + except Exception: + await asyncio.sleep(0.5) + if not ws_url: + raise RuntimeError(f'{self.name}: chromium gave no debugging target') + self.conn = await websockets.connect(ws_url, max_size=64 * 1024 * 1024, ping_interval=None) + self.ws = self.conn + for m in ('Runtime.enable', 'Log.enable', 'Network.enable', 'Page.enable'): + await self.send(m) + + async def login_and_open(self): + await self.send('Page.navigate', url=self.base + '/login') + await self.drain(5) + print(f' {self.name} login:', await self.eval(LOGIN_JS % (json.dumps(self.user), json.dumps(self.password)))) + await self.drain(8) + who = await self.eval("document.location.pathname + '|' + ((window.OC && OC.currentUser) || '?')") + print(f' {self.name} after login:', who) + await self.send('Page.navigate', url=f'{self.base}/apps/onlyoffice/{self.fileid}') + + async def where(self): + return await self.eval( + "document.location.pathname + ' | iframe=' + !!document.querySelector('iframe')" + r" + ' | ' + (document.body ? document.body.innerText.slice(0,160).replace(/[\n\t]+/g,' ') : '')") + + async def eval(self, expr): + res = await self.send('Runtime.evaluate', expression=expr, returnByValue=True) + if res.get('exceptionDetails'): + return f"JS error: {res['exceptionDetails'].get('text')}" + return res.get('result', {}).get('value') + + async def type(self, text, enter=True, row_offset=0): + for ev in ('mousePressed', 'mouseReleased'): + await self.send('Input.dispatchMouseEvent', type=ev, + x=self.click[0], y=self.click[1], + button='left', clickCount=1) + await self.drain(1.5) + # One cell per round: writing the same cell twice just replaces the + # value, which would make an earlier round's marker look lost. + cell = (self.cell[0], self.cell[1] + row_offset) + placed = await self.eval(PLACE_CELL_JS % (cell + cell)) + if placed is True: + # A click may have grabbed a chart; drop that selection before + # typing, then re-place the caret on the target cell. + await self.send('Input.dispatchKeyEvent', type='rawKeyDown', key='Escape', + code='Escape', windowsVirtualKeyCode=27, nativeVirtualKeyCode=27) + await self.send('Input.dispatchKeyEvent', type='keyUp', key='Escape', code='Escape') + await self.eval(PLACE_CELL_JS % (cell + cell)) + for ch in text: + await self.send('Input.dispatchKeyEvent', type='keyDown', text=ch, + unmodifiedText=ch, key=ch) + await self.send('Input.dispatchKeyEvent', type='keyUp', key=ch) + if enter: + # A spreadsheet keeps what was typed in the cell editor until the + # edit is committed, so nothing is sent to the server without this. + for t in ('rawKeyDown', 'char', 'keyUp'): + await self.send('Input.dispatchKeyEvent', type=t, key='Enter', + code='Enter', text='\r', unmodifiedText='\r', + windowsVirtualKeyCode=13, nativeVirtualKeyCode=13) + + async def messages(self): + """socket message types this session received, from the poll bodies.""" + types, raw = {}, [] + for rid in list(self.polls): + try: + body = await self.send('Network.getResponseBody', requestId=rid) + except Exception: + continue + text = body.get('body', '') + if body.get('base64Encoded'): + try: + text = base64.b64decode(text).decode('utf-8', 'replace') + except Exception: + continue + for m in re.finditer(r'42\[(.*)\]', text): + raw.append(m.group(1)[:200]) + for t in re.findall(r'"type"\s*:\s*"([a-zA-Z]+)"', m.group(1)): + types[t] = types.get(t, 0) + 1 + return types, raw + + async def shot(self, path): + img = await self.send('Page.captureScreenshot') + with open(path, 'wb') as f: + f.write(base64.b64decode(img['data'])) + + async def stop(self): + try: + await self.conn.close() + except Exception: + pass + self.proc.terminate() + try: + self.proc.wait(timeout=10) + except Exception: + self.proc.kill() + shutil.rmtree(self.profile, ignore_errors=True) + +class Tab(Session): + """A second tab in a browser that is already running. + + Two tabs share a cookie jar - so one Nextcloud session - a connection pool + and any service worker, which two separate browsers do not. That is what + somebody opening the same document twice actually has, and it is a case the + two-browser tests cannot reach. + """ + + async def start(self): + req = urllib.request.Request( + f'http://127.0.0.1:{self.port}/json/new?url=about:blank', method='PUT') + info = json.load(urllib.request.urlopen(req)) + self.target_id = info['id'] + self.conn = await websockets.connect(info['webSocketDebuggerUrl'], + max_size=64 * 1024 * 1024, ping_interval=None) + self.ws = self.conn + for m in ('Runtime.enable', 'Log.enable', 'Network.enable', 'Page.enable'): + await self.send(m) + + async def login_and_open(self): + # the cookie is already in this browser, so straight to the document + await self.send('Page.navigate', url=f'{self.base}/apps/onlyoffice/{self.fileid}') + + async def stop(self): + try: + await self.conn.close() + except Exception: + pass diff --git a/tests/rig/lib/harness.py b/tests/rig/lib/harness.py new file mode 100644 index 0000000..e659075 --- /dev/null +++ b/tests/rig/lib/harness.py @@ -0,0 +1,162 @@ +"""Talking to the rig from outside the browser: occ, the database, the files. + +The tests use these to set the app up, to read the state the app keeps, and to +look at what actually landed in the file - which is the only thing that counts +for the saving tests, since the editor saying "all changes are saved" only ever +meant they reached the server. +""" +import os +import subprocess +import sys +import tempfile + +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) +import config + +APP_ID = 'documentserver_community' + + +def occ(*args, check=False): + r = subprocess.run( + ['docker', 'exec', '-u', 'www-data', config.APP_CONTAINER, 'php', 'occ', *args], + capture_output=True, text=True) + out = (r.stdout + r.stderr).strip() + if check and r.returncode != 0: + raise RuntimeError(f'occ {" ".join(args)} failed: {out}') + return out + + +def app_exec(command): + r = subprocess.run(['docker', 'exec', config.APP_CONTAINER, 'sh', '-c', command], + capture_output=True, text=True) + return r.stdout.strip() + + +def sql(query): + r = subprocess.run( + ['docker', 'exec', config.DB_CONTAINER, 'mariadb', + f'-u{config.DB_USER}', f'-p{config.DB_PASSWORD}', config.DB_NAME, + '-N', '-B', '-e', query], + capture_output=True, text=True) + return r.stdout.strip() + + +def set_app_config(key, value): + """An app config value, and the wait for it to be visible to web requests. + + occ writes it through the local cache the web server does not share, so a + test that reads it back straight away sees the old one and concludes the + setting does nothing. + """ + occ('config:app:set', APP_ID, key, '--value', str(value)) + + +def delete_app_config(key): + occ('config:app:delete', APP_ID, key) + + +APP_CONFIG_PROPAGATION = 14 # seconds; see set_app_config + + +def reset(): + """Drop per-document state so the next run starts from the file on disk.""" + sql('truncate oc_documentserver_sess; truncate oc_documentserver_changes; ' + 'truncate oc_documentserver_locks; truncate oc_documentserver_ipc;') + app_exec('rm -rf /var/www/html/data/appdata_*/documentserver_community/doc_*') + # appdata was touched behind Nextcloud's back, so resync the file cache or + # the next conversion fails with 'Could not create path .../Editor.bin' + occ('files:scan-app-data') + app_exec(': > /var/www/html/data/nextcloud.log') + + +def doc_folders(): + """Open documents, ignoring doc_0 - the scratch folder the connector's + preview conversions go through, which has nothing to do with editing.""" + out = app_exec('ls -d /var/www/html/data/appdata_*/documentserver_community/doc_* ' + '2>/dev/null') + return len([line for line in out.split() if not line.endswith('/doc_0')]) + + +def snapshot_state(): + """The {index, time} each open document was last written out at.""" + out = app_exec( + 'for f in /var/www/html/data/appdata_*/documentserver_community/doc_*/snapshot; ' + 'do echo "$(dirname $f | sed s#.*/##)=$(cat $f)"; done') + return out.replace('\n', ' ') or 'none' + + +def changes(): + return int(sql('select count(*) from oc_documentserver_changes;') or 0) + + +def sessions(): + return int(sql('select count(*) from oc_documentserver_sess;') or 0) + + +def drop_sessions(): + sql('delete from oc_documentserver_sess;') + + +def job_last_run(): + """When the Cleanup background job last ran. + + A rig running background jobs in AJAX mode runs them whenever a browser + makes a request, and the job writes documents that are being edited too, so + a test asserting that a file did *not* change has to allow for it. + """ + return sql("select last_run from oc_jobs where class like " + "'%DocumentServer%Cleanup';") or '0' + + +def flush(*options): + r = subprocess.run( + ['docker', 'exec', '-u', 'www-data', config.APP_CONTAINER, 'php', 'occ', + 'documentserver:flush', *options], + capture_output=True, text=True) + return r.returncode, (r.stdout + r.stderr).strip() + + +def run_cleanup_job(): + """Run the Cleanup background job once, whatever the cron mode is.""" + job = sql("select id from oc_jobs where class like '%DocumentServer%Cleanup';") + if not job: + return 'no cleanup job registered' + r = subprocess.run( + ['docker', 'exec', '-u', 'www-data', config.APP_CONTAINER, 'php', 'occ', + 'background-job:execute', job.split()[0], '--force-execute'], + capture_output=True, text=True) + return f'exit={r.returncode}' + + +def download(name, user_pair=None): + """Fetch a file over WebDAV, returning the local path.""" + user, password = config.credentials(user_pair or config.ADMIN) + target = os.path.join(tempfile.gettempdir(), f'rig-{user}-{os.path.basename(name)}') + subprocess.run(['curl', '-s', '-u', f'{user}:{password}', + f'{config.BASE}/remote.php/dav/files/{user}/{name}', + '-o', target], capture_output=True) + return target + + +def file_markers(name, member, markers, user_pair=None): + """Which of these markers are in the saved file right now.""" + path = download(name, user_pair) + found = {} + for marker in markers: + r = subprocess.run(f'unzip -p {path} {member} 2>/dev/null | grep -c {marker}', + shell=True, capture_output=True, text=True) + found[marker] = (r.stdout.strip() or '0') != '0' + return found + + +def app_log(pattern): + out = app_exec(f"grep -c '{pattern}' /var/www/html/data/nextcloud.log || true") + return int(out or 0) + + +def app_log_errors(): + """Error-level log lines from this app, as one string per line.""" + out = app_exec( + "grep -o '\"level\":[34][^\\n]*documentserver[^\\n]*' " + "/var/www/html/data/nextcloud.log | tail -20 || true") + return [line for line in out.split('\n') if line.strip()] diff --git a/tests/rig/probe.py b/tests/rig/probe.py new file mode 100755 index 0000000..e04c63f --- /dev/null +++ b/tests/rig/probe.py @@ -0,0 +1,55 @@ +#!/usr/bin/env python3 +"""Open one document in one live session and evaluate arbitrary JS in it. + +The other harnesses here are batch runs; this one exists to poke at a live +editor while working out which sdkjs call reports what. Expressions come from +--js (repeatable) or a file, and run in the top-level page, so reach into the +editor through document.querySelector('iframe').contentWindow. +""" +import argparse, asyncio, json, os, sys +sys.path.insert(0, os.path.join(os.path.dirname(os.path.abspath(__file__)), 'lib')) +import config +import harness +from driver import Session, DISMISS_JS + +async def main(): + p = argparse.ArgumentParser() + p.add_argument('--base', default=config.BASE) + p.add_argument('--user', default=config.ADMIN) + p.add_argument('--kind', default='docx', choices=sorted(config.DOCUMENTS)) + p.add_argument('--fileid', default=None) + p.add_argument('--chromium', default=config.CHROMIUM) + p.add_argument('--type-text', default='') + p.add_argument('--js', action='append', default=[]) + p.add_argument('--js-file') + p.add_argument('--settle', type=float, default=25) + p.add_argument('--pause', type=float, default=0, + help='seconds to let the page run between expressions, for when one ' + 'expression triggers a round trip the next one checks') + args = p.parse_args() + args.fileid = args.fileid or config.document(args.kind)['fileid'] + + exprs = list(args.js) + if args.js_file: + exprs += [b for b in open(args.js_file).read().split('\n%%\n') if b.strip()] + + u, pw = args.user.split(':', 1) + s = Session('P', 9444, u, pw, args.base, args.fileid, args.chromium) + try: + await s.start() + await s.login_and_open() + await s.drain(args.settle) + print(' where:', await s.where()) + await s.eval(DISMISS_JS) + if args.type_text: + await s.type(args.type_text) + await s.drain(8) + for i, e in enumerate(exprs): + if i and args.pause: + await s.drain(args.pause) + print('\n>>>', e.strip().replace('\n', ' ')[:120]) + print(json.dumps(await s.eval(e), ensure_ascii=False)[:4000]) + finally: + await s.stop() + +asyncio.run(main()) diff --git a/tests/rig/provision.sh b/tests/rig/provision.sh new file mode 100755 index 0000000..61b47b5 --- /dev/null +++ b/tests/rig/provision.sh @@ -0,0 +1,119 @@ +#!/bin/bash +# Turn a freshly started rig into one the tests can run against: the connector +# installed, this app enabled, a second user to co-author with, and a document +# of each format to edit - shared with that second user, because without the +# share the second session gets "file not found" and a co-authoring run quietly +# degrades to one user. +# +# What it creates is written to .rig-env, which the tests read, so no file id +# ends up hardcoded in a test. +set -uo pipefail +HERE=$(cd "$(dirname "$0")" && pwd) + +RIG_PROJECT=${RIG_PROJECT:-rig} +APP=${RIG_APP_CONTAINER:-${RIG_PROJECT}-app-1} +PORT=${RIG_PORT:-8099} +BASE=${RIG_BASE:-http://127.0.0.1:${PORT}} +ADMIN_USER=${RIG_ADMIN_USER:-admin} +ADMIN_PASSWORD=${RIG_ADMIN_PASSWORD:-rigadmin} +USER2_NAME=${RIG_USER2_NAME:-bob} +USER2_PASSWORD=${RIG_USER2_PASSWORD:-Kq7-rigTest-3910} +# Pinned rather than "whatever the app store has today": a connector upgrade +# changing how the editor is embedded is a thing to find out about deliberately. +CONNECTOR_VERSION=${RIG_CONNECTOR_VERSION:-10.1.2} + +occ() { docker exec -u www-data "$APP" php occ "$@"; } +in_app() { docker exec "$APP" sh -c "$*"; } + +echo "==> waiting for Nextcloud to finish installing" +for _ in $(seq 1 120); do + if occ status 2>/dev/null | grep -q "installed: true"; then break; fi + sleep 5 +done +occ status | sed 's/^/ /' + +echo "==> installing the ONLYOFFICE connector ${CONNECTOR_VERSION}" +if in_app "test -d /var/www/html/custom_apps/onlyoffice"; then + echo " already there" +else + in_app "curl -fsSL -o /tmp/onlyoffice.tar.gz \ + https://github.com/ONLYOFFICE/onlyoffice-nextcloud/releases/download/v${CONNECTOR_VERSION}/onlyoffice.tar.gz \ + && tar xzf /tmp/onlyoffice.tar.gz -C /var/www/html/custom_apps \ + && chown -R www-data:www-data /var/www/html/custom_apps/onlyoffice \ + && rm -f /tmp/onlyoffice.tar.gz" || exit 1 +fi +occ app:enable onlyoffice | sed 's/^/ /' +occ app:enable documentserver_community | sed 's/^/ /' + +# Point the connector at this app explicitly rather than leaving it to +# auto-configuration: run from occ, that picks the url up from +# overwrite.cli.url, which is not necessarily the host the browser uses - and +# an editor iframe on a different origin than the page cannot be framed by it, +# nor read by the tests. +occ config:app:set onlyoffice DocumentServerUrl --value "${BASE}/apps/documentserver_community/" | sed 's/^/ /' + +echo "==> the app's own setup check" +in_app "php -r ' + \$x2t = \"/var/www/html/custom_apps/documentserver_community/3rdparty/onlyoffice/documentserver/server/FileConverter/bin/x2t\"; + echo file_exists(\$x2t) ? \" x2t present\n\" : \" x2t MISSING - run make in the app directory\n\"; +'" + +echo "==> creating ${USER2_NAME}" +if occ user:info "$USER2_NAME" >/dev/null 2>&1; then + echo " already there" +else + docker exec -u www-data -e OC_PASS="$USER2_PASSWORD" "$APP" \ + php occ user:add --password-from-env "$USER2_NAME" | sed 's/^/ /' +fi + +# The blank documents the bundled server ships for "new file" are the fixtures: +# every format, tiny, and by definition ones this document server can open. +TEMPLATES=/var/www/html/custom_apps/documentserver_community/3rdparty/onlyoffice/documentserver/document-templates/new/en-US +echo "==> uploading a document of each format" +: > "$HERE/.rig-env" +{ + echo "# written by provision.sh, read by tests/rig/lib/config.py" + echo "RIG_BASE=$BASE" + echo "RIG_PROJECT=$RIG_PROJECT" + echo "RIG_ADMIN=${ADMIN_USER}:${ADMIN_PASSWORD}" + echo "RIG_USER2=${USER2_NAME}:${USER2_PASSWORD}" +} >> "$HERE/.rig-env" + +for kind in docx xlsx pptx; do + name="sample.${kind}" + in_app "cp ${TEMPLATES}/new.${kind} /tmp/${name}" || { + echo " FAILED to find ${TEMPLATES}/new.${kind}"; exit 1; } + docker cp "$APP:/tmp/${name}" "/tmp/${name}" >/dev/null + + curl -s -u "${ADMIN_USER}:${ADMIN_PASSWORD}" -T "/tmp/${name}" \ + "${BASE}/remote.php/dav/files/${ADMIN_USER}/${name}" >/dev/null + + # share with the second user, so co-authoring runs have two real + # participants + curl -s -u "${ADMIN_USER}:${ADMIN_PASSWORD}" -X POST \ + -H "OCS-APIRequest: true" \ + -d "path=/${name}" -d "shareType=0" -d "shareWith=${USER2_NAME}" -d "permissions=31" \ + "${BASE}/ocs/v2.php/apps/files_sharing/api/v1/shares" >/dev/null + + fileid=$(curl -s -u "${ADMIN_USER}:${ADMIN_PASSWORD}" -X PROPFIND \ + -H "Depth: 0" -H "Content-Type: text/xml" \ + --data '' \ + "${BASE}/remote.php/dav/files/${ADMIN_USER}/${name}" \ + | grep -o '[0-9]*' | grep -o '[0-9]*' | head -1) + + if [ -z "$fileid" ]; then + echo " FAILED to upload ${name}" + exit 1 + fi + echo " ${name} -> fileid ${fileid}" + echo "RIG_FILEID_$(echo "$kind" | tr '[:lower:]' '[:upper:]')=${fileid}" >> "$HERE/.rig-env" + echo "RIG_FILE_$(echo "$kind" | tr '[:lower:]' '[:upper:]')=${name}" >> "$HERE/.rig-env" + rm -f "/tmp/${name}" +done + +echo "==> document server url the connector will use" +occ config:app:get onlyoffice DocumentServerUrl | sed 's/^/ /' + +echo +echo "provisioned; wrote $HERE/.rig-env" +cat "$HERE/.rig-env" | sed 's/^/ /' diff --git a/tests/rig/rig.sh b/tests/rig/rig.sh new file mode 100755 index 0000000..63bd183 --- /dev/null +++ b/tests/rig/rig.sh @@ -0,0 +1,81 @@ +#!/bin/bash +# The one entry point: bring the rig up, provision it, run the tests, tear it +# down. Everything it does is also doable by hand with docker compose and the +# python scripts; this exists so CI and a laptop run the same commands. +# +# ./rig.sh up start the containers and wait for Nextcloud +# ./rig.sh provision connector, users, documents (writes .rig-env) +# ./rig.sh reset drop per-document state between runs +# ./rig.sh test [names] run the regression suite, or the named tests +# ./rig.sh logs this app's lines from nextcloud.log +# ./rig.sh down throw it all away, volumes included +set -uo pipefail +HERE=$(cd "$(dirname "$0")" && pwd) +cd "$HERE" + +export RIG_PROJECT=${RIG_PROJECT:-rig} +export RIG_PORT=${RIG_PORT:-8099} +export APP_SOURCE=${APP_SOURCE:-$(cd "$HERE/../.." && pwd)} +APP=${RIG_APP_CONTAINER:-${RIG_PROJECT}-app-1} +BASE=${RIG_BASE:-http://127.0.0.1:${RIG_PORT}} + +case "${1:-}" in +up) + if [ ! -d "$APP_SOURCE/3rdparty/onlyoffice/documentserver" ]; then + echo "!! $APP_SOURCE/3rdparty/onlyoffice/documentserver is missing." + echo " Run 'make' in the app directory first; without the document" + echo " server there is nothing to test." + exit 1 + fi + docker compose up -d + echo "==> waiting for $BASE" + for _ in $(seq 1 120); do + code=$(curl -s -o /dev/null -w '%{http_code}' "$BASE/status.php" || true) + if [ "$code" = "200" ]; then + echo " up: $(curl -s "$BASE/status.php")" + exit 0 + fi + sleep 5 + done + echo "!! Nextcloud did not come up" + docker compose logs --tail 40 app + exit 1 + ;; +provision) + exec "$HERE/provision.sh" + ;; +reset) + exec python3 -c " +import sys; sys.path.insert(0, '$HERE/lib') +import harness; harness.reset(); print('rig state reset')" + ;; +test) + shift + exec python3 "$HERE/run_all.py" "$@" + ;; +logs) + docker exec "$APP" sh -c 'cat /var/www/html/data/nextcloud.log' | python3 -c " +import sys, json +for line in sys.stdin: + try: + d = json.loads(line) + except Exception: + continue + app = str(d.get('app')); msg = str(d.get('message')) + if 'documentserver' in app or 'onlyoffice' in app or 'documentserver' in msg.lower(): + lvl = {0:'DEBUG',1:'INFO',2:'WARN',3:'ERROR',4:'FATAL'}.get(d.get('level'), d.get('level')) + print(f'[{lvl}] {app}: {msg[:400]}') + ex = d.get('exception') or {} + if isinstance(ex, dict) and ex.get('Message'): + print(f\" {ex.get('Exception')}: {str(ex.get('Message'))[:300]}\") +" | tail -60 + ;; +down) + docker compose down -v + rm -f "$HERE/.rig-env" + ;; +*) + sed -n '2,12p' "$0" | sed 's/^# \?//' + exit 1 + ;; +esac diff --git a/tests/rig/run_all.py b/tests/rig/run_all.py new file mode 100755 index 0000000..c8680be --- /dev/null +++ b/tests/rig/run_all.py @@ -0,0 +1,85 @@ +#!/usr/bin/env python3 +"""Run the regression suite and say what passed. + +Each test is a script that exits non-zero when its verdict is FAIL, so this is +mostly bookkeeping: run them in a deliberate order - the cheap ones that fail +loudest first - keep the output, and report. + + ./run_all.py the whole suite + ./run_all.py autosave leave only those + ./run_all.py --list what there is +""" +import argparse +import os +import subprocess +import sys +import time + +HERE = os.path.dirname(os.path.abspath(__file__)) + +# name -> (script, args, what it covers) +SUITE = [ + ('smoke', 'smoke_test.py', [], + 'every format opens, edits and reaches the file'), + ('flush-live', 'flush_live_test.py', [], + 'flushing a document somebody is still editing must not empty its change list'), + ('autosave', 'autosave_test.py', [], + 'edits reach the file while editing, with no cron and no flush'), + ('leave', 'leave_test.py', [], + 'closing the editor, and closing the tab, save the document'), + ('twotab', 'twotab_test.py', [], + 'two tabs of one browser, and one of them dropped to view mode'), + ('coedit', 'coedit_test.py', ['--rounds', '2'], + 'two users see each other typing'), + ('coedit-xlsx', 'coedit_test.py', ['--kind', 'xlsx', '--rounds', '1', '--tag', 'X'], + 'the same, in a spreadsheet, where changes carry recalc indexes'), + ('chat', 'chat_test.py', [], + 'the chat panel, including a late joiner reading the backlog'), +] + + +def main(): + p = argparse.ArgumentParser() + p.add_argument('names', nargs='*', help='tests to run (default: all)') + p.add_argument('--list', action='store_true') + p.add_argument('--keep-going', action='store_true', + help='run the rest of the suite after a failure') + args = p.parse_args() + + if args.list: + for name, script, _, what in SUITE: + print(f'{name:12s} {script:22s} {what}') + return 0 + + selected = [t for t in SUITE if not args.names or t[0] in args.names] + unknown = set(args.names) - {t[0] for t in SUITE} + if unknown: + print(f'unknown test(s): {", ".join(sorted(unknown))}') + return 2 + + results = [] + for name, script, extra, what in selected: + print(f'\n{"=" * 72}\n== {name}: {what}\n{"=" * 72}', flush=True) + started = time.time() + code = subprocess.run([sys.executable, os.path.join(HERE, script), *extra]).returncode + took = time.time() - started + results.append((name, code, took)) + print(f'-- {name}: {"PASS" if code == 0 else f"FAIL (exit {code})"} in {took:.0f}s', + flush=True) + if code != 0 and not args.keep_going: + print('-- stopping; pass --keep-going to run the rest') + break + + print(f'\n{"=" * 72}') + for name, code, took in results: + print(f' {"PASS" if code == 0 else "FAIL"} {name:12s} {took:5.0f}s') + failed = [name for name, code, _ in results if code != 0] + skipped = len(selected) - len(results) + if skipped: + print(f' .... {skipped} not run') + print(' SUITE:', 'all green' if not failed else 'FAILED: ' + ', '.join(failed)) + return 0 if not failed else 1 + + +if __name__ == '__main__': + sys.exit(main()) diff --git a/tests/rig/smoke_test.py b/tests/rig/smoke_test.py new file mode 100755 index 0000000..fb8e2a7 --- /dev/null +++ b/tests/rig/smoke_test.py @@ -0,0 +1,159 @@ +#!/usr/bin/env python3 +"""Every format opens, edits, and reaches the file. + +The one test that would have caught most of what broke while this app was being +brought up to a current document server: an editor that never finishes loading +because api.js is a template nobody rendered, one that renders unstyled because +a CSP nonce killed the inline onload that loads its stylesheets, a converter +that cannot find its fonts, appdata on local storage the file cache never heard +about. All of those show up here as an editor that does not open, a JS +exception, a failed request, or text that never reaches the file. + +Per format: open it in a real browser, check the page for exceptions and failed +requests, type a marker, and look for it in the saved file. +""" +import argparse, asyncio, os, sys, time + +sys.path.insert(0, os.path.join(os.path.dirname(os.path.abspath(__file__)), 'lib')) +import config +import harness +from driver import Session, DISMISS_JS, PROBE_JS, TEXT_JS + +# The websocket attempt is expected to fail: PHP cannot upgrade, and the 400 is +# what makes the client fall back to long polling. Nextcloud has no preview for +# a document nobody has generated one for yet, which is a Files app 404 and +# nothing to do with the editor. +EXPECTED_FAILURES = ('transport=websocket', 'ws://', 'wss://', '/core/preview') + + +def interesting_failures(session): + out = [] + for event in session.events: + method, params = event.get('method'), event.get('params', {}) + if method == 'Network.loadingFailed': + text = f"{params.get('type')} {params.get('errorText')} {params.get('requestId')}" + elif method == 'Network.responseReceived': + response = params.get('response', {}) + if response.get('status', 0) < 400: + continue + text = f"HTTP {response.get('status')} {response.get('url')}" + else: + continue + if not any(skip in text for skip in EXPECTED_FAILURES): + out.append(text) + return out + + +def exceptions(session): + out = [] + for event in session.events: + if event.get('method') == 'Runtime.exceptionThrown': + details = event['params'].get('exceptionDetails', {}) + out.append(f"{details.get('text')} @ {details.get('url')}:{details.get('lineNumber')}") + return out + + +async def check_format(kind, args): + document = config.DOCUMENTS[kind] + fileid = document['fileid'] + if not fileid: + raise SystemExit(f'no file id for {kind}: run ./rig.sh provision') + + marker = f"SMOKE{kind.upper()}{int(time.time()) % 100000}" + user, password = config.credentials(args.a) + ok = True + + print(f'\n==> {document["name"]} (fileid {fileid})') + harness.reset() + + session = Session('A', 9222, user, password, args.base, fileid, args.chromium, (420, 400)) + try: + await session.start() + await session.login_and_open() + await session.drain(args.load_wait) + await session.eval(DISMISS_JS) + + state = await session.eval(PROBE_JS) + print(' editor:', state) + if state and 'errorTitle' in str(state) and 'null' not in str(state).split('errorTitle')[1][:20]: + ok = False + print(' FAIL - the editor is showing an error') + + broken = interesting_failures(session) + thrown = exceptions(session) + print(f' failed requests: {len(broken)} | JS exceptions: {len(thrown)}') + for line in (broken + thrown)[:8]: + print(' ', line) + if broken or thrown: + ok = False + print(' FAIL - opening the document was not clean') + + await session.type(f'{marker} ') + await session.drain(args.save_wait) + + typed = str(await session.eval(TEXT_JS)) + if marker not in typed: + ok = False + print(f' FAIL - the marker never reached the editor model: {typed[:120]!r}') + stored = harness.changes() + print(f' changes stored: {stored}') + if stored == 0: + ok = False + print(' FAIL - nothing was sent to the server') + finally: + await session.stop() + + # The browser is gone but its session has not expired yet, and a flush only + # disposes of a document nobody is in - so tell the server what killing the + # browser could not. + harness.drop_sessions() + + code, out = harness.flush() + print(f' flush: exit={code} {out[:60]}') + if code != 0: + ok = False + print(' FAIL - documentserver:flush reported an error') + + found = harness.file_markers(document['name'], document['member'], [marker], args.a) + print(' file:', found) + if not all(found.values()): + ok = False + print(' FAIL - the edit did not reach the saved file') + + left = harness.doc_folders() + if left: + ok = False + print(f' FAIL - {left} document folder(s) left behind after the flush') + + errors = harness.app_log_errors() + if errors: + ok = False + print(' FAIL - the app logged errors:') + for line in errors[:5]: + print(' ', line[:200]) + + return ok + + +async def main(): + p = argparse.ArgumentParser() + p.add_argument('--base', default=config.BASE) + p.add_argument('--a', default=config.ADMIN) + p.add_argument('--chromium', default=config.CHROMIUM) + p.add_argument('--kind', action='append', choices=sorted(config.DOCUMENTS), + help='only this format (repeatable); default is all three') + p.add_argument('--load-wait', type=float, default=30) + p.add_argument('--save-wait', type=float, default=15) + args = p.parse_args() + + kinds = args.kind or ['docx', 'xlsx', 'pptx'] + results = {} + for kind in kinds: + results[kind] = await check_format(kind, args) + + print('\n VERDICT:', ' '.join(f'{k}={"ok" if v else "FAIL"}' for k, v in results.items())) + return 0 if all(results.values()) else 1 + + +if __name__ == '__main__': + sys.exit(asyncio.run(main())) diff --git a/tests/rig/twotab_test.py b/tests/rig/twotab_test.py new file mode 100755 index 0000000..fd81721 --- /dev/null +++ b/tests/rig/twotab_test.py @@ -0,0 +1,117 @@ +#!/usr/bin/env python3 +"""Two tabs of one browser, in one document: does each see the other typing? + +coedit_test.py runs two separate browsers, which share nothing. Two tabs share +a cookie jar - so one Nextcloud PHP session - a connection pool, and any service +worker, and that is what someone opening a second tab on the same document +actually has. + +Each tab types a marker, and then both document models are read back: the test +is whether each tab *shows* what the other typed, not whether the messages were +sent. +""" +import argparse, asyncio, os, subprocess, sys, time + +sys.path.insert(0, os.path.join(os.path.dirname(os.path.abspath(__file__)), 'lib')) +import config +import harness +from driver import Session, Tab, COAUTH_JS, DISMISS_JS, TEXT_JS + + +async def main(): + p = argparse.ArgumentParser() + p.add_argument('--base', default=config.BASE) + p.add_argument('--user', default=config.ADMIN) + p.add_argument('--kind', default='docx', choices=sorted(config.DOCUMENTS)) + p.add_argument('--fileid', default=None) + p.add_argument('--file', default=None) + p.add_argument('--member', default=None) + p.add_argument('--chromium', default=config.CHROMIUM) + p.add_argument('--tag', default='TAB') + p.add_argument('--rounds', type=int, default=2) + p.add_argument('--interval', type=int, default=20) + p.add_argument('--no-reset', action='store_true') + args = p.parse_args() + document = config.document(args.kind) + args.fileid = args.fileid or document['fileid'] + args.file = args.file or document['name'] + args.member = args.member or document['member'] + + u, pw = args.user.split(':', 1) + t = args.tag + str(int(time.time()) % 100000) + ok = True + + if not args.no_reset: + print('==> resetting rig state') + harness.reset() + harness.set_app_config('autosave_interval', args.interval) + time.sleep(harness.APP_CONFIG_PROPAGATION) + + A = Session('A', 9222, u, pw, args.base, args.fileid, args.chromium, (420, 400)) + B = Tab('B', 9222, u, pw, args.base, args.fileid, args.chromium, (420, 470)) + try: + await A.start() + await A.login_and_open() + await A.drain(25) + await A.eval(DISMISS_JS) + print(' tab A:', await A.where()) + + await B.start() + await B.login_and_open() + await B.drain(30) + await B.eval(DISMISS_JS) + print(' tab B:', await B.where()) + print(' sessions in the document:', harness.sessions()) + + for r in range(1, args.rounds + 1): + print(f'\n==> round {r}') + await A.type(f'{t}-A{r} ', row_offset=r * 2) + await A.drain(10); await B.drain(10) + await B.type(f'{t}-B{r} ', row_offset=r * 2 + 1) + await B.drain(10); await A.drain(10) + print(' A co-auth:', await A.eval(COAUTH_JS)) + print(' B co-auth:', await B.eval(COAUTH_JS)) + + print('\n==> tab B is dropped to view mode (what sdkjs does on a licence' + ' verdict, rights change or disconnectEveryone)') + await B.eval("document.querySelector('iframe').contentWindow" + ".Asc.editor.asc_coAuthoringDisconnect()") + await B.drain(15); await A.drain(15) + print(' doc folders:', harness.doc_folders(), '| changes:', harness.changes(), + '| sessions:', harness.sessions()) + if harness.doc_folders() == 0 or harness.changes() == 0: + ok = False + print(' FAIL - the document was written off while a page was still open') + + print(' A types on, and it must still reach the file') + await A.type(f'{t}-A9 ', row_offset=20) + await A.drain(args.interval) + await A.type(f'{t}-A9x ', row_offset=21) # a save past the interval + await A.drain(15) + got = harness.file_markers(args.file, args.member, + [f'{t}-A9']) + print(' file:', got) + if not all(got.values()): + ok = False + print(' FAIL - the session lost its document when the other one went to view mode') + + await A.drain(8); await B.drain(8) + atext, btext = str(await A.eval(TEXT_JS)), str(await B.eval(TEXT_JS)) + wanted = [f'{t}-{who}{r}' for r in range(1, args.rounds + 1) for who in ('A', 'B')] + missA = [m for m in wanted if m not in atext] + missB = [m for m in wanted if m not in btext] + print('\n tab A is missing:', missA or 'nothing') + print(' tab B is missing:', missB or 'nothing') + if missA or missB: + ok = False + print(' FAIL - a tab does not show what the other one typed') + finally: + await B.stop() + await A.stop() + + print('\n VERDICT:', 'two tabs co-author' if ok else 'FAIL') + return 0 if ok else 1 + + +if __name__ == '__main__': + sys.exit(asyncio.run(main())) From ef40b022d4800d7dc45640c325186422f2ce7d00 Mon Sep 17 00:00:00 2001 From: Fabrice Meyer Date: Sun, 6 Sep 2026 11:53:06 -0300 Subject: [PATCH 02/17] Run the rig, and the linters, in CI The point of having the browser tests in the repository is that they run without anybody remembering to run them. Two workflows: * lint - php syntax on the versions the supported Nextcloud releases use, node --check on the injected scripts, and a compile of the rig itself. A minute, on everything. * integration - the rig: build (or restore from cache) the document server, start Nextcloud, provision it, and run the regression suite in a real browser. Runs when something it covers changes, on pull requests, on demand with a choice of tests, and weekly - the weekly run is what turns "the document server released something that breaks the editor" into a red build rather than a support ticket. The document server tree is 1.5 GB and takes a quarter of an hour to build, so it is cached on the release the Makefile pins; the build step only runs when that changes. It also takes a GITHUB_TOKEN now: the release asset is resolved through the API, which is rate limited per IP for anonymous callers, and a runner shares its IP with every other job on the same host. Signed-off-by: Fabrice Meyer Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/integration.yml | 111 ++++++++++++++++++++++++++++++ .github/workflows/lint.yml | 56 +++++++++++++++ Makefile | 3 +- 3 files changed, 169 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/integration.yml create mode 100644 .github/workflows/lint.yml diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml new file mode 100644 index 0000000..c9cc145 --- /dev/null +++ b/.github/workflows/integration.yml @@ -0,0 +1,111 @@ +name: integration + +# Drives real editors against a real Nextcloud with this app in it. Everything +# it runs is what tests/rig/README.md describes, so a failure here can be +# reproduced with the same commands on a laptop. + +on: + push: + paths: + - 'lib/**' + - 'js/**' + - 'appinfo/**' + - 'tests/rig/**' + - 'Makefile' + - '.github/workflows/integration.yml' + pull_request: + workflow_dispatch: + inputs: + tests: + description: 'which tests to run (blank = all; see run_all.py --list)' + required: false + default: '' + schedule: + # the document server tree is rebuilt from upstream weekly, so a release + # that breaks the editor shows up here rather than in production + - cron: '0 4 * * 1' + +permissions: + contents: read + +concurrency: + group: integration-${{ github.ref }} + cancel-in-progress: true + +jobs: + rig: + runs-on: ubuntu-latest + timeout-minutes: 90 + + steps: + - uses: actions/checkout@v4 + + - name: read the document server release out of the Makefile + id: ds + run: | + echo "repo=$(grep -m1 '^ds_repo=' Makefile | cut -d= -f2 | tr '/' '-')" >> "$GITHUB_OUTPUT" + echo "version=$(grep -m1 '^ds_version=' Makefile | cut -d= -f2)" >> "$GITHUB_OUTPUT" + + - name: cache the document server + id: ds-cache + uses: actions/cache@v4 + with: + path: 3rdparty/onlyoffice/documentserver + key: ds-${{ steps.ds.outputs.repo }}-${{ steps.ds.outputs.version }}-${{ hashFiles('Makefile') }} + + - name: build the document server + if: steps.ds-cache.outputs.cache-hit != 'true' + env: + # the release asset is resolved through the API, which is rate + # limited per IP for anonymous callers - shared with every other + # runner on the same host + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + sudo apt-get update -qq + sudo apt-get install -y -qq rpm cpio jq + make 3rdparty/onlyoffice/documentserver + du -sh 3rdparty/onlyoffice/documentserver + + - name: install what drives the browser + run: | + python3 -m pip install --quiet websockets requests + # the runner image ships Chrome; the rig will take either + echo "RIG_CHROMIUM=$(command -v google-chrome || command -v chromium || command -v chromium-browser)" >> "$GITHUB_ENV" + + - name: start the rig + env: + RIG_PORT: 8099 + run: tests/rig/rig.sh up + + - name: provision it + env: + RIG_PORT: 8099 + run: tests/rig/rig.sh provision + + - name: run the regression suite + env: + RIG_PORT: 8099 + run: tests/rig/rig.sh test --keep-going ${{ github.event.inputs.tests }} + + - name: what the app logged + if: always() + env: + RIG_PORT: 8099 + run: tests/rig/rig.sh logs + + - name: screenshots and reports from the failing sessions + if: failure() + uses: actions/upload-artifact@v4 + with: + name: rig-output + path: | + /tmp/coedit/** + /tmp/rig-* + if-no-files-found: ignore + retention-days: 7 + + - name: stop the rig + if: always() + env: + RIG_PORT: 8099 + run: tests/rig/rig.sh down diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml new file mode 100644 index 0000000..0f7b61d --- /dev/null +++ b/.github/workflows/lint.yml @@ -0,0 +1,56 @@ +name: lint + +on: + push: + branches-ignore: [] + pull_request: + workflow_dispatch: + +permissions: + contents: read + +jobs: + php: + name: php ${{ matrix.php }} syntax + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + # what the supported Nextcloud releases run on, and what the servers + # this is deployed to actually have + php: ['8.1', '8.3', '8.4'] + steps: + - uses: actions/checkout@v4 + + - uses: shivammathur/setup-php@v2 + with: + php-version: ${{ matrix.php }} + coverage: none + + - name: lint + run: | + find lib appinfo tests -name '*.php' -not -path './3rdparty/*' -print0 \ + | xargs -0 -n1 -P4 php -l + + javascript: + name: javascript syntax + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: lint + run: for f in js/*.js; do node --check "$f" && echo "ok $f"; done + + python: + name: rig scripts + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: '3.11' + - name: compile + run: python3 -m compileall -q tests/rig + - name: shellcheck + run: | + sudo apt-get update -qq && sudo apt-get install -y -qq shellcheck + shellcheck -S warning tests/rig/*.sh diff --git a/Makefile b/Makefile index 9cf675a..19a56a6 100644 --- a/Makefile +++ b/Makefile @@ -51,7 +51,8 @@ appstore: # one fixed filename per release, while Euro-Office puts the version in the # filename and it does not always match the tag (the v9.3.2 release ships a # 9.3.1-dev.1 rpm). - url=$$(curl -sfL https://api.github.com/repos/$(ds_repo)/releases/tags/$(ds_version) \ + url=$$(curl -sfL $${GITHUB_TOKEN:+-H "Authorization: Bearer $$GITHUB_TOKEN"} \ + https://api.github.com/repos/$(ds_repo)/releases/tags/$(ds_version) \ | grep -o 'https://[^"]*x86_64[.]rpm' | head -1); \ if [ -z "$$url" ]; then echo "no x86_64 rpm asset for $(ds_repo) $(ds_version)" >&2; exit 1; fi; \ echo "fetching $$url"; \ From b4310abc6c113092a76f4629ac64742a6c78965d Mon Sep 17 00:00:00 2001 From: Fabrice Meyer Date: Sun, 6 Sep 2026 11:58:04 -0300 Subject: [PATCH 03/17] Run the tests that do not need a browser, and add the ones that were missing tests/ has had unit tests since 2019 and nothing has ever run them. One of them could not have run: RedisIPCBackendTest called getGetRedisFactory(), a method that does not exist. It skips when there is no Redis now, which is also what an instance without Redis configured should do with it. The phpunit workflow builds a Nextcloud around the app - these tests talk to a real database and a real app container - and runs them. SaveHandlerTest is new, and covers the decisions the browser tests can only observe the consequences of: a flush of a document nobody is in consumes the change list and closes the document; a flush of one somebody is editing writes the file and leaves both alone; a session arriving while the converter is running keeps its changes; a snapshot of a document nobody has typed into since the last one does not run the converter; and the autosave interval is respected, including being turned off. Signed-off-by: Fabrice Meyer Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/phpunit.yml | 72 ++++++++++ tests/Document/SaveHandlerTest.php | 207 +++++++++++++++++++++++++++++ tests/IPC/RedisIPCBackendTest.php | 11 +- tests/phpunit.xml | 21 +++ 4 files changed, 310 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/phpunit.yml create mode 100644 tests/Document/SaveHandlerTest.php create mode 100644 tests/phpunit.xml diff --git a/.github/workflows/phpunit.yml b/.github/workflows/phpunit.yml new file mode 100644 index 0000000..184a779 --- /dev/null +++ b/.github/workflows/phpunit.yml @@ -0,0 +1,72 @@ +name: phpunit + +# The tests that do not need a browser: the session table, the IPC backends, +# and the decisions about when a document's change list may be thrown away. +# They talk to a real Nextcloud, so this builds one around the app. + +on: + push: + paths: + - 'lib/**' + - 'tests/**' + - '!tests/rig/**' + - 'appinfo/**' + - '.github/workflows/phpunit.yml' + pull_request: + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: phpunit-${{ github.ref }} + cancel-in-progress: true + +jobs: + phpunit: + runs-on: ubuntu-latest + timeout-minutes: 30 + + steps: + - name: check out the server this app supports + uses: actions/checkout@v4 + with: + repository: nextcloud/server + # the newest release appinfo/info.xml declares support for + ref: stable34 + submodules: true + path: server + + - name: check out the app into it + uses: actions/checkout@v4 + with: + path: server/apps/documentserver_community + + - uses: shivammathur/setup-php@v2 + with: + php-version: '8.3' + extensions: bcmath, curl, gd, gmp, intl, mbstring, pdo_sqlite, sqlite3, xml, zip + coverage: none + tools: composer + + - name: server dev dependencies (phpunit) + working-directory: server + run: composer install --no-progress --no-interaction + + - name: install nextcloud + working-directory: server + run: | + mkdir -p data + php occ maintenance:install \ + --database sqlite --admin-user admin --admin-pass admin --no-interaction + php occ app:enable --force documentserver_community + php occ app:list | grep -A2 documentserver + + - name: phpunit + working-directory: server + run: vendor/bin/phpunit -c apps/documentserver_community/tests/phpunit.xml + + - name: what the server logged + if: failure() + working-directory: server + run: tail -50 data/nextcloud.log || true diff --git a/tests/Document/SaveHandlerTest.php b/tests/Document/SaveHandlerTest.php new file mode 100644 index 0000000..90be117 --- /dev/null +++ b/tests/Document/SaveHandlerTest.php @@ -0,0 +1,207 @@ + + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OCA\DocumentServer\Tests\Document; + +use OCA\DocumentServer\Channel\SessionManager; +use OCA\DocumentServer\Document\Change; +use OCA\DocumentServer\Document\ChangeStore; +use OCA\DocumentServer\Document\DocumentStore; +use OCA\DocumentServer\Document\SaveHandler; +use OCA\DocumentServer\DocumentConverter; +use OCP\AppFramework\Utility\ITimeFactory; +use OCP\IConfig; +use OCP\Lock\ILockingProvider; +use PHPUnit\Framework\MockObject\MockObject; +use Test\TestCase; + +/** + * When the document gets written, and - the part that cost a lot of documents - + * when its change list may be thrown away. + * + * The change list is the whole record of what was typed: Editor.bin stays at the + * version the document was opened at, and every write replays the list against + * it. So consuming it is only ever safe once nobody is left in the document, and + * these are the decisions that make sure of that. + */ +class SaveHandlerTest extends TestCase { + private const DOCUMENT = 42; + + /** @var DocumentStore|MockObject */ + private $documentStore; + /** @var ChangeStore|MockObject */ + private $changeStore; + /** @var SessionManager|MockObject */ + private $sessionManager; + /** @var IConfig|MockObject */ + private $config; + private $now = 1000000; + /** @var SaveHandler */ + private $saveHandler; + + protected function setUp(): void { + parent::setUp(); + + $this->documentStore = $this->createMock(DocumentStore::class); + $this->changeStore = $this->createMock(ChangeStore::class); + $this->sessionManager = $this->createMock(SessionManager::class); + $this->config = $this->createMock(IConfig::class); + + $timeFactory = $this->createMock(ITimeFactory::class); + $timeFactory->method('getTime')->willReturnCallback(function () { + return $this->now; + }); + + $this->config->method('getAppValue') + ->willReturnCallback(function (string $app, string $key, string $default) { + return $default; + }); + + $this->saveHandler = new SaveHandler( + $this->documentStore, + $this->changeStore, + $this->createMock(DocumentConverter::class), + $this->createMock(ILockingProvider::class), + $this->sessionManager, + $this->config, + $timeFactory + ); + } + + private function change(): Change { + return new Change(self::DOCUMENT, $this->now, 'a change', 'user', 'user', 0); + } + + private function snapshotState(int $index, int $time): array { + return ['index' => $index, 'time' => $time]; + } + + public function testFlushOfADocumentNobodyIsInConsumesItAndClosesIt() { + $this->sessionManager->method('isDocumentActive')->willReturn(false); + $this->changeStore->method('getMaxChangeIndexForDocument')->willReturn(3); + $this->changeStore->method('getChangesAndMarkProcessingForDocument') + ->willReturn([$this->change()]); + $this->documentStore->method('getSnapshotState')->willReturn($this->snapshotState(-1, 0)); + + $this->documentStore->expects($this->once())->method('saveChanges'); + $this->changeStore->expects($this->once())->method('deleteProcessedChanges'); + $this->documentStore->expects($this->once())->method('closeDocument'); + + $this->saveHandler->flushChanges(self::DOCUMENT); + } + + public function testFlushOfADocumentSomebodyIsEditingWritesItWithoutConsumingIt() { + $this->sessionManager->method('isDocumentActive')->willReturn(true); + $this->changeStore->method('getMaxChangeIndexForDocument')->willReturn(3); + $this->changeStore->method('getChangesForDocument')->willReturn([$this->change()]); + $this->documentStore->method('getSnapshotState')->willReturn($this->snapshotState(-1, 0)); + + $this->documentStore->expects($this->once())->method('saveChanges'); + $this->changeStore->expects($this->never())->method('getChangesAndMarkProcessingForDocument'); + $this->changeStore->expects($this->never())->method('deleteProcessedChanges'); + $this->documentStore->expects($this->never())->method('closeDocument'); + + $this->saveHandler->flushChanges(self::DOCUMENT); + } + + public function testASessionArrivingWhileTheDocumentIsAssembledKeepsItsChanges() { + // nobody is in the document when the flush starts, somebody is by the + // time the converter has finished + $this->sessionManager->method('isDocumentActive') + ->willReturnOnConsecutiveCalls(false, true); + $this->changeStore->method('getMaxChangeIndexForDocument')->willReturn(3); + $this->changeStore->method('getChangesAndMarkProcessingForDocument') + ->willReturn([$this->change()]); + $this->documentStore->method('getSnapshotState')->willReturn($this->snapshotState(-1, 0)); + + $this->documentStore->expects($this->once())->method('saveChanges'); + $this->changeStore->expects($this->once())->method('unmarkProcessing'); + $this->changeStore->expects($this->never())->method('deleteProcessedChanges'); + $this->documentStore->expects($this->never())->method('closeDocument'); + + $this->saveHandler->flushChanges(self::DOCUMENT); + } + + public function testASnapshotOfADocumentNobodyHasTypedIntoDoesNotRunTheConverter() { + $this->changeStore->method('getMaxChangeIndexForDocument')->willReturn(7); + $this->documentStore->method('getSnapshotState') + ->willReturn($this->snapshotState(7, $this->now - 3600)); + + $this->documentStore->expects($this->never())->method('saveChanges'); + + $this->assertFalse($this->saveHandler->saveSnapshot(self::DOCUMENT)); + } + + public function testASnapshotWritesTheDocumentAndLeavesTheChangeListAlone() { + $this->changeStore->method('getMaxChangeIndexForDocument')->willReturn(9); + $this->changeStore->method('getChangesForDocument')->willReturn([$this->change()]); + $this->documentStore->method('getSnapshotState') + ->willReturn($this->snapshotState(7, $this->now - 3600)); + + $this->documentStore->expects($this->once())->method('saveChanges'); + $this->documentStore->expects($this->exactly(2))->method('setSnapshotState'); + $this->changeStore->expects($this->never())->method('deleteProcessedChanges'); + $this->documentStore->expects($this->never())->method('closeDocument'); + + $this->assertTrue($this->saveHandler->saveSnapshot(self::DOCUMENT)); + } + + public function testTheDocumentIsNotWrittenAgainInsideTheAutosaveInterval() { + $this->documentStore->method('getSnapshotState')->willReturn($this->snapshotState( + 1, $this->now - (SaveHandler::DEFAULT_AUTOSAVE_INTERVAL - 5))); + + $this->documentStore->expects($this->never())->method('saveChanges'); + + $this->assertFalse($this->saveHandler->saveSnapshotIfDue(self::DOCUMENT)); + } + + public function testTheDocumentIsWrittenOnceTheAutosaveIntervalHasPassed() { + $this->documentStore->method('getSnapshotState')->willReturn($this->snapshotState( + 1, $this->now - (SaveHandler::DEFAULT_AUTOSAVE_INTERVAL + 5))); + $this->changeStore->method('getMaxChangeIndexForDocument')->willReturn(4); + $this->changeStore->method('getChangesForDocument')->willReturn([$this->change()]); + + $this->documentStore->expects($this->once())->method('saveChanges'); + + $this->assertTrue($this->saveHandler->saveSnapshotIfDue(self::DOCUMENT)); + } + + public function testAutosaveCanBeTurnedOff() { + $config = $this->createMock(IConfig::class); + $config->method('getAppValue')->willReturn('0'); + $saveHandler = new SaveHandler( + $this->documentStore, + $this->changeStore, + $this->createMock(DocumentConverter::class), + $this->createMock(ILockingProvider::class), + $this->sessionManager, + $config, + $this->createMock(ITimeFactory::class) + ); + + $this->documentStore->expects($this->never())->method('getSnapshotState'); + $this->documentStore->expects($this->never())->method('saveChanges'); + + $this->assertFalse($saveHandler->saveSnapshotIfDue(self::DOCUMENT)); + } +} diff --git a/tests/IPC/RedisIPCBackendTest.php b/tests/IPC/RedisIPCBackendTest.php index 26d365d..e919989 100644 --- a/tests/IPC/RedisIPCBackendTest.php +++ b/tests/IPC/RedisIPCBackendTest.php @@ -31,7 +31,16 @@ class RedisIPCBackendTest extends BackendTest { private $redis; protected function setupBackend() { - $this->redis = \OC::$server->getGetRedisFactory()->getInstance(); + // Redis is optional: an instance without it configured runs the other + // backends and skips this one, rather than failing on a dependency the + // app itself treats as optional. (This used to call a method that does + // not exist - getGetRedisFactory - so the test could only ever fatal; + // it was never run anywhere.) + $factory = \OCP\Server::get(\OC\RedisFactory::class); + if (!$factory->isAvailable()) { + $this->markTestSkipped('no redis configured'); + } + $this->redis = $factory->getInstance(); } protected function getBackend(): IIPCBackend { diff --git a/tests/phpunit.xml b/tests/phpunit.xml new file mode 100644 index 0000000..bbf3d37 --- /dev/null +++ b/tests/phpunit.xml @@ -0,0 +1,21 @@ + + + + + + . + + + From f391998b47a68efb40b132602c7d2596008c06ab Mon Sep 17 00:00:00 2001 From: Fabrice Meyer Date: Sun, 6 Sep 2026 12:18:40 -0300 Subject: [PATCH 04/17] Start the smoke test from a pristine document, and fix the migration leftovers Two things the first full run of the suite in one go turned up. The sample documents keep whatever earlier runs typed into them, and that is not only about leftover markers: an empty presentation placeholder takes a click and starts editing, while one that already has text takes the same click and selects a shape, so the keystrokes go nowhere. The smoke test puts the blank document provisioning uploaded back before each format, which is what makes it behave like the first time, every time. The other is mine from moving the tests in: autosave_test still reached for a container name that now lives in the harness, so it raised at the very end - after every one of its own checks had passed, which is the most annoying shape a broken test can have. Signed-off-by: Fabrice Meyer Co-Authored-By: Claude Opus 5 (1M context) --- tests/rig/autosave_test.py | 10 +++++----- tests/rig/lib/harness.py | 22 ++++++++++++++++++++++ tests/rig/rig.sh | 2 +- tests/rig/smoke_test.py | 2 ++ 4 files changed, 30 insertions(+), 6 deletions(-) diff --git a/tests/rig/autosave_test.py b/tests/rig/autosave_test.py index f989342..b99ce7a 100755 --- a/tests/rig/autosave_test.py +++ b/tests/rig/autosave_test.py @@ -147,11 +147,11 @@ async def main(): ok = False print(' FAIL - the document was written but its session was not ended') - print('\n===== server log =====') - print(subprocess.run(['docker', 'exec', DOCKER_APP, 'sh', '-c', - "grep -o '\"message\":\"[^\"]*documentserver[^\"]*\"' " - "/var/www/html/data/nextcloud.log | tail -25"], - capture_output=True, text=True).stdout) + errors = harness.app_log_errors() + if errors: + print('\n===== errors the app logged =====') + for line in errors[:5]: + print(' ', line[:200]) print('\n VERDICT:', 'saving during editing works' if ok else 'FAIL') return 0 if ok else 1 diff --git a/tests/rig/lib/harness.py b/tests/rig/lib/harness.py index e659075..c6de18d 100644 --- a/tests/rig/lib/harness.py +++ b/tests/rig/lib/harness.py @@ -69,6 +69,28 @@ def reset(): app_exec(': > /var/www/html/data/nextcloud.log') +TEMPLATES = ('/var/www/html/custom_apps/documentserver_community/3rdparty/onlyoffice' + '/documentserver/document-templates/new/en-US') + + +def restore_document(kind): + """Put the pristine blank document back. + + The sample files keep whatever earlier runs typed into them, which is a + problem beyond leftover markers: an empty presentation placeholder takes a + click and starts editing, one that already has text in it takes the same + click and selects a shape instead. A test that wants to behave like the + first time has to start from the file provisioning uploaded. + """ + document = config.DOCUMENTS[kind] + user, password = config.credentials(config.ADMIN) + app_exec( + f'cp {TEMPLATES}/new.{kind} /tmp/pristine.{kind} && ' + f'curl -s -u {user}:{password} -T /tmp/pristine.{kind} ' + f'"http://localhost/remote.php/dav/files/{user}/{document["name"]}" && ' + f'rm -f /tmp/pristine.{kind}') + + def doc_folders(): """Open documents, ignoring doc_0 - the scratch folder the connector's preview conversions go through, which has nothing to do with editing.""" diff --git a/tests/rig/rig.sh b/tests/rig/rig.sh index 63bd183..a9cba8f 100755 --- a/tests/rig/rig.sh +++ b/tests/rig/rig.sh @@ -11,7 +11,7 @@ # ./rig.sh down throw it all away, volumes included set -uo pipefail HERE=$(cd "$(dirname "$0")" && pwd) -cd "$HERE" +cd "$HERE" || exit 1 export RIG_PROJECT=${RIG_PROJECT:-rig} export RIG_PORT=${RIG_PORT:-8099} diff --git a/tests/rig/smoke_test.py b/tests/rig/smoke_test.py index fb8e2a7..d9e0461 100755 --- a/tests/rig/smoke_test.py +++ b/tests/rig/smoke_test.py @@ -64,6 +64,8 @@ async def check_format(kind, args): ok = True print(f'\n==> {document["name"]} (fileid {fileid})') + # from the blank document, not from whatever the last run typed into it + harness.restore_document(kind) harness.reset() session = Session('A', 9222, user, password, args.base, fileid, args.chromium, (420, 400)) From 6ed351389865761c935532aed2ca39c442bca84a Mon Sep 17 00:00:00 2001 From: Fabrice Meyer Date: Sun, 6 Sep 2026 12:40:00 -0300 Subject: [PATCH 05/17] Make the browser tests reliable enough to gate a merge Three things the first CI run and a full local run turned up, none of them about the app. Typing was not checked. A click sometimes lands where the caret does not follow - below the text in a nearly empty document, on a drawing, on an overlay that was still up - and the keystrokes go nowhere. That is indistinguishable from a change never reaching the server, which is what most of these tests are about, so a test would fail for the wrong reason and send you looking in the wrong place. Typing now reads the editor's model back and tries once more if the text is not in it. The two-tab test asserted too much. After a tab is dropped to view mode, what that tab's model holds is sdkjs's business - going to view mode can roll back changes it had already applied - so the co-authoring assertion moves to before the disconnect, and what follows it is what this test is actually about: the document, its change list and the other session survive, and the other session keeps saving. And the document server cache was thrown away whenever the suite failed, which is the one time you least want to wait another quarter of an hour for the next run: it is saved as soon as it is built now. Signed-off-by: Fabrice Meyer Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/integration.yml | 14 ++++++++++++-- tests/rig/lib/driver.py | 21 ++++++++++++++++++++- tests/rig/twotab_test.py | 24 ++++++++++++++---------- 3 files changed, 46 insertions(+), 13 deletions(-) diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index c9cc145..6fa99e8 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -46,9 +46,9 @@ jobs: echo "repo=$(grep -m1 '^ds_repo=' Makefile | cut -d= -f2 | tr '/' '-')" >> "$GITHUB_OUTPUT" echo "version=$(grep -m1 '^ds_version=' Makefile | cut -d= -f2)" >> "$GITHUB_OUTPUT" - - name: cache the document server + - name: restore the document server id: ds-cache - uses: actions/cache@v4 + uses: actions/cache/restore@v4 with: path: 3rdparty/onlyoffice/documentserver key: ds-${{ steps.ds.outputs.repo }}-${{ steps.ds.outputs.version }}-${{ hashFiles('Makefile') }} @@ -66,6 +66,16 @@ jobs: make 3rdparty/onlyoffice/documentserver du -sh 3rdparty/onlyoffice/documentserver + # saved here rather than at the end of the job: the whole point of the + # cache is the quarter of an hour the build takes, and a job that fails + # its tests would otherwise throw it away and rebuild next time + - name: keep the document server for the next run + if: steps.ds-cache.outputs.cache-hit != 'true' + uses: actions/cache/save@v4 + with: + path: 3rdparty/onlyoffice/documentserver + key: ds-${{ steps.ds.outputs.repo }}-${{ steps.ds.outputs.version }}-${{ hashFiles('Makefile') }} + - name: install what drives the browser run: | python3 -m pip install --quiet websockets requests diff --git a/tests/rig/lib/driver.py b/tests/rig/lib/driver.py index bb897c6..8734b98 100644 --- a/tests/rig/lib/driver.py +++ b/tests/rig/lib/driver.py @@ -234,7 +234,26 @@ async def eval(self, expr): return f"JS error: {res['exceptionDetails'].get('text')}" return res.get('result', {}).get('value') - async def type(self, text, enter=True, row_offset=0): + async def type(self, text, enter=True, row_offset=0, attempts=2): + """Type text into the document, and make sure it went in. + + A click sometimes lands where the caret does not follow - below the + text in a nearly empty document, on a drawing, on an overlay that was + still up - and the keystrokes then go nowhere. That is indistinguishable + from the change never reaching the server, which is what most of these + tests are about, so typing checks the editor's own model and tries once + more rather than leaving a test to fail for the wrong reason. + """ + for attempt in range(1, attempts + 1): + await self._type_once(text, enter=enter, row_offset=row_offset) + await self.drain(3) + if text.strip() in str(await self.eval(TEXT_JS)): + return True + if attempt < attempts: + print(f' {self.name}: the text did not land, typing it again') + return False + + async def _type_once(self, text, enter=True, row_offset=0): for ev in ('mousePressed', 'mouseReleased'): await self.send('Input.dispatchMouseEvent', type=ev, x=self.click[0], y=self.click[1], diff --git a/tests/rig/twotab_test.py b/tests/rig/twotab_test.py index fd81721..3bf290d 100755 --- a/tests/rig/twotab_test.py +++ b/tests/rig/twotab_test.py @@ -72,6 +72,20 @@ async def main(): print(' A co-auth:', await A.eval(COAUTH_JS)) print(' B co-auth:', await B.eval(COAUTH_JS)) + await A.drain(8); await B.drain(8) + atext, btext = str(await A.eval(TEXT_JS)), str(await B.eval(TEXT_JS)) + wanted = [f'{t}-{who}{r}' for r in range(1, args.rounds + 1) for who in ('A', 'B')] + missA = [m for m in wanted if m not in atext] + missB = [m for m in wanted if m not in btext] + print('\n tab A is missing:', missA or 'nothing') + print(' tab B is missing:', missB or 'nothing') + if missA or missB: + ok = False + print(' FAIL - a tab does not show what the other one typed') + + # From here on B is not a participant any more, so what B's model holds + # is sdkjs's business - going to view mode can roll back changes it had + # applied. What matters is what happens to the document and to A. print('\n==> tab B is dropped to view mode (what sdkjs does on a licence' ' verdict, rights change or disconnectEveryone)') await B.eval("document.querySelector('iframe').contentWindow" @@ -95,16 +109,6 @@ async def main(): ok = False print(' FAIL - the session lost its document when the other one went to view mode') - await A.drain(8); await B.drain(8) - atext, btext = str(await A.eval(TEXT_JS)), str(await B.eval(TEXT_JS)) - wanted = [f'{t}-{who}{r}' for r in range(1, args.rounds + 1) for who in ('A', 'B')] - missA = [m for m in wanted if m not in atext] - missB = [m for m in wanted if m not in btext] - print('\n tab A is missing:', missA or 'nothing') - print(' tab B is missing:', missB or 'nothing') - if missA or missB: - ok = False - print(' FAIL - a tab does not show what the other one typed') finally: await B.stop() await A.stop() From 3b91b6997aec6485c376c5a3183273b7d32bd64b Mon Sep 17 00:00:00 2001 From: Fabrice Meyer Date: Sun, 6 Sep 2026 12:42:00 -0300 Subject: [PATCH 06/17] Point the phpunit job at the phpunit the server actually has nextcloud/server keeps its dependencies in lib/composer and its dev tools in vendor-bin, so vendor/bin/phpunit is not a path that exists there; the bin plugin has to install it first. Also gives PHP the apcu extension the server requires. Signed-off-by: Fabrice Meyer Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/phpunit.yml | 11 ++++++++--- tests/phpunit.xml | 2 ++ 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/.github/workflows/phpunit.yml b/.github/workflows/phpunit.yml index 184a779..2512088 100644 --- a/.github/workflows/phpunit.yml +++ b/.github/workflows/phpunit.yml @@ -45,13 +45,18 @@ jobs: - uses: shivammathur/setup-php@v2 with: php-version: '8.3' - extensions: bcmath, curl, gd, gmp, intl, mbstring, pdo_sqlite, sqlite3, xml, zip + extensions: apcu, bcmath, curl, gd, gmp, intl, mbstring, pdo_sqlite, sqlite3, xml, zip + ini-values: apc.enable_cli=1 coverage: none tools: composer - name: server dev dependencies (phpunit) working-directory: server - run: composer install --no-progress --no-interaction + run: | + composer install --no-progress --no-interaction + # the server keeps its own dependencies in lib/composer and its dev + # tools in vendor-bin, so there is no vendor/bin/phpunit + composer bin phpunit install --no-progress --no-interaction - name: install nextcloud working-directory: server @@ -64,7 +69,7 @@ jobs: - name: phpunit working-directory: server - run: vendor/bin/phpunit -c apps/documentserver_community/tests/phpunit.xml + run: vendor-bin/phpunit/vendor/bin/phpunit -c apps/documentserver_community/tests/phpunit.xml - name: what the server logged if: failure() diff --git a/tests/phpunit.xml b/tests/phpunit.xml index bbf3d37..762b924 100644 --- a/tests/phpunit.xml +++ b/tests/phpunit.xml @@ -9,7 +9,9 @@ a database connection, the app container, Test\TestCase. --> From fb1f4b7224da6d49357aa04a31ae3e49413d1475 Mon Sep 17 00:00:00 2001 From: Fabrice Meyer Date: Sun, 6 Sep 2026 12:42:52 -0300 Subject: [PATCH 07/17] Point the README at the tests Somebody arriving at this repository should be told there is a way to find out whether their change works, and where it is. Signed-off-by: Fabrice Meyer Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/README.md b/README.md index 0d2f6c6..c33dd8b 100644 --- a/README.md +++ b/README.md @@ -67,6 +67,24 @@ restorecon -R -v /var/www/html/nextcloud Specific commands and paths will differ based on your specific setup. +## Tests + +`tests/rig` is a disposable Nextcloud with this app in it and a set of +regression tests that drive real editors in headless chromium — they open +documents, type into them, and then ask the editor what it displays and the +saved file what it kept. See its README; the short version is: + + make # once: the document server this all runs against + cd tests/rig + ./rig.sh up && ./rig.sh provision + ./rig.sh test + +`tests/` also holds the unit tests, which run against a Nextcloud checkout: + + vendor-bin/phpunit/vendor/bin/phpunit -c apps/documentserver_community/tests/phpunit.xml + +Both run in CI on every push, along with php, javascript and shell linting. + ## Setup from git When installing from git `make`, `curl`, `rpm2cpio`, and `cpio` are required. From 700dd3f820f66f7e2a585985f1d22d9b132e5159 Mon Sep 17 00:00:00 2001 From: Fabrice Meyer Date: Sun, 6 Sep 2026 12:49:54 -0300 Subject: [PATCH 08/17] Give the app a composer.json, so it brings its own phpunit The way a Nextcloud app is tested now: its own composer.json with phpunit as a dev dependency and a test:unit script, run from a server checkout with the app in apps/. Borrowing the server's phpunit does not work - it keeps its dependencies in lib/composer and its dev tools in vendor-bin, so there is no vendor/bin/phpunit there to reach for. vendor/ and the lock file are ignored, and the release archive excludes them along with composer.json, so building one from a tree somebody has run composer in does not ship dev dependencies. Signed-off-by: Fabrice Meyer Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/phpunit.yml | 20 ++++++++++++-------- .gitignore | 4 +++- Makefile | 3 +++ composer.json | 16 ++++++++++++++++ 4 files changed, 34 insertions(+), 9 deletions(-) create mode 100644 composer.json diff --git a/.github/workflows/phpunit.yml b/.github/workflows/phpunit.yml index 2512088..227beed 100644 --- a/.github/workflows/phpunit.yml +++ b/.github/workflows/phpunit.yml @@ -50,13 +50,13 @@ jobs: coverage: none tools: composer - - name: server dev dependencies (phpunit) + - name: server dependencies working-directory: server - run: | - composer install --no-progress --no-interaction - # the server keeps its own dependencies in lib/composer and its dev - # tools in vendor-bin, so there is no vendor/bin/phpunit - composer bin phpunit install --no-progress --no-interaction + run: composer install --no-progress --no-interaction + + - name: app dev dependencies (phpunit) + working-directory: server/apps/documentserver_community + run: composer install --no-progress --no-interaction - name: install nextcloud working-directory: server @@ -67,9 +67,13 @@ jobs: php occ app:enable --force documentserver_community php occ app:list | grep -A2 documentserver - - name: phpunit + - name: run the tests working-directory: server - run: vendor-bin/phpunit/vendor/bin/phpunit -c apps/documentserver_community/tests/phpunit.xml + # from the server root, which is where a Nextcloud test expects to be, + # with the app's own phpunit - the server keeps its dependencies in + # lib/composer and its dev tools in vendor-bin, so there is no + # vendor/bin/phpunit there to borrow + run: apps/documentserver_community/vendor/bin/phpunit -c apps/documentserver_community/tests/phpunit.xml - name: what the server logged if: failure() diff --git a/.gitignore b/.gitignore index 2f42eec..96c2770 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,6 @@ 3rdparty/onlyoffice build oo-extract -*.rpm \ No newline at end of file +*.rpm +vendor +composer.lock diff --git a/Makefile b/Makefile index 19a56a6..6f10869 100644 --- a/Makefile +++ b/Makefile @@ -38,6 +38,9 @@ appstore: --exclude="../$(app_name)/3rdparty/onlyoffice/documentserver/fonts" \ --exclude="../$(app_name)/build" \ --exclude="../$(app_name)/tests" \ + --exclude="../$(app_name)/vendor" \ + --exclude="../$(app_name)/composer.json" \ + --exclude="../$(app_name)/composer.lock" \ --exclude="../$(app_name)/Makefile" \ --exclude="../$(app_name)/screenshots" \ --exclude="../$(app_name)/.*" \ diff --git a/composer.json b/composer.json new file mode 100644 index 0000000..5407e69 --- /dev/null +++ b/composer.json @@ -0,0 +1,16 @@ +{ + "name": "nextcloud/documentserver_community", + "description": "A document server for Nextcloud, for home users who would rather not run one", + "type": "project", + "license": "AGPL-3.0-or-later", + "config": { + "optimize-autoloader": true, + "sort-packages": true + }, + "require-dev": { + "phpunit/phpunit": "^11.5" + }, + "scripts": { + "test:unit": "phpunit -c tests/phpunit.xml" + } +} From db6f2c4bbd6841c38fac353a137737c320054b9a Mon Sep 17 00:00:00 2001 From: Fabrice Meyer Date: Sun, 6 Sep 2026 12:52:13 -0300 Subject: [PATCH 09/17] Make the unit tests run on a current Nextcloud They were written in 2019 and have not been run since: they ask \OC::$server->getDatabaseConnection() for a database, a method that no longer exists. \OCP\Server::get(IDBConnection::class) is how you get one now. The Redis backend test gets a Redis to talk to in CI rather than being skipped - it is one of the three IPC backends, and the one an instance under load actually uses - and skips properly when there is nothing listening, which the extension being loaded does not tell you. And the abstract base the three backend tests share is excluded from the suite; phpunit 11 warns when its file matches the test suffix, and the workflow treats a warning as a failure. Signed-off-by: Fabrice Meyer Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/phpunit.yml | 17 ++++++++++++++++- tests/Channel/SessionManagerTest.php | 2 +- tests/IPC/DatabaseIPCBackendTest.php | 2 +- tests/IPC/RedisIPCBackendTest.php | 11 ++++++++--- tests/phpunit.xml | 2 ++ 5 files changed, 28 insertions(+), 6 deletions(-) diff --git a/.github/workflows/phpunit.yml b/.github/workflows/phpunit.yml index 227beed..efbc640 100644 --- a/.github/workflows/phpunit.yml +++ b/.github/workflows/phpunit.yml @@ -27,6 +27,19 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 30 + services: + # one of the three IPC backends, and the one an instance under load + # actually uses; without it those tests only ever skip + redis: + image: redis:7 + ports: + - 6379:6379 + options: >- + --health-cmd "redis-cli ping" + --health-interval 5s + --health-timeout 3s + --health-retries 10 + steps: - name: check out the server this app supports uses: actions/checkout@v4 @@ -45,7 +58,7 @@ jobs: - uses: shivammathur/setup-php@v2 with: php-version: '8.3' - extensions: apcu, bcmath, curl, gd, gmp, intl, mbstring, pdo_sqlite, sqlite3, xml, zip + extensions: apcu, bcmath, curl, gd, gmp, intl, mbstring, pdo_sqlite, redis, sqlite3, xml, zip ini-values: apc.enable_cli=1 coverage: none tools: composer @@ -65,6 +78,8 @@ jobs: php occ maintenance:install \ --database sqlite --admin-user admin --admin-pass admin --no-interaction php occ app:enable --force documentserver_community + php occ config:system:set redis host --value 127.0.0.1 + php occ config:system:set redis port --value 6379 --type integer php occ app:list | grep -A2 documentserver - name: run the tests diff --git a/tests/Channel/SessionManagerTest.php b/tests/Channel/SessionManagerTest.php index 50685aa..6edd257 100644 --- a/tests/Channel/SessionManagerTest.php +++ b/tests/Channel/SessionManagerTest.php @@ -49,7 +49,7 @@ class SessionManagerTest extends TestCase { protected function setUp(): void { parent::setUp(); - $this->connection = \OC::$server->getDatabaseConnection(); + $this->connection = \OCP\Server::get(IDBConnection::class); $this->timeFactory = $this->createMock(ITimeFactory::class); $this->ipcFactory = $this->createMock(IIPCFactory::class); $this->timeFactory->method('getTime') diff --git a/tests/IPC/DatabaseIPCBackendTest.php b/tests/IPC/DatabaseIPCBackendTest.php index 60cafc1..5453a04 100644 --- a/tests/IPC/DatabaseIPCBackendTest.php +++ b/tests/IPC/DatabaseIPCBackendTest.php @@ -38,7 +38,7 @@ class DatabaseIPCBackendTest extends BackendTest { private $timeFactory; protected function setupBackend() { - $this->connection = \OC::$server->getDatabaseConnection(); + $this->connection = \OCP\Server::get(IDBConnection::class); $this->timeFactory = $this->createMock(ITimeFactory::class); $this->timeFactory->method('getTime') ->willReturn(100); diff --git a/tests/IPC/RedisIPCBackendTest.php b/tests/IPC/RedisIPCBackendTest.php index e919989..e9bc62a 100644 --- a/tests/IPC/RedisIPCBackendTest.php +++ b/tests/IPC/RedisIPCBackendTest.php @@ -37,10 +37,15 @@ protected function setupBackend() { // not exist - getGetRedisFactory - so the test could only ever fatal; // it was never run anywhere.) $factory = \OCP\Server::get(\OC\RedisFactory::class); - if (!$factory->isAvailable()) { - $this->markTestSkipped('no redis configured'); + try { + if (!$factory->isAvailable()) { + $this->markTestSkipped('no redis configured'); + } + $this->redis = $factory->getInstance(); + } catch (\Exception $e) { + // the extension can be there with nothing listening + $this->markTestSkipped('redis is not reachable: ' . $e->getMessage()); } - $this->redis = $factory->getInstance(); } protected function getBackend(): IIPCBackend { diff --git a/tests/phpunit.xml b/tests/phpunit.xml index 762b924..bb4ffad 100644 --- a/tests/phpunit.xml +++ b/tests/phpunit.xml @@ -18,6 +18,8 @@ . + + IPC/BackendTest.php From 0cf5f8604efa111bdd0b5593b68a4346f6c6fad6 Mon Sep 17 00:00:00 2001 From: Fabrice Meyer Date: Sun, 6 Sep 2026 12:54:19 -0300 Subject: [PATCH 10/17] Fix two assertions that had been wrong since nobody ran them One asserted that the same getter returns both the user id and the display name; the display name has had its own column, and its own getter, all along. The other assumed a session expiry of over eighty seconds, which was true when it was written and has been thirty for years - so it expected a session that is now long expired to survive. It works from the constant now. Signed-off-by: Fabrice Meyer Co-Authored-By: Claude Opus 5 (1M context) --- tests/Channel/SessionManagerTest.php | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/tests/Channel/SessionManagerTest.php b/tests/Channel/SessionManagerTest.php index 6edd257..c646120 100644 --- a/tests/Channel/SessionManagerTest.php +++ b/tests/Channel/SessionManagerTest.php @@ -82,7 +82,8 @@ public function testNewGet() { $this->assertEquals('foo', $session->getSessionId()); $this->assertEquals(5, $session->getDocumentId()); $this->assertEquals('user', $session->getUser()); - $this->assertEquals('name', $session->getUser()); + // the display name, which is a different column and a different getter + $this->assertEquals('name', $session->getUserName()); $this->assertEquals('original', $session->getUserOriginal()); $this->assertEquals(10, $session->getLastSeen()); } @@ -118,16 +119,23 @@ public function testLastSeen() { } public function testCleanSessions() { + // relative to the timeout rather than to numbers that were right when + // it was longer: the one that stops being seen for longer than the + // timeout goes, the one that was seen since stays + $timeout = SessionManager::EXPIRED_SESSION_TIMEOUT; + $this->time = 10; $this->manager->newSession('foo', 5); - $this->time = 50; + $this->time = 10 + $timeout; $this->manager->newSession('bar', 5); $this->assertNotNull($this->manager->getSession('foo')); $this->assertNotNull($this->manager->getSession('bar')); - $this->time = 130; + // half a timeout later: foo has not been seen for one and a half, bar + // for half + $this->time = 10 + $timeout + intdiv($timeout, 2); $this->manager->cleanSessions(); $this->assertNull($this->manager->getSession('foo')); From b0c727a3e3c3993069b4e9beb3c987e268e5bc8e Mon Sep 17 00:00:00 2001 From: Fabrice Meyer Date: Sun, 6 Sep 2026 13:11:40 -0300 Subject: [PATCH 11/17] Tidy: attributes for the test groups, and imports the driver no longer needs phpunit 11 deprecates group metadata in doc comments, and the driver kept the imports it had when it was one script rather than a module. Signed-off-by: Fabrice Meyer Co-Authored-By: Claude Opus 5 (1M context) --- tests/Channel/SessionManagerTest.php | 5 ++--- tests/IPC/DatabaseIPCBackendTest.php | 5 ++--- tests/rig/lib/driver.py | 6 +----- 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/tests/Channel/SessionManagerTest.php b/tests/Channel/SessionManagerTest.php index c646120..3e24eb1 100644 --- a/tests/Channel/SessionManagerTest.php +++ b/tests/Channel/SessionManagerTest.php @@ -28,12 +28,11 @@ use OCA\DocumentServer\Channel\SessionManager; use OCP\AppFramework\Utility\ITimeFactory; use OCP\IDBConnection; +use PHPUnit\Framework\Attributes\Group; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; -/** - * @group DB - */ +#[Group('DB')] class SessionManagerTest extends TestCase { /** @var IDBConnection */ private $connection; diff --git a/tests/IPC/DatabaseIPCBackendTest.php b/tests/IPC/DatabaseIPCBackendTest.php index 5453a04..09082ea 100644 --- a/tests/IPC/DatabaseIPCBackendTest.php +++ b/tests/IPC/DatabaseIPCBackendTest.php @@ -27,10 +27,9 @@ use OCA\DocumentServer\IPC\DatabaseIPCBackend; use OCP\AppFramework\Utility\ITimeFactory; use OCP\IDBConnection; +use PHPUnit\Framework\Attributes\Group; -/** - * @group DB - */ +#[Group('DB')] class DatabaseIPCBackendTest extends BackendTest { /** @var IDBConnection */ private $connection; diff --git a/tests/rig/lib/driver.py b/tests/rig/lib/driver.py index 8734b98..7425b8b 100644 --- a/tests/rig/lib/driver.py +++ b/tests/rig/lib/driver.py @@ -5,14 +5,10 @@ the editor's own document model instead, which is what TEXT_JS and COAUTH_JS are for. """ -import asyncio, base64, json, os, re, shutil, subprocess, sys, tempfile, time +import asyncio, base64, json, re, shutil, subprocess, tempfile, time import urllib.request import websockets -sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) -import config - - LOGIN_JS = r"""(() => { const u = document.querySelector('input[name=user], input#user, input[autocomplete="username"]'); const p = document.querySelector('input[name=password], input#password, input[type=password]'); From e04bb5c9d7df5c9016f34bee472b40ea0ccd046b Mon Sep 17 00:00:00 2001 From: Fabrice Meyer Date: Sun, 6 Sep 2026 13:22:19 -0300 Subject: [PATCH 12/17] Judge the smoke test on the editor's own requests Its "opening the document was not clean" check counted every failed request the page made, and the page is Nextcloud's: a preview that has not been generated for a new document, a user with no status set. Neither says anything about whether the editor came up, and CI failed on the second one. It looks at requests to this app and to the connector now, which is what the test is about, and the websocket 400 stays excluded - PHP cannot upgrade, and that 400 is what makes the client fall back to long polling. Signed-off-by: Fabrice Meyer Co-Authored-By: Claude Opus 5 (1M context) --- tests/rig/smoke_test.py | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/tests/rig/smoke_test.py b/tests/rig/smoke_test.py index d9e0461..7f0d617 100755 --- a/tests/rig/smoke_test.py +++ b/tests/rig/smoke_test.py @@ -19,26 +19,41 @@ import harness from driver import Session, DISMISS_JS, PROBE_JS, TEXT_JS +# What the editor and the connector ask for. The page around them is +# Nextcloud's, and its own requests failing - no preview generated for a +# document yet, no status set for the user - says nothing about whether the +# editor came up. +OURS = ('/apps/documentserver_community/', '/apps/onlyoffice/') + # The websocket attempt is expected to fail: PHP cannot upgrade, and the 400 is -# what makes the client fall back to long polling. Nextcloud has no preview for -# a document nobody has generated one for yet, which is a Files app 404 and -# nothing to do with the editor. -EXPECTED_FAILURES = ('transport=websocket', 'ws://', 'wss://', '/core/preview') +# what makes the client fall back to long polling. +EXPECTED_FAILURES = ('transport=websocket', 'ws://', 'wss://') def interesting_failures(session): + """Requests the editor made that failed.""" + urls = {} + for event in session.events: + if event.get('method') == 'Network.requestWillBeSent': + urls[event['params'].get('requestId')] = \ + event['params'].get('request', {}).get('url', '') + out = [] for event in session.events: method, params = event.get('method'), event.get('params', {}) if method == 'Network.loadingFailed': - text = f"{params.get('type')} {params.get('errorText')} {params.get('requestId')}" + url = urls.get(params.get('requestId'), '') + text = f"{params.get('type')} {params.get('errorText')} {url}" elif method == 'Network.responseReceived': response = params.get('response', {}) if response.get('status', 0) < 400: continue - text = f"HTTP {response.get('status')} {response.get('url')}" + url = response.get('url', '') + text = f"HTTP {response.get('status')} {url}" else: continue + if not any(mine in url for mine in OURS): + continue if not any(skip in text for skip in EXPECTED_FAILURES): out.append(text) return out From 1b481883bf42dae990c6f974058e4a081a1cb8ce Mon Sep 17 00:00:00 2001 From: Fabrice Meyer Date: Sun, 6 Sep 2026 13:24:35 -0300 Subject: [PATCH 13/17] Cover the one recent fix nothing was testing: the image import guard Importing an image is a request the server makes on the client's say-so - the url is whatever the editor put in the command - so it was a way to make this server read local files and reach services on its own network. Nextcloud's HTTP client is what closes most of that, but it hands anything that is not http to curl, so the scheme check is ours and worth holding onto: file://, gopher://, ftp:// and a bare path are refused before a client is even asked for, an https url goes through Nextcloud's client, and a fetch that fails is reported to the editor rather than thrown. Signed-off-by: Fabrice Meyer Co-Authored-By: Claude Opus 5 (1M context) --- tests/XHRCommand/OpenDocumentTest.php | 155 ++++++++++++++++++++++++++ 1 file changed, 155 insertions(+) create mode 100644 tests/XHRCommand/OpenDocumentTest.php diff --git a/tests/XHRCommand/OpenDocumentTest.php b/tests/XHRCommand/OpenDocumentTest.php new file mode 100644 index 0000000..39fde17 --- /dev/null +++ b/tests/XHRCommand/OpenDocumentTest.php @@ -0,0 +1,155 @@ + + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OCA\DocumentServer\Tests\XHRCommand; + +use OCA\DocumentServer\Channel\Session; +use OCA\DocumentServer\Document\DocumentStore; +use OCA\DocumentServer\IPC\IIPCChannel; +use OCA\DocumentServer\OnlyOffice\URLDecoder; +use OCA\DocumentServer\XHRCommand\OpenDocument; +use OCP\Http\Client\IClient; +use OCP\Http\Client\IClientService; +use OCP\ISession; +use OCP\IURLGenerator; +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\LoggerInterface; +use Test\TestCase; + +/** + * Importing an image is a request the server makes on the client's say-so: the + * url is whatever the editor put in the command. Nextcloud's HTTP client is + * what keeps that from reaching the server's own network - it refuses private + * addresses and re-checks them on redirect - but it hands anything that is not + * http to curl, so the scheme is ours to check. + */ +class OpenDocumentTest extends TestCase { + /** @var IClientService|MockObject */ + private $clientService; + /** @var DocumentStore|MockObject */ + private $documentStore; + /** @var OpenDocument */ + private $handler; + + protected function setUp(): void { + parent::setUp(); + + $this->clientService = $this->createMock(IClientService::class); + $this->documentStore = $this->createMock(DocumentStore::class); + + $urlGenerator = $this->createMock(IURLGenerator::class); + $urlGenerator->method('linkToRouteAbsolute')->willReturn('https://cloud.example.com/image'); + + $this->handler = new OpenDocument( + $urlGenerator, + $this->documentStore, + $this->createMock(URLDecoder::class), + $this->createMock(ISession::class), + $this->clientService, + $this->createMock(LoggerInterface::class) + ); + } + + private function session(): Session { + return new Session('sid', 5, 'user', 'user', 'user', 100, false, 1); + } + + /** + * @return array{IIPCChannel|MockObject, array} the channel, and the reply + * pushed to it + */ + private function importImages(array $urls): array { + $reply = []; + $channel = $this->createMock(IIPCChannel::class); + $channel->method('pushMessage')->willReturnCallback(function (string $message) use (&$reply) { + $reply = json_decode($message, true); + }); + + $this->handler->imageUrls( + ['data' => $urls], + $this->session(), + $channel, + $this->createMock(IIPCChannel::class) + ); + + return [$channel, $reply]; + } + + public static function unsupportedSchemes(): array { + return [ + ['file:///etc/passwd'], + ['file://localhost/etc/passwd'], + ['gopher://example.com/'], + ['ftp://example.com/image.png'], + ['/etc/passwd'], + ]; + } + + #[DataProvider('unsupportedSchemes')] + public function testAnImageIsNotFetchedOverSomethingOtherThanHttp(string $url) { + // the guard is before the client: it is never asked for one + $this->clientService->expects($this->never())->method('newClient'); + $this->documentStore->expects($this->never())->method('saveDocumentFile'); + + [, $reply] = $this->importImages([$url]); + + $this->assertEquals(-104, $reply['data']['data']['error']); + $this->assertEquals('error', $reply['data']['data']['urls'][0]['url']); + } + + public function testAnHttpImageIsFetchedThroughNextcloudsClient() { + $body = fopen('php://memory', 'r+'); + fwrite($body, 'an image'); + rewind($body); + + $response = $this->createMock(\OCP\Http\Client\IResponse::class); + $response->method('getBody')->willReturn($body); + + $client = $this->createMock(IClient::class); + $client->expects($this->once()) + ->method('get') + ->with('https://example.com/image.png', ['stream' => true]) + ->willReturn($response); + $this->clientService->method('newClient')->willReturn($client); + + $this->documentStore->expects($this->once())->method('saveDocumentFile'); + + [, $reply] = $this->importImages(['https://example.com/image.png']); + + $this->assertEquals(0, $reply['data']['data']['error']); + $this->assertEquals('https://cloud.example.com/image', $reply['data']['data']['urls'][0]['url']); + } + + public function testAFailedFetchIsReportedRatherThanThrown() { + $client = $this->createMock(IClient::class); + $client->method('get')->willThrowException(new \Exception('no')); + $this->clientService->method('newClient')->willReturn($client); + + $this->documentStore->expects($this->never())->method('saveDocumentFile'); + + [, $reply] = $this->importImages(['https://example.com/image.png']); + + $this->assertEquals(-104, $reply['data']['data']['error']); + } +} From af8e3f595774c47c88b5ff984f17eef818e0d9e0 Mon Sep 17 00:00:00 2001 From: Fabrice Meyer Date: Sun, 6 Sep 2026 18:29:34 -0300 Subject: [PATCH 14/17] Start a browser again when it dies on the way up A browser that answers on the debugging port can still be on its way out: the page target is there, the connection is accepted, and the process is gone by the first command - which is how a forty minute suite run lost its last two tests to a chromium that never got going. Starting a second one is cheap. The teardown had to stop assuming the browser got that far, since it is also the cleanup after a failed start. Signed-off-by: Fabrice Meyer Co-Authored-By: Claude Opus 5 (1M context) --- tests/rig/lib/driver.py | 44 +++++++++++++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 8 deletions(-) diff --git a/tests/rig/lib/driver.py b/tests/rig/lib/driver.py index 7425b8b..e56eb59 100644 --- a/tests/rig/lib/driver.py +++ b/tests/rig/lib/driver.py @@ -185,7 +185,28 @@ async def drain(self, seconds): except Exception: break - async def start(self): + async def start(self, attempts=2): + """Launch a browser and attach to it. + + A browser that answers on the debugging port can still be on its way + out - a page target exists, the connection is accepted, and the process + is gone by the first command. Starting again is cheap and a suite that + loses forty minutes to it is not. + """ + for attempt in range(1, attempts + 1): + try: + await self._start_once() + # a command it has to answer, rather than trusting the socket + await self.send('Runtime.evaluate', expression='1', returnByValue=True) + return + except Exception as e: + print(f' {self.name}: browser did not come up ({e})') + await self.stop() + if attempt == attempts: + raise + await asyncio.sleep(2) + + async def _start_once(self): self.profile = tempfile.mkdtemp(prefix=f'cdp-{self.name}-') self.proc = subprocess.Popen([ self.chromium, '--headless=new', '--no-sandbox', '--disable-gpu', @@ -304,16 +325,23 @@ async def shot(self, path): f.write(base64.b64decode(img['data'])) async def stop(self): + # also the cleanup after a browser that failed to start, so none of + # this may assume it got that far try: await self.conn.close() except Exception: pass - self.proc.terminate() - try: - self.proc.wait(timeout=10) - except Exception: - self.proc.kill() - shutil.rmtree(self.profile, ignore_errors=True) + proc = getattr(self, 'proc', None) + if proc: + proc.terminate() + try: + proc.wait(timeout=10) + except Exception: + proc.kill() + profile = getattr(self, 'profile', None) + if profile: + shutil.rmtree(profile, ignore_errors=True) + class Tab(Session): """A second tab in a browser that is already running. @@ -324,7 +352,7 @@ class Tab(Session): two-browser tests cannot reach. """ - async def start(self): + async def _start_once(self): req = urllib.request.Request( f'http://127.0.0.1:{self.port}/json/new?url=about:blank', method='PUT') info = json.load(urllib.request.urlopen(req)) From e6b9532142dafd45e3d9863e8ba9202a2025b51c Mon Sep 17 00:00:00 2001 From: Fabrice Meyer Date: Sun, 6 Sep 2026 18:34:32 -0300 Subject: [PATCH 15/17] Don't run the whole suite twice for one pull request A push and the pull request whose head it is are two different refs, so keying the concurrency group on the ref let both run - thirty-five minutes of browser tests, twice, for one change. Keyed on the branch they collapse into one, and lint gets a group too. Signed-off-by: Fabrice Meyer Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/integration.yml | 5 ++++- .github/workflows/lint.yml | 4 ++++ .github/workflows/phpunit.yml | 5 ++++- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index 6fa99e8..0966552 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -29,7 +29,10 @@ permissions: contents: read concurrency: - group: integration-${{ github.ref }} + # the branch name, not the ref: a push is refs/heads/x and the pull request + # whose head it is is refs/pull/n/merge, and both would otherwise run the + # whole thing + group: integration-${{ github.head_ref || github.ref_name }} cancel-in-progress: true jobs: diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 0f7b61d..1973443 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -9,6 +9,10 @@ on: permissions: contents: read +concurrency: + group: lint-${{ github.head_ref || github.ref_name }} + cancel-in-progress: true + jobs: php: name: php ${{ matrix.php }} syntax diff --git a/.github/workflows/phpunit.yml b/.github/workflows/phpunit.yml index efbc640..5d2fa31 100644 --- a/.github/workflows/phpunit.yml +++ b/.github/workflows/phpunit.yml @@ -19,7 +19,10 @@ permissions: contents: read concurrency: - group: phpunit-${{ github.ref }} + # the branch name, not the ref: a push is refs/heads/x and the pull request + # whose head it is is refs/pull/n/merge, and both would otherwise run the + # whole thing + group: phpunit-${{ github.head_ref || github.ref_name }} cancel-in-progress: true jobs: From aeac242fcffc680f18a8b869d59b72f54b4474ab Mon Sep 17 00:00:00 2001 From: Fabrice Meyer Date: Sun, 6 Sep 2026 19:03:03 -0300 Subject: [PATCH 16/17] Wait for a change to propagate rather than assuming how long it takes The two co-authoring assertions read both document models after a fixed drain, and every so often one of them was a second early: the change had been sent, stored and broadcast - the receiving session's lastOtherSaveTime had already moved - but the text was not in the model yet, and the test called that a failure to propagate. What they are asking is whether the other session ends up showing it, so they poll for it now, up to forty-five seconds. A test of an asynchronous protocol that asserts on a fixed wait is testing the machine it runs on. Signed-off-by: Fabrice Meyer Co-Authored-By: Claude Opus 5 (1M context) --- tests/rig/coedit_test.py | 9 +++------ tests/rig/lib/driver.py | 18 ++++++++++++++++++ tests/rig/twotab_test.py | 8 +++----- 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/tests/rig/coedit_test.py b/tests/rig/coedit_test.py index d32bcbb..3442077 100755 --- a/tests/rig/coedit_test.py +++ b/tests/rig/coedit_test.py @@ -9,7 +9,7 @@ sys.path.insert(0, os.path.join(os.path.dirname(os.path.abspath(__file__)), 'lib')) import config -from driver import Session, COAUTH_JS, DISMISS_JS, PROBE_JS, TEXT_JS +from driver import Session, wait_for_markers, COAUTH_JS, DISMISS_JS, PROBE_JS, TEXT_JS async def main(): @@ -67,13 +67,10 @@ async def main(): print(f' round {r} B co-auth:', await B.eval(COAUTH_JS)) print('\n===== LIVE PROPAGATION =====') - text = {} - for s_ in (A, B): - text[s_.name] = await s_.eval(TEXT_JS) - print(f' {s_.name} sees: {(text[s_.name] or "")[:220]!r}') ok = True for s_ in (A, B): - missing = [m for m in markers if m not in (text[s_.name] or '')] + missing = await wait_for_markers(s_, markers) + print(f' {s_.name} sees: {str(await s_.eval(TEXT_JS))[:220]!r}') if missing: ok = False print(f' {s_.name}: missing markers {missing}' if missing diff --git a/tests/rig/lib/driver.py b/tests/rig/lib/driver.py index e56eb59..c404f29 100644 --- a/tests/rig/lib/driver.py +++ b/tests/rig/lib/driver.py @@ -136,6 +136,24 @@ })()""" +async def wait_for_markers(session, markers, timeout=45): + """Wait until a session's document model shows every one of these. + + Co-authoring is asynchronous - a change is sent, stored, broadcast, applied, + and how long that takes depends on how busy the browser is - so the question + is whether the other session ends up showing it, not whether it did within + some fixed wait. Returns what is still missing when the time is up. + """ + deadline = time.time() + timeout + missing = list(markers) + while True: + text = str(await session.eval(TEXT_JS)) + missing = [m for m in markers if m not in text] + if not missing or time.time() >= deadline: + return missing + await session.drain(3) + + class Session: def __init__(self, name, port, user, password, base, fileid, chromium, click=(420, 400), cell=(20, 60)): diff --git a/tests/rig/twotab_test.py b/tests/rig/twotab_test.py index 3bf290d..6873af7 100755 --- a/tests/rig/twotab_test.py +++ b/tests/rig/twotab_test.py @@ -15,7 +15,7 @@ sys.path.insert(0, os.path.join(os.path.dirname(os.path.abspath(__file__)), 'lib')) import config import harness -from driver import Session, Tab, COAUTH_JS, DISMISS_JS, TEXT_JS +from driver import Session, Tab, wait_for_markers, COAUTH_JS, DISMISS_JS, TEXT_JS async def main(): @@ -72,11 +72,9 @@ async def main(): print(' A co-auth:', await A.eval(COAUTH_JS)) print(' B co-auth:', await B.eval(COAUTH_JS)) - await A.drain(8); await B.drain(8) - atext, btext = str(await A.eval(TEXT_JS)), str(await B.eval(TEXT_JS)) wanted = [f'{t}-{who}{r}' for r in range(1, args.rounds + 1) for who in ('A', 'B')] - missA = [m for m in wanted if m not in atext] - missB = [m for m in wanted if m not in btext] + missA = await wait_for_markers(A, wanted) + missB = await wait_for_markers(B, wanted) print('\n tab A is missing:', missA or 'nothing') print(' tab B is missing:', missB or 'nothing') if missA or missB: From 5a34f0c8ec091b2379ef0c9e33e94a568e133acd Mon Sep 17 00:00:00 2001 From: Fabrice Meyer Date: Sun, 6 Sep 2026 19:32:27 -0300 Subject: [PATCH 17/17] Run on master and on pull requests, not both for the same commit A branch push and the pull request whose head it is are two events for one commit. The concurrency group cancels the loser, but a cancelled run still shows up on the pull request as a failed check - so the branch trigger goes, and pushes to master keep theirs. Signed-off-by: Fabrice Meyer Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/integration.yml | 3 +++ .github/workflows/lint.yml | 4 +++- .github/workflows/phpunit.yml | 3 +++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index 0966552..18454d5 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -5,7 +5,10 @@ name: integration # reproduced with the same commands on a laptop. on: + # branches are covered by pull_request; running both would run everything + # twice for one change and leave a cancelled run on the pull request push: + branches: [master] paths: - 'lib/**' - 'js/**' diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 1973443..abb9361 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -1,8 +1,10 @@ name: lint on: + # branches are covered by pull_request; running both would run everything + # twice for one change and leave a cancelled run on the pull request push: - branches-ignore: [] + branches: [master] pull_request: workflow_dispatch: diff --git a/.github/workflows/phpunit.yml b/.github/workflows/phpunit.yml index 5d2fa31..d25e109 100644 --- a/.github/workflows/phpunit.yml +++ b/.github/workflows/phpunit.yml @@ -5,7 +5,10 @@ name: phpunit # They talk to a real Nextcloud, so this builds one around the app. on: + # branches are covered by pull_request; running both would run everything + # twice for one change and leave a cancelled run on the pull request push: + branches: [master] paths: - 'lib/**' - 'tests/**'