Repository navigation
Test rig and ci - #397
Merged
Merged
Test rig and ci#397
Conversation
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 <meyer.fabrice@gmx.fr> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 <meyer.fabrice@gmx.fr> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…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 <meyer.fabrice@gmx.fr> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…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 <meyer.fabrice@gmx.fr> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 <meyer.fabrice@gmx.fr> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 <meyer.fabrice@gmx.fr> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 <meyer.fabrice@gmx.fr> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 <meyer.fabrice@gmx.fr> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 <meyer.fabrice@gmx.fr> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 <meyer.fabrice@gmx.fr> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r 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 <meyer.fabrice@gmx.fr> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 <meyer.fabrice@gmx.fr> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 <meyer.fabrice@gmx.fr> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 <meyer.fabrice@gmx.fr> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 <meyer.fabrice@gmx.fr> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
chadek
force-pushed
the
test/rig-and-ci
branch
from
September 6, 2026 21:35
82a9113 to
e6b9532
Compare
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 <meyer.fabrice@gmx.fr> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 <meyer.fabrice@gmx.fr> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adding testing rig and regression tests