Skip to content

build: fetch LFS test data before the suites run - #4106

Open
KrishnaH96 wants to merge 4 commits into
mainfrom
krishna/fix/rel-fetch-test-data
Open

build: fetch LFS test data before the suites run#4106
KrishnaH96 wants to merge 4 commits into
mainfrom
krishna/fix/rel-fetch-test-data

Conversation

@KrishnaH96

Copy link
Copy Markdown
Contributor

Problem

Tests pull their LFS archives lazily inside the test. On a normal link a 1.2 GB archive takes longer than the 600 s per-test timeout: the worker is killed and the session hangs. Same root cause as #2119.

Solution

bin/fetch-test-data pulls every archive a test references; pytest-slow/all/coverage call it first, like build-test-natives. Three setup lines in docs/development/testing.md (fetch, playwright browsers, -n 8 on laptops).

How to test

bin/fetch-test-data && bin/pytest-all --numprocesses=8

Lists 40 archives; no in-test LFS pull afterwards.

AI assistance

Used Fable 5.1 extensively for root cause analysis, fix and testing.

Checklist

  • I have read and approved the CLA.

@codecov

codecov Bot commented Sep 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

@@            Coverage Diff             @@
##             main    #4106      +/-   ##
==========================================
- Coverage   78.60%   78.59%   -0.01%     
==========================================
  Files        1460     1460              
  Lines      138234   138234              
  Branches    11881    11881              
==========================================
- Hits       108656   108644      -12     
- Misses      26240    26245       +5     
- Partials     3338     3345       +7     
Components Coverage Δ
Tests 94.59% <ø> (ø)
Flag Coverage Δ
OS-ubuntu-24.04-arm 74.77% <ø> (-0.01%) ⬇️
OS-ubuntu-latest 75.48% <ø> (ø)
Py-3.10 75.25% <ø> (-0.01%) ⬇️
Py-3.11 75.46% <ø> (-0.01%) ⬇️
Py-3.12 75.46% <ø> (-0.01%) ⬇️
SelfHosted-Large 30.78% <ø> (+<0.01%) ⬆️
SelfHosted-Linux 35.69% <ø> (+<0.01%) ⬆️
SelfHosted-macOS 35.01% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread docs/development/testing.md
paul-nechifor
paul-nechifor previously approved these changes Sep 13, 2026
@spomichter
spomichter marked this pull request as ready for review September 13, 2026 01:00
@github-actions github-actions Bot added the ready-to-merge Required CI checks have passed on this PR label Sep 13, 2026
@github-actions github-actions Bot removed the ready-to-merge Required CI checks have passed on this PR label Sep 13, 2026
spomichter
spomichter previously approved these changes Sep 13, 2026
@greptile-apps

greptile-apps Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

Not safe to merge until the prefetch step includes dynamically constructed test-data archive names.

Findings

  1. P1 Dynamic archives skip prefetch

Summary

  • Summary

  • The new test-data prefetch script misses archives whose names are constructed dynamically by tests. Affected tests can still start an LFS download during execution and hit their timeout or leave the test session waiting. This should be fixed before merging.

Reviews (1) · Last reviewed commit: "docs: worker count for the full suite"

Comment thread bin/fetch-test-data Outdated
include=()
for f in data/.lfs/*.tar.gz; do
n=$(basename "$f" .tar.gz)
if grep -rqF -e "\"$n\"" -e "'$n'" --include='test_*.py' --include='conftest.py' dimos; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Dynamic archives skip prefetch

This scan only matches complete archive roots written as quoted literals. The parametrized gradient test constructs gradient_{method}.png at runtime, so gradient_simple.png.tar.gz and gradient_voronoi.png.tar.gz are not included. Those tests still download LFS data while running, which can exhaust their timeout and leave the test session waiting.

Artifacts

Evidence from the check

  • Authored and executed shell validation AST-resolves the gradient test's parametrized f-string and applies the changed script's exact scan predicate, showing the two required archive names are excluded.

Command output from the check

  • Captured output from `bash trex-artifacts/pr4106-archive-discovery-validation.sh` in `/home/user/repo` exits 0 and shows both existing required gradient archives are undiscovered, confirming the finding.

View artifacts

T-Rex Ran code and verified through T-Rex

@github-actions github-actions Bot added the ready-to-merge Required CI checks have passed on this PR label Sep 13, 2026
@KrishnaH96
KrishnaH96 force-pushed the krishna/fix/rel-fetch-test-data branch from 295837e to 5e4ad09 Compare September 13, 2026 01:44
@github-actions github-actions Bot removed the ready-to-merge Required CI checks have passed on this PR label Sep 13, 2026
@github-actions github-actions Bot added the ready-to-merge Required CI checks have passed on this PR label Sep 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport release/0.0.14 ready-to-merge Required CI checks have passed on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants