test: add server-free unit tests for note, path and cursor logic - #1966
karlitschek wants to merge 4 commits into
Conversation
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.
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
AndyScherzinger
added
2. developing
tests
AI assisted
27f152a to
bfde452
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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. The tests cover 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. No production code is touched by this commit. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
gatherNoteFiles() merged the recursion's category list into the parent's with the `+` array union. Both operands are sequentially-keyed lists, so the union keeps the left-hand value for every index that is already occupied and discards the rest: a subcategory was dropped whenever the parent's list had already reached that index. Which ones were lost therefore depended on position rather than depth — for a folder 'Work' containing A, B and C, 'Work/A' collided with 'Work' at index 0 and vanished while its siblings survived. The visible effect is a nested folder without notes of its own missing from the sidebar, because the frontend derives categories from the notes themselves and only consults this list for folders that have none. The files union above it is keyed by file id, which is unique across the tree, so no note was ever lost. array_merge() appends instead of filling gaps, which is what the recursion needs. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
Two title comparisons treated a string as a byte sequence or as a
boolean, and both are wrong for real note content.
getExcerpt() decided whether the content repeats the note's title by
handing a character count to strncasecmp(), which counts bytes:
$length = mb_strlen($title, 'utf-8'); // characters
strncasecmp($excerpt, $title, $length) // bytes
mb_substr($excerpt, $length, null, 'utf-8') // characters
For an ASCII title the two agree. For a multi-byte one strncasecmp only
ever compared the first few bytes, so content that merely started with
the same character was mistaken for a repeated title and that many
characters were cut off. A note titled 日本語 whose body is
"日曜日 is Sunday" was excerpted as "is Sunday". Comparing a
case-folded prefix of the same character length fixes it and keeps the
case-insensitive match the byte version provided.
getSafeTitle() guarded its fallback with empty(), and empty('0') is true
in PHP, so a note whose first line was exactly "0" was titled "New note".
Comparing against '' is what the guard meant.
Assisted-by: Claude Code:claude-opus-5
Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
phpcs reports "The closing brace for the class must go on the next line after the body" for the stray blank line between the last method and the end of the class. It is the only phpcs error in the tree, so removing it makes `make lint` pass. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
e6429cf to
6ad562d
Compare
|
@enjeck thanks for the review — both points should be covered now, and the branch is rebased on current main. Fewer tests. The suite is down from 121 tests to 81, in 683 lines instead of 1390. The characterization tests that pinned known-bad behaviour in long docblocks are gone, along with a duplicate cursor round-trip, tests of trivial getters, and the thinner data-provider rows. It runs in about 50ms, so the Fewer comments. The explanatory blocks are gone throughout, including the workflow header you flagged inline. What is left in Writing the tests did turn up three genuine bugs, so those are fixed in their own commits with regression tests rather than pinned: nested categories dropped by a That does widen what started as a test-only PR, so happy to split the fix commits out if you would rather review them separately. |
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 declaresOC\Hooks\Emitter, whichOCP\Files\IRootFolderextends but nextcloud/ocp does not ship, the same way tests/stubs/ocp.php already fills gaps for Psalm.OCP\IDBConnectionreflects overIQueryBuilder, 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.81 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 including hidden files and attachment folders, titles from content), Note (title, category, excerpt, BOM and object-storage content handling),
Util::retryIfLockedand ChunkCursor.Changes since the first review
Rebased onto current main, which needed two fixes on its own:
NotesService::__constructhas gained a fourth parameter andgatherNoteFiles()a$showHiddenargument since this branch was opened.Addressing @enjeck's review:
maxRetries, and an empty custom file extension — describe states no caller can reach, so they are simply gone. Also dropped: a duplicate cursor round-trip, tests of trivial getters, and the thinner rows of several data providers.falserather than''for an empty file — plus two short docblocks describing what a helper builds.php_dirssomake lintcovers it the way it already covers tests/api/.One side effect worth knowing about: installing doctrine/dbal lets Psalm resolve Doctrine's classes for the first time, and it immediately flagged a pre-existing deprecated
Table::setPrimaryKey()call in a migration. That is recorded in tests/psalm-baseline.xml rather than worked around, since the call is what Nextcloud's own migration API expects.Two bugs the tests turned up
Both are long-standing, neither is introduced here, and each is fixed in its own commit with a regression test.
Nested categories were silently dropped.
gatherNoteFiles()merged the recursion's category list into the parent's with the+array union. Both operands are sequentially-keyed lists, so the union keeps the left-hand value at every occupied index and discards the rest — which subcategory was lost depended on its position, not its depth. Confirmed against a local Nextcloud 36 instance, where the sidebar received 7 categories although 16 exist. The files union beside it is keyed by file id, which is unique across the tree, so no note was ever lost, and the v1 API discards this list entirely, so there is no client-facing change.Excerpts mis-stripped multi-byte titles.
getExcerpt()passed a character count tostrncasecmp(), which counts bytes, so content that merely began with the same first character as the title was mistaken for a repeated title and truncated. On the same instance, a note titled 日本語 with the body "日曜日 is Sunday" was excerpted as "is Sunday". The same commit replacesempty($title)ingetSafeTitle(), which made a note whose first line is exactly0come out titled "New note".Separately,
make linthad one phpcs error left in it — a stray blank line before the closing brace ofSettingsService— which is now gone too, so the target passes cleanly.These three commits widen what was a test-only PR. They are small and self-contained, so say the word if you would rather review them separately and I will split them out.
🤖 AI (if applicable)