test(install): stop assuming pytest's tmp_path is on disk by ChuckBuilds · Pull Request #492 · ChuckBuilds/LEDMatrix

ChuckBuilds · 2026-08-21T18:04:11Z

The bug

test_returns_nothing_when_tmpdir_is_already_disk_backed fails on main. It asserts that lm_disk_backed_tmpdir prints nothing when TMPDIR is already disk-backed, and uses pytest's tmp_path as the "disk-backed" directory:

# tmp_path is on the regular filesystem, so the default must be kept.
assert call("lm_disk_backed_tmpdir", env={"TMPDIR": str(tmp_path)}) == ""

That premise is false on the platform the helper was written for. Debian 13 mounts /tmp as tmpfs — which is the entire reason lm_disk_backed_tmpdir exists — and pytest puts tmp_path under /tmp. So on the target platform TMPDIR is memory-backed, the helper correctly answers /var/tmp, and the test fails:

E   AssertionError: assert '/var/tmp' == ''
E     + /var/tmp

Reproduced on a box where /tmp is tmpfs and / is ext4:

$ findmnt -no FSTYPE,TARGET --target /tmp
tmpfs  /tmp
$ findmnt -no FSTYPE,TARGET --target /var/tmp
ext4   /

The helper is right; the test is wrong. No production change here.

The fix

The test now looks for a directory whose backing store is actually disk — tmp_path, else a scratch dir under /var/tmp, else beside the library — using the same findmnt lookup the helper itself uses. It skips only if no disk-backed directory exists anywhere.

Worth noting: my first attempt just skipped whenever tmp_path was tmpfs. That made it skip on every machine with a tmpfs /tmp — barely better than asserting the wrong thing, since a test that never runs catches nothing. It searches instead of giving up.

Verification

31 passed, 0 skipped (was 30 passed / 1 failed)
Mutation-checked — deleting the "is the current TMPDIR memory-backed?" guard from lm_disk_backed_tmpdir fails this test, so it still catches the regression it exists for:
```
=== helper's memory-backed guard removed ===
FAILED TestDiskBackedTmpdir::test_returns_nothing_when_tmpdir_is_already_disk_backed
1 failed, 30 passed
```

Why CI is green today

CI runners generally give you a disk-backed /tmp, so this only bites on real Debian 13 hardware and on dev boxes with a tmpfs /tmp — i.e. the actual deployment target.

test_returns_nothing_when_tmpdir_is_already_disk_backed asserted that lm_disk_backed_tmpdir prints nothing when TMPDIR is already disk-backed, and used pytest's tmp_path as the "disk-backed" directory: # tmp_path is on the regular filesystem, so the default must be kept. assert call("lm_disk_backed_tmpdir", env={"TMPDIR": str(tmp_path)}) == "" That premise is false on the platform the helper was written for. Debian 13 mounts /tmp as tmpfs -- which is the entire reason lm_disk_backed_tmpdir exists -- and pytest puts tmp_path under /tmp. So on the target platform TMPDIR is memory-backed, the helper correctly answers /var/tmp, and the test fails: E AssertionError: assert '/var/tmp' == '' The helper is right; the test was wrong. Reproduced on a box where /tmp is tmpfs and / is ext4. The test now looks for a directory whose backing store is actually disk -- tmp_path, else a scratch dir under /var/tmp, else beside the library -- using the same findmnt lookup the helper itself uses, and skips only if no disk-backed directory exists anywhere. An earlier version of this fix skipped whenever tmp_path was tmpfs, which made it skip on every machine with a tmpfs /tmp; that is barely better than asserting the wrong thing, so it now searches instead of giving up. Verified: 31 passed, 0 skipped. Mutation-checked -- deleting the "is the current TMPDIR memory-backed?" guard from lm_disk_backed_tmpdir fails this test, so it still catches the regression it is there for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW

coderabbitai · 2026-08-21T18:04:23Z

Warning

Review limit reached

@ChuckBuilds, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 20 minutes

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6ce1bfa6-e0c5-4f86-8ead-3237d15c8a1c

📥 Commits

Reviewing files that changed from the base of the PR and between fe5a3aa and 0912332.

📒 Files selected for processing (1)

test/test_install_lowmem.py

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.}

codacy-production · 2026-08-21T18:05:06Z

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer}
_{TIP This summary will be updated as you push new changes.}

The branch was 57 commits behind and conflicting. I had put the rebase aside earlier as needing the author's eyes, on the grounds that the PR is +5091 lines -- but that was the wrong measure. The actual conflict was a single hunk in app.css, where this branch adds .md\:inline and main added .md\:block and .md\:w-auto at the same place. All three are kept. Merging rather than rebasing: the branch is public and 57 commits behind, so a rebase would rewrite shared history for a force-push. Three findings fixed on top: A missing `text` or `format` was a 500. `p` is a copy of the raw element and the defaults were applied to the locals t1/fmt1 only, so an element omitting either key left it absent, manager.py.j2 rendered `{{ el.text | tojson }}` over a jinja2.Undefined, and tojson raised TypeError -- which no handler catches: text without 'text': TypeError: Object of type Undefined is not JSON serializable clock without 'format': same Both keys are now set explicitly. Verified: removing either assignment fails 4 of the new tests. E741 on my own injection-test file: two `for i, l in enumerate(...)` loops, which ruff rejects and would fail a lint-gated build. Renamed. Ruff now clean on all three files this PR touches. Not done: registering composer_bp. This PR's own description gates it -- "Not yet wired up ... tracking as a follow-up", with an unchecked box for "Register composer_bp in app.py before merging or exposing this route" -- so it is a deliberate decision, not an oversight. Confirmed the blueprint appears in no register_blueprint call outside this branch's tests, which also means the code-injection fixed earlier in this PR was never reachable in a deployed instance. Worth fixing before the route is exposed; not worth exposing the route to satisfy a review comment. Verified on the merged tree: 3850 passed, 1 failed, 60 skipped. The failure is test_install_lowmem's tmpfs assumption, which is fixed in #492 and not yet on main. The static audit now passes 3/3 -- the twelve classes it flagged before were defined on main all along and only looked missing because this branch was behind. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW

Reproduced from the review comment. A `group` element carrying minWidth generated: if width >= 64: # breakpoint: 64px+ displays only # <nothing> manager.py.j2 wraps each element in the breakpoint and blink blocks, but the body comes from the per-type branches -- and a type with no branch contributes nothing, so the wrapper opens a block with no statements. ast.parse then fails and the caller is told only "Generated code has a syntax error: expected an indented block ... line 49", naming a line of generated source they never see. Two defences: - _preprocess_elements drops types the template has no branch for, alongside the existing `section` skip. This is the root cause: those elements should never have reached the template. - The branch chain ends in `{% else %}pass`, so a type added to the canvas before its drawing branch exists degrades to a no-op rather than a plugin that will not parse. The review also cited dynamic_text with binding_source != 'config'. That one does not reproduce -- the branch emits a draw_text regardless -- which is why an earlier attempt to reproduce this found nothing. _RENDERABLE_ELEMENT_TYPES has to stay in step with the template: a type listed with no branch emits an empty block again, and a branch missing from the list is silently dropped from every generated plugin. A test asserts the two sets are equal rather than trusting them to be maintained together. Tests: 12 new, covering group/unknown/section against breakpoint, blink and both nested, plus the set-equality and fallback checks. 7 fail with both defences reverted. 172 composer tests pass; full suite 3862 passed, the one failure being test_install_lowmem (pre-existing, awaiting #492). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW

Code injection, found by chasing why a security test could not have caught it. _preprocess_elements built the far corner of five shapes by interpolating the payload's width/height straight into generated Python: w = el.get('width', 10) p['x2_expr'] = f"({x_expr}) + {w}" so a rectangle with width='0 or __import__("os").system("id")' generated [0, 0, (0) + 0 or __import__("os").system("id"), (0) + 8], inside a manager.py that /api/install writes to disk and the plugin loader imports and executes. rectangle, arc, ellipse, rounded_rectangle and gauge all share the pattern. Both fields now go through _safe_int, like every other geometry value. Unreachable today only because composer_bp is still unregistered -- the same caveat as the docstring injection fixed earlier in this PR. Why the existing test missed it ------------------------------- test_a_non_numeric_geometry_value_cannot_reach_the_source drove its payloads through a "line" element. manager.py.j2 has never had a `line` branch, so _preprocess_elements produced nothing for it and no value it set could reach the generated source. Every assertion passed trivially, against code that was in fact vulnerable. The test has been vacuous since it was written; the _RENDERABLE_ELEMENT_TYPES constant added in the previous commit only made the cause legible. It now runs across the five types that actually render, over x/y/width/height: 40 of those cases fail with the clamping reverted, where the old version passed 100%. A second test asserts every type used by the injection suite is in _RENDERABLE_ELEMENT_TYPES, so the suite cannot quietly go vacuous again. Also: _payload set "config_vars", but _generate_plugin_files reads data['dataModel']['configVars']. Nothing passed through that key was ever read. Fixed so config-var tests exercise the real path. Full suite: 3967 passed, the one failure being test_install_lowmem (pre-existing, awaiting #492). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW

Three review findings in the composer's JavaScript, all confirmed against the code. Stroke widths did not scale with SCALE -------------------------------------- _drawElement scales all geometry by `s`, but left ctx.lineWidth in canvas pixels, so at SCALE>1 every outline rendered thinner than one LED pixel and the preview stopped matching the panel it is previewing. Fixed for rectangle, ellipse, arc, rounded_rectangle, line, divider and progress_bar. Ellipse and arc also inset their radii by half the scaled width -- a stroke straddles its path, so without the inset the outline spills outside the element's bounds. The gauge branch already did this; the rest now match it. Selection handles and the grid stay in canvas pixels deliberately: they are editor chrome, not LED geometry, and live in other functions. `line` ignored anchors ---------------------- _drawElement resolves ax/ay for every element, but the line branch drew raw el.x0/el.y0/el.x1/el.y1. Setting xAnchor or yAnchor moved every other element type and left lines where they were. getBoundingBox had the same omission, so even once a line moved its hit box would not have. Both now translate by (ax - el.x0, ay - el.y0); ax resolves from el.x0 for a line, so that is exactly the anchor offset. Four state mutations skipped _snapshot -------------------------------------- _snapshot serialises metadata and currentPreset and is the only caller of _debouncedAutosave. onBgColorChange, setCustomSize, changePreset and applyPresetLabel each changed exactly those values without calling it, so the background colour and the canvas size were lost on reload and could not be undone. Same defect already fixed in onColorChange. The review named three; applyPresetLabel has it too -- it is the branch that handles sizes absent from DISPLAY_PRESETS. Snapshotting is on the user-driven path only. _applyState and loadTemplate drive these with {silent: true} while restoring, and snapshotting there would push restore steps onto the undo stack and re-autosave the state just loaded. Tests ----- No JS runner here, so test_composer_js_contracts.py asserts on the parse tree via tree-sitter: both files parse, no bare `ctx.lineWidth = 1` inside _drawElement, the line branch and its bounding box carry the anchor offset, each of the five mutations snapshots, and the two preset paths keep their !opts.silent guard ahead of the snapshot. 9 of its 11 checks fail against the previous JS. Full suite 3978 passed, the one failure being test_install_lowmem (pre-existing, awaiting #492). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW

…arquee ids Three more routes into the generated source, plus a fix to one of my own tests that was checking the wrong branch. Prefixed colour channels were interpolated raw ---------------------------------------------- Five tuples were built without coercion: p['fill_tuple'] = f"({el.get('r', 100)}, {el.get('g', 200)}, ...)" p['empty_tuple'] = f"({el.get('emptyR', 50)}, ...)" p['label_tuple'] = f"({el.get('labelR', 200)}, ...)" so progress_bar, pips, sparkline and gauge took arbitrary expressions the same way width/height did. Confirmed: every one of the five put __import__ into the generated source. They now go through a new _rgb_tuple helper, which _rgb_expr also delegates to. The pre-existing colour test only covered r/g/b on a text element, which is why the prefixed channels and these four types were never exercised. Non-finite numbers escaped as a 500 ----------------------------------- json.loads accepts Infinity/-Infinity/NaN by default and Flask's get_json passes them straight through, so a payload can hand _safe_int a non-finite float. int(inf) raises OverflowError, which is neither ValueError nor ComposerInputError, so it escaped both handlers and surfaced as a 500 with a traceback rather than a 422. Verified end to end through Flask's parser. Marquee ids reached the source as identifiers --------------------------------------------- data_key is spliced UNQUOTED into variable names (_{{ data_key }}_text = ...) and only '-' was normalised. A punctuated id landed in the generated source as code. ast.parse caught it, so this was not exploitable, but the caller got an opaque "Generated code has a syntax error" instead of being told the id was unusable -- the same failure mode as the empty-block bug. Now restricted to identifier characters and bounded to 64. The line-anchor test was testing the wrong branch ------------------------------------------------- test_line_branch_applies_the_anchor_offset searched the whole file for "case 'line': {". getBoundingBox has one too and comes first, so the assertion was reading the bounding-box branch: stripping the anchor offset from _drawElement left all 11 checks green. Both line tests are now scoped to their own function via tree-sitter, so they cannot be satisfied by the same branch. Tests: 35 of the injection suite's checks fail against the reverted fixes; the scoped line test fails when _drawElement's offset is removed. Full suite 4059 passed, the one failure being test_install_lowmem (pre-existing, awaiting #492). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW

Two alignment implementations existed and the toolbar used the wrong one. alignElement(dir) set el.x/el.y and stopped there. resolveAnchor turns anchor='right' into `dim - val`, so with xAnchor='right' an "align left" (el.x = 0) resolved to x = MATRIX_W and the element jumped to the far right edge -- the opposite of what was asked. It also never touched el.x0/el.y0, so a line's endpoints were left where they were. _alignElement already did both correctly: it clears the anchor so the stored value is absolute, and moves x0/y0 for lines. Its six wrappers -- alignLeft, alignHCenter, alignRight, alignTop, alignVCenter, alignBottom -- existed and had no callers at all. All six toolbar buttons now call the wrappers, and the legacy method is removed rather than left to drift back into use. Tests: the toolbar calls each wrapper and no longer calls alignElement, the legacy definition is gone, and _alignElement still clears the anchor and moves line endpoints. Two of them fail against the previous markup. Full suite 4062 passed, the one failure being test_install_lowmem (pre-existing, awaiting #492). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW

ChuckBuilds mentioned this pull request Aug 21, 2026

consolidate(perf): cut SD writes, log volume, and metrics churn #486

Merged

This was referenced Aug 22, 2026

feat(web): add Plugin Composer -- visual drag-and-drop plugin builder #413

Open

fix(display): retry a plugin that is enabled but failed to load #495

Merged

test(logging): stop the location assertion matching the clock #496

Merged

ChuckBuilds mentioned this pull request Aug 22, 2026

fix(web): reject non-finite JSON numbers instead of raising #497

Merged

ChuckBuilds merged commit 6138a3c into main Aug 23, 2026
9 checks passed

ChuckBuilds deleted the fix/lowmem-tmpdir-test branch August 23, 2026 15:42

Navigation Menu

Uh oh!

test(install): stop assuming pytest's tmp_path is on disk - #492

test(install): stop assuming pytest's tmp_path is on disk#492
ChuckBuilds merged 1 commit into
mainfrom
fix/lowmem-tmpdir-test

ChuckBuilds commented Aug 21, 2026

Uh oh!

coderabbitai Bot commented Aug 21, 2026

Review limit reached

Uh oh!

codacy-production Bot commented Aug 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

ChuckBuilds commented Aug 21, 2026

The bug

The fix

Verification

Why CI is green today

Uh oh!

coderabbitai Bot commented Aug 21, 2026

Review limit reached

Uh oh!

codacy-production Bot commented Aug 21, 2026

Up to standards ✅

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant