Skip to content

Test framework: fix the old test bugs in llrender's tests - #469

Merged
RyeMutt merged 5 commits into
rye/tests-tm22from
rye/tm22-test-fixes
Oct 13, 2026
Merged

Test framework: fix the old test bugs in llrender's tests#469
RyeMutt merged 5 commits into
rye/tests-tm22from
rye/tm22-test-fixes

Conversation

@RyeMutt

@RyeMutt RyeMutt commented Oct 13, 2026

Copy link
Copy Markdown
Member

Description

This PR fixes bugs in llrender's own tests. The TM22 Catch2 conversion found these bugs and kept them, because a conversion must not change what a test does. Each fixed test was one that:

  • could not fail for the reason it existed;
  • checked something other than what it said, or less than its comment said;
  • read through what a lookup returned, or indexed what nothing had sized, so a miss would crash the whole binary and take every later test with it, rather than fail its own case;
  • left state behind for later cases when it failed;
  • always skipped, because it asked for a font the package does not ship.

It stacks on #468 (rye/tests-tm22) and changes only test files.

Font registry (3fa253c)

  • llfontregistry:
    • Ordering: "operator< compares style before size" now gives the two descriptors different sizes. Before, both had the same size, so it never tested whether style comes first.
    • Normalization: getMatchingFontDesc's normalization is now queried; nothing ever reached it before.
    • Second resolve: it now checks the size table and the source-family tags it promised to keep.
    • Duplicate case: a self-<use> case that duplicated another is gone.
  • Guards: templateFor() results, getFontFiles()[0], and the GL cases' freetypes are required before they are read.

LLFontGL and LLFontTextCache (b4e7499)

  • llfonttextcache:
    • CHECK(true): five are gone. Two cases now pass by returning; the toggles case reads each toggle's effect off a draw.
    • Duplicate case: one is gone.
    • The generation tick: it now takes a glyph nothing has drawn, and checks that the cache measured again.
    • Two caches: they now measure different texts, so a shared slot would show.
    • Render cases: the five checked only counts. Each now shows which way its draw went, through regenCount() and the pixels: replay, recolor in place, or rebuild.
  • llfontgl:
    • VCENTER: its case renders it now.
    • The reflow bound: it now counts lines rather than shapes.
    • The run-end case: it can no longer pass having measured nothing.
    • Frame compares: they report the first differing pixel instead of 262,000 values.
    • Guards: fonts, freetypes and caches are required before they are read.

Shaping and COLRv1 (585512b)

  • alfontshaping:
    • Font that never shipped: three cases that asked for it now load the CJK face fonts.xml uses, and run.
    • Cache cases: they now check hits and drops, not just glyph counts.
    • GPOS branch: the combining-mark case now reaches it, and checks the mark's offset.
    • Smaller fixes: the codepoint bound and the load-flags check. An unused variable is gone.
  • alfontshaping_gl: a kerning check after a SKIP for the same condition can fail now.
  • alfontcolrv1:
    • Second paint: now compared with a fresh painter's.
    • Size bound: now three times the point size, as its comment says.
    • No-scale case: zeroes the scale, so it reaches the 2-em fallback.
    • Black-floor case: now runs, and checks every black pixel.
    • Palette case: its count is checked before painting. Noto-COLRv1 ships one palette, so the case still skips.
  • alfontcolrv1_gl: the force-monochrome flag is restored however the case ends.
  • Guards: faces, HarfBuzz fonts and bitmaps are required before they are read.

FreeType, bitmap caches, UI transform (8e022b5)

  • llfontfreetype:
    • CHECK(face->isValid()): it could not fail. It is now a required non-null face, or gone where one was already required.
    • The dark-palette case: now checks the index FreeType reports.
    • The CJK case: now runs.
    • hasSvg: gains a positive case.
    • The functor case: now uses a codepoint both fonts have.
    • The refcount case: reads a reload off the atlas generation rather than an address.
  • llfontfreetype_gl:
    • The throttle case: moves the frame clock, so the throttle shows.
    • Two "not released" checks: they now reject an index past the end.
  • llfontbitmapcache: "a reset bumps the generation" read the generation before init()'s own reset.
  • lluitransform: the Y scale is now checked after a pop.

GL objects (fae1eb9)

  • llglslshader_gl:
    • Uniform readers: they never checked a uniform's location, so every expected zero passed for a missing one. Each location is now required.
    • Binding points and texture units: required before use.
  • llimagegl:
    • The mip chain: now read back.
    • A binding read: now goes through LLRender's slot.
    • The texel reader: now bounded by GL's size.
    • Readback sizes: checked in full.
    • destroyGLTexture: the image now takes a new upload.
  • algpubuffer_gl: a failed read is now reported.
  • alinfiniteprojection_gl: a failed REQUIRE no longer leaks its query.
  • alwindowsdlheadless: the worker now records whether its shared context became current, and the test's thread requires it.
  • Comments: alimageslot_gl's fixture comment is corrected.

Related Issues

  • Please link to a relevant GitHub issue for additional context.

Issue Link: none. The bugs were found by #468's idiom pass and listed in bb3f752's message.

Checklist

  • I have provided a clear title and detailed description for this pull request.
  • If useful, I have included media such as screenshots and video to show off my changes.
  • I have tested the changes locally and verified they work as intended.
  • All new and existing tests pass.
  • Code follows the project's style guidelines.
  • Documentation has been updated if needed.
  • Any dependent changes have been merged and published in downstream modules.
  • I have reviewed the contributing guidelines.

Additional Notes

  • Tested on macOS arm64, RelWithDebInfo, on a GL 4.1 core context:
    • all 32 llrender entries pass, the three combined runs among them;
    • each binary, run whole, passes 10 random orders and 20 repeats with no state guard report;
    • clang reports no unused entity in the changed files beyond two in-class defaulted destructors, which GCC does not flag.
  • Each new or corrected check fails when it should. 146 small changes to the tests' own inputs or expected values were made, each reverted after.
    • What counted as failing: a check failing at the check itself, with the message it should show. A crash did not count.
    • Restores: where a fix restores state however a case ends, the change also had to leave no state guard report.
    • Result: all 137 that can run here failed as they should.
    • Not run here: 9 are for cases that need compute, clip control or a second COLRv1 palette, which this Mac's context and fonts lack. CI's Linux and Windows rows run those cases.
  • Not compiled here: alwindowsdlheadless's Windows (WGL) and Linux (EGL through SDL) branches. They ask the platform the same way llsdl makes the shared context.
  • Counts:
    • Cases now running: six cases that always skipped here run now: the four that asked for the unshipped Source Han Sans, llfontfreetype's dark-palette case, and the COLRv1 black-floor case.
    • Cases removed: two duplicates, so llfontregistry has 95 cases and llfonttextcache 23.
    • Assertions: they rose where guards and checks were added, for example llfonttextcache from 76 to 93 and llglslshader from 1,034 to 1,128.
  • Merge order: this should follow Test framework, stage 3: llrender's tests in Catch2 #468. Once Test framework, stage 3: llrender's tests in Catch2 #468 merges, its base becomes develop.

🤖 Generated with Claude Code

RyeMutt and others added 5 commits October 13, 2026 08:33
@RyeMutt @claude
llrender's font registry tests check what they say they check
3fa253c 
Old test bugs TM22's idiom pass found (bb3f752), fixed.

- llfontregistry:
  - "operator< orders descriptors that differ only in style" gave both
    the same size, so whether style is compared before size was never
    tested; the lower style now has the later size;
  - getMatchingFontDesc's normalization was never queried: a BOLD
    style and a "SansSerifBold" query now need it. The comment's
    "SansSerifHuge" could not work: normalizing moves Huge into the
    size, so the query names a head, not a template;
  - a second resolveFontReferences promised to keep the size table and
    the source-family tags and checked only the file count; it checks
    both now;
  - the sweep case's CHECK(true) is gone: the case passes by
    returning, and now checks the map holds the entry it walks;
  - a family that <use>s itself was tested twice, once under a comment
    naming a 'self' alias there is none of; the second is gone;
  - templateFor() results and getFontFiles()[0] are required before
    they are read.
- llfontregistry_gl:
  - each case requires its font's freetype before reading through it,
    and case 1's existing REQUIRE moved ahead of the first read;
  - after an eviction, a glyph's entry naming no sheet (Unspecified,
    -1) passed the "not released" checks; the entry is checked to
    name a Color sheet first.

Each new or corrected check fails under a small change to the test's
own input or expectation, at the check and not by crashing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@RyeMutt @claude
LLFontGL's and LLFontTextCache's tests check what they say they check
b4e7499 
Old test bugs TM22's idiom pass found (bb3f752), fixed.

- llfontgl:
  - the vertical alignment case's comment named VCENTER, which it never
    rendered; it renders it now;
  - the reflow bound took a wrap's shape count for its line count, so
    "two shapes a line" was looser than it said; each width's wrap now
    counts its lines;
  - the run-end case passed with no assertion if no sample had a
    width; it checks that some were measured;
  - two frame comparisons compared 256x256 pixel vectors with ==, so a
    failure printed some 262,000 values; they count the pixels that
    differ and show the first;
  - freetypes, bitmap caches and fonts are required before they are
    read;
  - the file's header said each case had a registry of its own; the
    registry is the binary's.
- llfonttextcache:
  - five CHECK(true)s: two cases now pass by returning, as they meant;
    the toggles case reads each toggle's effect off regenCount() and a
    draw;
  - "resets twice and outlives a toggle" repeated another case and the
    toggles case, and is gone;
  - the generation-tick case rasterized É, which an earlier case could
    already have drawn, and never checked that the cache measured again;
    it takes a glyph nothing has drawn and reads the shaper;
  - "a reset leaves another cache alone" measured the same text in
    both, so a shared slot could not show; they measure different texts;
  - the five render cases checked only counts. Each now shows which way
    the draw went, through regenCount() and the pixels it left: a
    repeat replays, a colour change recolors in place (and draws red),
    a move rebuilds by 100 pixels, a style flip rebuilds, and with the
    fast path off a colour change rebuilds. The file's header no longer
    says the harness cannot test that.

Each new or corrected check fails under a small change to the test's
own input or expectation, at the check and not by crashing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@RyeMutt @claude
llrender's shaping and COLRv1 tests check what they say they check
585512b 
Old test bugs TM22's idiom pass found (bb3f752), fixed.

- alfontshaping:
  - three cases asked for SourceHanSans-Regular.woff2, which the font
    package does not ship, so they always skipped; they load face 0 of
    SourceHanSans-VF.otf.ttc, which fonts.xml's CJK font loads, and run;
  - the cache cases checked a repeat's glyph count, never that it hit
    the cache, nor that a cleared face's entry was dropped; they read
    the shape cache's size and mutation count, and compare the glyphs
    themselves. a_addr, set and never read, is gone;
  - the tag-sequence bound compared a byte count where it meant
    codepoints;
  - the combining-mark case's GPOS branch never ran (DejaVuSansMono
    composes a + U+0301), and checked no offset; a q + U+0301 probe,
    which cannot compose, now reaches it, and its x offset is checked;
  - the load-flags check compared FT_Int32 with int, which cannot
    differ; it loads at light hinting and checks HarfBuzz is given it;
  - fonts' faces are required before they are read, and the variation
    case's FT_MM_Var is owned, so a failure does not leak it.
- alfontshaping_gl: the kerning check followed a SKIP for the same
  condition, so it could not fail; DejaVuSans kerns, and the SKIP is
  gone.
- alfontcolrv1:
  - "a second paint" never compared the two; it compares the bytes
    with a fresh painter's;
  - the size bound said three times the point size and checked four;
  - the no-scale case zeroed the ppem, already 0, not the scale its
    comment named, so it never reached the 2-em fallback; it zeroes the
    scale now and checks the surface the fallback sizes;
  - the black-floor case skipped for want of a pure-black pixel and
    checked only the first; it paints U+1F576, which has 28, and checks
    every one;
  - the palette case's count check followed a SKIP for the same
    condition; the count is checked before painting. Noto-COLRv1 ships
    one palette, so the case still skips here;
  - faces, HarfBuzz fonts and bitmaps are required before they are read.
- alfontcolrv1_gl: the force-monochrome case checked its own ternary;
  it asks for Grayscale directly, and sForceMonochromeEmoji is put back
  however the case ends. Caches, faces and HarfBuzz fonts are required
  before they are read.

Each new or corrected check fails under a small change to the test's
own input or expectation, at the check and not by crashing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@RyeMutt @claude
llrender's FreeType, bitmap cache and UI transform tests check what t…
8e022b5 
…hey say they check

Old test bugs TM22's idiom pass found (bb3f752), fixed.

- llfontfreetype:
  - CHECK(face->isValid()) could not fail: getOrCreateFace returns null
    for a face that does not load. Where nothing required the face, it
    is a REQUIRE that it is not null; where something did, it is gone.
    One stays, after resetBitmapCache, which a broken reset would fail;
  - faces loadFt returned are required before they are read;
  - the dark-palette check followed a SKIP for the same condition; the
    expected index now comes from FreeType's palette flags, and the
    global flag is put back however the case ends;
  - the CJK fallback case asked for a font the package does not ship,
    so it always skipped; it loads face 0 of SourceHanSans-VF.otf.ttc;
  - hasSvg had no positive case; TwemojiSVG is one;
  - the functor case's 'a' was not in TwemojiSVG at all, so the
    functor was never what kept the fallback from it; it uses U+2600,
    which both fonts have;
  - the refcount case compared addresses, which a freed and reused
    block can repeat; a reload is read off the atlas generation;
  - comments promised a match with FT_Get_Char_Index and a cleared
    atlas; both are checked now, and a third comment is corrected;
  - a CHECK(true) is gone.
- llfontfreetype_gl:
  - the throttle case could not see the throttle, since nothing was
    idle; it moves the frame clock (and puts it back) so the second call
    comes past the idle threshold but within the interval;
  - two "not released" checks also held for an index past the end; the
    index is checked to name a sheet.
- llfontbitmapcache: the generation was read before init(), whose reset
  bumps it, so "a reset bumps it" passed even if reset did not.
- llfontbitmapcache_gl: two comments still said vertex buffers compare
  the global generation.
- lluitransform: after a pop only the X scale was checked.

Each new or corrected check fails under a small change to the test's
own input or expectation, at the check and not by crashing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@RyeMutt @claude
llrender's GL object tests check what they say they check
fae1eb9 
Old test bugs TM22's idiom pass found (bb3f752), fixed.

- llglslshader_gl:
  - the uniform readers never checked a uniform's location, so a
    missing one read zeros, and every zero expected passed; each
    location is required, and so is a list of at most four values;
  - storage binding points and texture units were used unchecked,
    where a -1 would end the process in an llassert or leave a
    check passing for want of anything to read; they are required.
- llimagegl:
  - the mip chain case checked mip 1's width, which glTexStorage2D
    gives every level whether or not mips were made; it reads mip 1
    back and checks it holds the base's colour;
  - a binding was read just after the test bound the same name
    itself; the texture is bound through LLRender's slot now;
  - the texel reader indexed unchecked, sized from the image's size,
    not GL's; it reads GL's and fails on a texel outside it;
  - a readback was compared over the full size after checking only
    its width; its height and components are required too;
  - "destroyGLTexture leaves the image usable" never used it; it takes
    a new upload.
- algpubuffer_gl: readAll ignored read()'s result, so a failed read
  showed only as 0xFFFFFFFF words.
- alimageslot_gl: its fixture comment named the wrong targets and the
  wrong reason to skip.
- alinfiniteprojection_gl: each occlusion query was deleted only at
  its case's end, so a failed REQUIRE leaked it on the shared context;
  the fixture owns it.
- alwindowsdlheadless: the worker never checked that its shared
  context became current, so with none its GL calls did nothing and
  its error said nothing. It records what the platform reports (WGL,
  CGL, or EGL through SDL, as llsdl makes the context), makes the
  texture only then, and the test's thread requires it. A REQUIRE that
  guarded nothing is a CHECK.

Each new or corrected check fails under a small change to the test's
own input or expectation, at the check and not by crashing. Most of
llglslshader's and all of alimageslot's and three of
alinfiniteprojection's run only where the context has compute or clip
control, so their changes were compiled here and not run.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

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

Next included review available in 52 minutes.

Check out review usage here.

View limit details

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

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 90aa2592-627a-4faf-bf6c-8c1d3239c2db
📥 Commits

Reviewing files that changed from the base of the PR and between 5b26ba4 and fae1eb9.

📒 Files selected for processing (19)
  • indra/llrender/tests/alfontcolrv1_gl_test.cpp
  • indra/llrender/tests/alfontcolrv1_test.cpp
  • indra/llrender/tests/alfontshaping_gl_test.cpp
  • indra/llrender/tests/alfontshaping_test.cpp
  • indra/llrender/tests/algpubuffer_gl_test.cpp
  • indra/llrender/tests/alimageslot_gl_test.cpp
  • indra/llrender/tests/alinfiniteprojection_gl_test.cpp
  • indra/llrender/tests/alwindowsdlheadless_test.cpp
  • indra/llrender/tests/llfontbitmapcache_gl_test.cpp
  • indra/llrender/tests/llfontbitmapcache_test.cpp
  • indra/llrender/tests/llfontfreetype_gl_test.cpp
  • indra/llrender/tests/llfontfreetype_test.cpp
  • indra/llrender/tests/llfontgl_test.cpp
  • indra/llrender/tests/llfontregistry_gl_test.cpp
  • indra/llrender/tests/llfontregistry_test.cpp
  • indra/llrender/tests/llfonttextcache_test.cpp
  • indra/llrender/tests/llglslshader_gl_test.cpp
  • indra/llrender/tests/llimagegl_test.cpp
  • indra/llrender/tests/lluitransform_test.cpp
  • 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.

@RyeMutt
RyeMutt added this pull request to stack #470 October 13, 2026 18:14
@RyeMutt
RyeMutt merged commit 9649a04 into develop Oct 13, 2026
15 checks passed
@RyeMutt
RyeMutt deleted the rye/tm22-test-fixes branch October 13, 2026 20:20
@RyeMutt RyeMutt mentioned this pull request Oct 14, 2026
Merged
5 of 9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

c/cpp llrender

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@RyeMutt