Skip to content

Store system test screenshots in git lfs#1641

Open
nygrenh wants to merge 3 commits intomasterfrom
git-lfs
Open

Store system test screenshots in git lfs#1641
nygrenh wants to merge 3 commits intomasterfrom
git-lfs

Conversation

@nygrenh
Copy link
Member

@nygrenh nygrenh commented Jan 29, 2026

Summary by CodeRabbit

  • Chores
    • Configured Git LFS for managing test screenshot assets
    • Added automatic Git LFS prerequisite checks to all system test commands to ensure proper setup before execution

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 29, 2026

📝 Walkthrough

Walkthrough

Adds Git LFS support across the project by introducing a verification function that ensures git-lfs is installed and functional, integrating this check into all system test scripts, configuring binary asset handling in Git attributes, and displaying Git LFS version information.

Changes

Cohort / File(s) Summary
Git LFS Configuration
.gitattributes
Adds LFS filter configuration for PNG files in system-tests path to enable LFS-managed storage of binary test assets.
LFS Verification Function
bin/.common
Implements ensure_git_lfs_installed() function that validates git-lfs installation, functionality, and initializes local LFS filters if needed; pulls and checks out LFS objects when filters are missing.
System Test Scripts
bin/system-tests-record-test-admin, bin/system-tests-record-test-teacher, bin/system-tests-record-test-user, bin/system-tests-record-test-without-resetting-db, bin/system-tests-remove-login-states, bin/system-tests-run-tests, bin/system-tests-run-tests-debug, bin/system-tests-run-tests-record-video, bin/system-tests-run-tests-slowmo, bin/system-tests-update-snapshots, bin/system-tests-update-snapshots-with-no-scroll-restoration
Adds prerequisite call to ensure_git_lfs_installed() early in each script to verify Git LFS is available before proceeding with test execution.
Infrastructure & Utility Scripts
bin/minikube-start, bin/print-versions
Adds Git LFS prerequisite check in minikube startup; adds Git LFS version detection and reporting in version printing utility.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested labels

feature

Poem

🐰 Git LFS now guards our snapshots tight,
Binary assets stored just right,
Prerequisites checked before each test run—
Our screenshots safe, our workflow's fun! 📸✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Store system test screenshots in git lfs' accurately describes the main change: configuring Git LFS for PNG screenshots in the system-tests path and ensuring Git LFS is available across all system test scripts.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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 and usage tips.

@coderabbitai coderabbitai bot added the feature label Jan 29, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@bin/.common`:
- Around line 67-75: The check for missing Git LFS filters is incorrect: it uses
&& so remediation only runs when both lfs_clean and lfs_smudge are empty; update
the condition in the block that sets had_lfs_filters and calls git lfs install
--local so it uses || (i.e., if either lfs_clean or lfs_smudge is empty) to
treat partial configs as missing; adjust the conditional surrounding lfs_clean,
lfs_smudge, had_lfs_filters and the git lfs install invocation accordingly.
🧹 Nitpick comments (1)
bin/.common (1)

42-42: Add a brief purpose comment for this public helper.

Guidelines ask for minimal function documentation. A short intent comment (including the hydration side-effect) would clarify behavior without restating the name.

✍️ Proposed tweak
+# Preflight Git LFS availability and hydrate LFS objects if this checkout lacked LFS filters.
 function ensure_git_lfs_installed() {

Comment on lines +67 to +75
lfs_clean=$(git config --get filter.lfs.clean || true)
lfs_smudge=$(git config --get filter.lfs.smudge || true)
had_lfs_filters=true
if [ -z "$lfs_clean" ] && [ -z "$lfs_smudge" ]; then
had_lfs_filters=false
git lfs install --local &> /dev/null || true
fi
if [ "$had_lfs_filters" = false ]; then
echo "Git LFS filters were missing for this checkout. Hydrating LFS objects now..."
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

Git LFS filter.lfs.clean filter.lfs.smudge both required installation

💡 Result:

filter.lfs.clean and filter.lfs.smudge are the Git attributes “filters” that Git LFS registers so Git can transparently convert large files to/from LFS pointer files:

  • clean: runs when you add/commit → stores content in LFS and writes a pointer into Git.
  • smudge: runs when you checkout/clone/pull → replaces the pointer in your working tree with the real LFS content.

If your config (often via git lfs install) includes filter.lfs.required=true, Git treats that filter as mandatory: operations that need the filter will fail if Git LFS isn’t installed or the filters aren’t registered for that user/repo. (support.atlassian.com)

What you must install/do

  1. Install the Git LFS client (git-lfs) on the machine.
  2. Run one of:
    • git lfs install (registers the LFS clean/smudge filters in your global Git config and installs the pre-push hook), or
    • git lfs install --local (registers filters for just that repo). (manpages.debian.org)

Notes

  • If smudging is intentionally disabled (e.g., git lfs install --skip-smudge), files won’t be downloaded on checkout; you must run git lfs pull to fetch LFS content. (support.atlassian.com)

Citations:


Treat partial LFS filter config as missing.

Git LFS requires both filter.lfs.clean and filter.lfs.smudge to be registered for proper operation—clean stores content in LFS during commit, and smudge replaces pointer files with actual content during checkout. If only one is missing, LFS is broken. Line 70 currently uses &&, which skips remediation unless both filters are absent. Change to || so either missing filter triggers git lfs install --local.

🧩 Proposed fix
-    if [ -z "$lfs_clean" ] && [ -z "$lfs_smudge" ]; then
+    if [ -z "$lfs_clean" ] || [ -z "$lfs_smudge" ]; then
       had_lfs_filters=false
       git lfs install --local &> /dev/null || true
     fi
🤖 Prompt for AI Agents
In `@bin/.common` around lines 67 - 75, The check for missing Git LFS filters is
incorrect: it uses && so remediation only runs when both lfs_clean and
lfs_smudge are empty; update the condition in the block that sets
had_lfs_filters and calls git lfs install --local so it uses || (i.e., if either
lfs_clean or lfs_smudge is empty) to treat partial configs as missing; adjust
the conditional surrounding lfs_clean, lfs_smudge, had_lfs_filters and the git
lfs install invocation accordingly.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant