test: add server-free unit tests for note, path and cursor logic - #1966
Open
karlitschek wants to merge 2 commits into
Open
test: add server-free unit tests for note, path and cursor logic#1966karlitschek wants to merge 2 commits into
karlitschek wants to merge 2 commits into
Conversation
The suite so far has been integration-only: everything under tests/api/ boots a real Nextcloud, so there was no way to exercise the app's pure logic without a server, and no test at all covered the functions that turn user input into file names. composer.json already declared a `test:unit` script pointing at tests/unit/phpunit.xml, but neither the config nor PHPUnit itself was present. This makes that script real: * tests/unit/phpunit.xml + bootstrap.php — no server, no database, no web server. `composer test:unit` works on a bare checkout. The bootstrap declares OC\Hooks\Emitter, which OCP\Files\IRootFolder extends but nextcloud/ocp does not ship, the same way tests/stubs/ocp.php already fills gaps for Psalm. * phpunit/phpunit and doctrine/dbal as dev dependencies. DBAL is needed because mocking OCP\IDBConnection reflects over IQueryBuilder, whose signatures reference Doctrine's types; the server provides it at runtime. * OCA\Notes\ is mapped in autoload-dev — the app relies on Nextcloud's own app autoloader, which is absent outside a server. * A separate phpunit-unit.yml workflow so these run on every pull request in seconds, independently of the server-backed test.yml. 120 tests covering NoteUtil (category-path normalisation including traversal attempts, title derivation, collision-safe file names, markdown stripping), NotesService (which files count as notes, the folder walk, titles from content), Note (title, category, excerpt, BOM and object-storage content handling), Util::retryIfLocked and ChunkCursor. Three tests are marked in their docblocks as characterization tests: they pin current behaviour that looks wrong so that a fix is a visible change rather than a silent one. No production code is touched by this commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
karlitschek
requested review from
enjeck
and
a lite review from Copilot
and removed request for
enjeck and
silverkszlo
August 6, 2026 12:59
There was a problem hiding this comment.
Pull request overview
Adds a server-free PHPUnit unit test suite for the app’s pure logic (note/title/category/path normalization, folder walking, cursor parsing, retry-on-lock behavior), making composer run test:unit runnable on a bare checkout without a Nextcloud instance.
Changes:
- Introduces unit-test infrastructure under
tests/unit/(phpunit config + bootstrap stubs) and adds comprehensive unit tests for key logic classes. - Adds dev dependencies and autoload-dev mappings needed to run app classes and mock OCP interfaces outside a server.
- Adds a dedicated GitHub Actions workflow for fast unit-test execution on every pull request, and wires unit tests into the
Makefile.
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/Service/UtilTest.php | Unit tests for Util::retryIfLocked() retry and rethrow behavior. |
| tests/unit/Service/NoteUtilTest.php | Unit tests for category-path normalization, safe title derivation, markdown stripping, and collision-safe filename generation. |
| tests/unit/Service/NoteTest.php | Unit tests for deriving title/category/excerpt/content behavior from mocked file nodes (incl. BOM/object-storage edge cases). |
| tests/unit/Service/NotesServiceTest.php | Unit tests for folder-walk logic determining note files/categories and title derivation from content. |
| tests/unit/Controller/ChunkCursorTest.php | Unit tests ensuring chunk cursor round-trips and rejects malformed inputs. |
| tests/unit/phpunit.xml | PHPUnit config for server-free unit tests (bootstrap, strictness, suite discovery). |
| tests/unit/bootstrap.php | Minimal stubs/bootstrap to allow mocking OCP interfaces that extend server-internal OC interfaces. |
| .github/workflows/phpunit-unit.yml | CI workflow to run unit tests quickly on PRs across the Nextcloud PHP version matrix. |
| composer.json | Adds PHPUnit/DBAL dev deps and autoload-dev mappings for app + unit tests. |
| composer.lock | Locks the added dev dependency graph for PHPUnit/DBAL and transitive packages. |
| Makefile | Adds test-unit target and runs unit tests as part of make test. |
| .gitignore | Ignores PHPUnit cache/result cache artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The characterization test claimed that every nested subcategory is dropped. That is only true while the parent's accumulated list is at least as long as the recursion's: the "+" union discards an entry whose index is already occupied, so a folder with more children than the parent has collected keeps the later ones. A folder 'Work' containing A, B and C therefore yields ['Work', 'Work/B', 'Work/C'] — 'Work/A' collides with 'Work' at index 0 and is lost, its siblings are not. Added as a second test case so the fix is verified against the real shape of the bug and not against a simpler mental model of it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
enjeck
reviewed
Aug 6, 2026
Contributor
There was a problem hiding this comment.
Haven't looked deeply yet, but a brief glance shows there are too many tests and I don't think they are all useful. Having too many makes CI slower and harder to maintain. I think we should strip this down to a few necessary tests that test core Notes functionality.
Also, there are too many long comments. I would expect the code to be readable enough to not need so many comments unless where there are "gotchas" to be aware of
Comment on lines
+4
to
+7
| # Unit tests. Deliberately separate from test.yml: these need no Nextcloud | ||
| # server, no database and no web server, so they finish in seconds and give | ||
| # feedback on every pull request. test.yml remains the place where the HTTP API | ||
| # is exercised against a real instance. |
Contributor
There was a problem hiding this comment.
Comment is unnecessary, can be removed
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
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.
The suite so far has been integration-only: everything under tests/api/ boots a real Nextcloud, so there was no way to exercise the app's pure logic without a server, and no test at all covered the functions that turn user input into file names.
composer.json already declared a
test:unitscript pointing at tests/unit/phpunit.xml, but neither the config nor PHPUnit itself was present. This makes that script real:composer test:unitworks on a bare checkout. The bootstrap declares OC\Hooks\Emitter, which OCP\Files\IRootFolder extends but nextcloud/ocp does not ship, the same way tests/stubs/ocp.php already fills gaps for Psalm.120 tests covering NoteUtil (category-path normalisation including traversal attempts, title derivation, collision-safe file names, markdown stripping), NotesService (which files count as notes, the folder walk, titles from content), Note (title, category, excerpt, BOM and object-storage content handling), Util::retryIfLocked and ChunkCursor.
Three tests are marked in their docblocks as characterization tests: they pin current behaviour that looks wrong so that a fix is a visible change rather than a silent one. No production code is touched by this commit.
🤖 AI (if applicable)