Skip to content

ci: cache test fixtures between runs - #3537

Open
TheTisiboth wants to merge 9 commits into
masterfrom
ci/cache-fixtures
Open

TheTisiboth wants to merge 9 commits into
masterfrom
ci/cache-fixtures

Conversation

@TheTisiboth

@TheTisiboth TheTisiboth commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #2722

Topic and Scope

Fixture creation (clone + install NC server, dumps, app install) takes ~30–55 s per job. The fixture is now archived with sudo tar (keeps ownership of root-owned DB files) and cached with actions/cache, keyed on DB (incl. its image), PHP, NC branch commit, app migrations, appinfo/info.xml and the test runner. On a hit, only the fixture is restored. The cache is saved right after fixture creation, even if tests fail.

Job Before After (cache hit, + ~7 s restore)
mysql 32 s 9 s
sqlite 27 s 4 s
pgsql 34 s 10 s

The MySQL healthcheck now pings over TCP: the socket ping reported healthy on the temporary init server, which a fast restore exposed.

Concerns/issues

  • NC master job not cached: master moves too often for cache hits.
  • Each entry is ~430 MB (10 GB repo limit) and changes whenever the NC branch moves.

Formal requirements

  • I did check that the app can still be opened and does not throw any browser logs
  • I created tests for newly added PHP code (check this if no PHP changes were made)
  • I updated the OpenAPI specs and added an entry to the API changelog (check if API was not modified)
  • I notified the matrix channel if I introduced an API change

AI disclosure

I used AI for code gen.

TheTisiboth and others added 2 commits October 29, 2026 19:11
Run unit, integration and migration tests as separate steps to expose per-suite timings.
Collect coverage XML from all runs since each call creates its own run folder.

Assisted-by: Claude Code:claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Léo Jan <leo.jan@doctolib.com>
Restore the fixture instead of reinstalling the Nextcloud server when the server commit, app migrations and test runner are unchanged. Skipped for pgsql, whose fixture holds root-owned database files.

Assisted-by: Claude Code:claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Léo Jan <leo.jan@doctolib.com>
@github-actions

github-actions Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Test Results

   12 files    600 suites   2m 50s ⏱️
  605 tests   597 ✅  8 💤 0 ❌
2 420 runs  2 387 ✅ 33 💤 0 ❌

Results for commit 2f4d3e2.

♻️ This comment has been updated with latest results.

The cache post step re-evaluated hashFiles and failed on root-owned files created in the test volumes.

Assisted-by: Claude Code:claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Léo Jan <leo.jan@doctolib.com>
@TheTisiboth
TheTisiboth marked this pull request as ready for review October 29, 2026 18:54
TheTisiboth and others added 2 commits October 29, 2026 20:00
Assisted-by: Claude Code:claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Léo Jan <leo.jan@doctolib.com>
Otherwise docker creates them as root when starting the helpers, and the restore cannot change their ownership.

Assisted-by: Claude Code:claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Léo Jan <leo.jan@doctolib.com>
@TheTisiboth
TheTisiboth marked this pull request as draft October 29, 2026 19:18
The socket ping reported healthy on the temporary init server, so a quick restore of a cached fixture hit the restart window.

Assisted-by: Claude Code:claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Léo Jan <leo.jan@doctolib.com>
@TheTisiboth
TheTisiboth marked this pull request as ready for review October 29, 2026 19:39
@codecov

codecov Bot commented Oct 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Base automatically changed from ci/split-test-steps to master October 29, 2026 20:37
@coderabbitai

coderabbitai Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Walkthrough

The test workflow now caches fixture archives using a key based on the database, PHP version, stable-branch commit, and selected file hashes. It restores and extracts fixtures on a cache hit. On a cache miss, it creates and saves the fixtures. The MySQL health check now runs mysqladmin ping against 127.0.0.1 instead of using the Unix socket.

Priority: Low

Merge Risk: Moderate · up to 2f4d3

Cache-hit CI jobs may run against an empty cookbook app directory, which breaks the reliability of the test results. The fixture cache is also unlikely to be shared across pull requests unless a default-branch cache is seeded. Fix the app synchronization on the cache-hit path before merging.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title "ci: cache test fixtures between runs" matches the main change. The workflow now computes a fixture-cache key, restores a cached fixture tar on a hit, and saves the cache after fixture creat…
Linked Issues check Passed Issue #2722 requests fixture caching to reduce fixture recreation time. The workflow adds a cache key for database, PHP, Nextcloud branch commit, migrations, manifest, and test-helper files. It restor…
Out of Scope Changes check Passed The workflow changes support fixture caching. The MySQL Dockerfile changes the healthcheck from a Unix socket to TCP on 127.0.0.1. This change supports reliable startup after fixture restoration. The …
Description check Passed The description follows the required template. It explains the scope, implementation, performance impact, concerns, and formal requirements. All required sections are present and complete.
Full details: Title check

Explanation

The title "ci: cache test fixtures between runs" matches the main change. The workflow now computes a fixture-cache key, restores a cached fixture tar on a hit, and saves the cache after fixture creation. The title is concise and uses a conventional commit prefix. It omits the MySQL healthcheck change, which is a minor secondary change.

Full details: Linked Issues check

Explanation

Issue #2722 requests fixture caching to reduce fixture recreation time. The workflow adds a cache key for database, PHP, Nextcloud branch commit, migrations, manifest, and test-helper files. It restores and extracts the fixture on a cache hit. It archives and saves the fixture after creation on a cache miss. These changes implement the issue requirement.

Full details: Out of Scope Changes check

Explanation

The workflow changes support fixture caching. The MySQL Dockerfile changes the healthcheck from a Unix socket to TCP on 127.0.0.1. This change supports reliable startup after fixture restoration. The reviewed change summary shows no unrelated functional changes.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@christianlupus

Copy link
Copy Markdown
Collaborator

Would it make sense to tar the cache content? This should allow us to preserve the permissions. Also, it is faster to create and download artifacts with a low number of entries (heuristic finding>

@TheTisiboth

Copy link
Copy Markdown
Collaborator Author

ah yeah tar is a good idea, for permissions, and also we can now include pgsql!

actions/cache packs all the cached paths into one zstd tarball before uploading, so the cache is a single entry no matter how many files the fixture has. The per-file overhead you have in mind applies to upload-artifact, not to actions/cache, so tarring first wouldn't change the speed.

TheTisiboth and others added 2 commits October 30, 2026 00:09
Tarring with sudo keeps ownership of the root-owned pgsql DB files, which actions/cache could not read.

Assisted-by: Claude Code:claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Léo Jan <leo.jan@doctolib.com>
Signed-off-by: Léo Jan <leo.jan@doctolib.com>

# Conflicts:
#	.github/workflows/tests.yml
@nextcloud-command nextcloud-command added the AI assisted This PR contains AI-assisted commits label Oct 29, 2026
@christianlupus

Copy link
Copy Markdown
Collaborator

Aah, right, it was about artifacts. I built an image on docker hub from this branch and I'm fact it seems to speed things up. See the MariaDB run.
I will remove the permission rule again to not forget it.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2


Review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: dc0ce68b-31e8-450b-ad58-8440ef6d2b63
Commits

Reviewing files that changed from the base of the PR and between 665c61e and 97bf3fe.

Files selected for processing: 2
  • .github/actions/run-tests/mysql/Dockerfile
  • .github/workflows/tests.yml

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

Comment thread .github/workflows/tests.yml Outdated
Comment thread .github/workflows/tests.yml
Assisted-by: Claude Code:claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Léo Jan <leo.jan@doctolib.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up Comments

Outside diff Synchronize the cookbook app on cache hits.

239-239: Major · Functional Correctness · Quick win

Synchronize the cookbook app on cache hits.

The cache-hit branch only creates directories and starts helpers. It does not run the app synchronization performed during fixture creation. The archive contains only version.json and fixtures/default, while Docker Compose mounts volumes/cookbook over /var/www/html/custom_apps/cookbook. A fresh cache-hit runner therefore mounts an empty cookbook app, so the tests cannot load the current app code. Synchronize the checkout into volumes/cookbook before starting the helpers, or include that directory in a cache design keyed by the app revision. (raw.githubusercontent.com)

Open the source location


Review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 3adc56c6-c08b-4af7-9768-b896b06dfee1
Commits

Reviewing files that changed from the base of the PR and between 97bf3fe and 2f4d3e2.

Files selected for processing: 1
  • .github/workflows/tests.yml

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

@TheTisiboth

TheTisiboth commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator Author

there is a catch: the pr would always have a cache miss on the initial ci runsince the cache is only created from the pr (this doesnt run on master)
So we see the benefit only on a second run from a PR, wich is not ideal I guess
i don't have an ideal solution to fix that actually (i don't want to make the whole CI run on master, it would be costly
But yeah we could either run the ci on master, or a daily cron job , or use a daily key for the cache : A PR's first run each day misses, then every later push or rerun that day hits (that is the cheapest option with higher gain, for a daily approach)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI assisted This PR contains AI-assisted commits

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Optmize Fixtures

3 participants