Test framework: fix the old test bugs in alscript's and lscript's tests by RyeMutt · Pull Request #463 · AlchemyViewer/Alchemy

RyeMutt · 2026-10-13T08:02:15Z

Description

Fixes the bugs in alscript's and lscript's own tests that the Catch2 conversion in #462 found and kept, since 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;
read through what a lookup returned, or indexed what nothing had sized, so that a miss would crash the whole binary, taking every later test with it, rather than fail its own case;
had a message that could not say what it meant: built before the call that fills its reason, or taken by a different assertion.

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

Preprocessor (19b270f)

alpreprocessor:
- "yes4 on its line" compared a find() with itself. It now checks yes4 is on output line 19, where the source has it.
- Three messages were built before the call that fills their reason.
- Guards before reads that could fail, among them a problem said before problems.back() in the five cases that overrun a limit.
alincludesearch: problems and suggestions are sized before they are indexed.
aldiskcache: "kept a moment" could not fail. It held a reference to the list the cache keeps, which the next call refills, so it compared that list with itself. It now holds a copy.
alscriptlexicon: a line comment's end is checked against the text's length before the character there is read.

Luau and LSL services (752f807)

alluauservice and alluauconfig: messages built before their calls now come after them. The messages the idiom pass had cut for saying nothing are back where a failure needs them.
allslservice: a case's locals hid the fixture's error and loaded.
Guards:
- alluauservice: an outline's first entry, two lints' arguments, and an INFO that indexed past a signature's parameters;
- allslservice: the third problem's arguments;
- allsleffects: the matches and calls it reads;
- allslconsts: a declaration's position.
Leftovers: a doubled include and a doubled comment.

alluaufragment (36be676): its "builtins loaded" message was built before the load that fills its reason. CodeRabbit found it in its review of #462.

Lints and fixes (6f4d178)

Labels on the wrong assertion: a label written before CHECK(require_fixed(...) == ...) went to the helper's own first assertion, not to the CHECK. Each is now a scoped INFO around its CHECK: 52 in alscriptfixes, 58 in alscriptlintpass.
A helper run twice: alscriptfixes built four messages by calling an asserting helper, so it and its REQUIREs ran twice. It now runs once.
Guards before reading the first of the fixes offered.
One error string for every load: both fixtures wrote every load's error to it. Each load now has its own.

Disk masters, envelopes, messages (3fccb75)

Guards before the first read of each lookup:
- almasterindex: linkOf(), of(), and the index read back from disk;
- almasterwatch, almasterburst, almasterlinks, almodulelook;
- alscriptenvelope: lines and headers;
- alnotecardformat;
- alscriptformatter.
almessagemap: every match shared one Match, so a failed match showed the one before it. Each match now has its own, and checks all of its arguments, so it also checks that the module name is left blank, as the case's name says.

Conversion, optimizer, weights (212f053)

allsltoslua:
- an order check looked for ll.OwnerSay, which the conversion writes as print, so it compared against "not found" and could not fail;
- two order checks passed when what they ordered was missing;
- one block of checks ran only if a span happened to start on line 4.
allsloptimizer: "Mono: what it wrote weighs what it says" weighed nothing. It now checks the weight against the weigher's, and the l = l check it replaced keeps a message of its own.
alscriptweight:
- messages built before their calls;
- two messages one check off their checks;
- count's end checked as "at least line 8", where it is line 10.
allslreferencecheck: a message built before the compile that fills it.

Related Issues

Please link to a relevant GitHub issue for additional context.

Issue Link: none. The bugs were found by #462's idiom pass and listed in e661a8e'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:
- all alscript and lscript entries pass, the combined runs and the new solver's reruns among them;
- each binary passes 10 random orders and 20 repeats with no state guard report, and so do the six new-solver files together under the new solver;
- clang reports no unused member in the changed files.
Each new or corrected check fails when it should. 133 small changes to the tests' own inputs or expected values were run, each reverted after. A file the new solver reruns had each change run in both runs.
- What counted as failing: a check failing at the check itself, with the message it should show. A crash did not count.
- Result: all 133 failed as they should.
- No such change exists for two fixes:
  - alluauservice's solver-switch messages: nothing in a test makes one solver refuse definitions the other took;
  - almodulelook's names guard: every module found has a name.
Counts: no case was added or removed. Assertion counts rose where guards were added, for example almasterindex 122 to 153 and almodulelook 30 to 41. alscriptfixes went from 865 to 858, since the helper no longer runs twice.
Merge order: this should follow Test framework, stage 3: alscript's and lscript's tests in Catch2 #462. Once Test framework, stage 3: alscript's and lscript's tests in Catch2 #462 merges, its base becomes develop.

🤖 Generated with Claude Code

Old test bugs #462's idiom pass found, fixed. - alpreprocessor: - "yes4 on its line" compared a find() with itself; it now checks yes4 is output line 19, where the source has it; - three messages were built before the call that fills their reason (the grid builtins' load, and the @line reader's twice): each call comes first now; - guards before reads that could fail: a source location found before its file is read, the size-weighed note's five arguments, a module note's three, and a problem said before problems.back() in the five cases that overrun a limit (CHECK(r.overran) alone did not say there was one). - alincludesearch: the problems an explanation adds, and the suggestions in ./lib/, are sized before they are indexed. - aldiskcache: "kept a moment" could not fail. It held a reference to the list the cache keeps, which the next call for the same folder refills, so it compared that list with itself; it holds a copy now. The later list is sized before [1] is read. - alscriptlexicon: a line comment's end is checked against the text's length before the character there is read; s.closed was no guard, since a line comment is always closed. 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>

Old test bugs #462's idiom pass found, fixed. - Messages built before the call that fills their reason, so they said nothing or an earlier call's error: in alluauservice, the definitions' loads, a switch of solver and back, the docs' load and a configuration's parse; in alluauconfig, the parses and the script runs. Each call now comes first, and the messages the idiom pass had cut for saying nothing are back where a failure needs them. - allslservice: "another region's builtins" built its message before the load, from a local that hid the fixture's error; the second load's result hid the fixture's loaded. Both are renamed, and the load comes first. - Guards before reads that could fail: - alluauservice: an outline's first entry, and two lints' arguments; an INFO no longer indexes a signature's parameters by an active parameter that may be past them; - allslservice: the third problem's arguments; - allsleffects: the matches and the calls each case reads two of; - allslconsts: a declaration's position, before an iterator is made from it. - Leftovers: allsleffects included linden_common.h twice, and alluauservice had a comment line its next one repeated. 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, in both solvers' runs. Three restored messages (a switch of solver, back, and the other solver's load) have none: no change to the test makes one solver refuse definitions the other took. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Old test bugs #462's idiom pass found, fixed. - A label before CHECK(require_fixed(...) == ...) was taken by the helper's own first assertion: an unscoped message goes to the next assertion, which is the helper's, so a failing CHECK carried no label and the helper's carried the wrong one. Each is a scoped INFO around its CHECK now: 52 in alscriptfixes, 58 in alscriptlintpass. - alscriptfixes built four messages by calling require_acted_on, so the helper, its REQUIREs with it, ran twice; it runs once, and the message reads its result. - alscriptfixes, guards before reads that could fail: the fixes offered for a removal, a require, an include and a negation, each sized before front(). "Not offered a name declared after" read the first fix's second argument unchecked; it now checks no fix offers it. - Both fixtures wrote every load's error to one string, so a "builtins:" or "definitions:" message could show another load's error, or none: each load has its own. - alscriptlintpass: a comment about TUT's fifty-test limit goes. 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, in both solvers' runs. alscriptfixes runs 858 assertions where it ran 865: the helper no longer runs twice. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…y they check Old test bugs #462's idiom pass found, fixed. Most were reads through what a lookup returned, with nothing checking it found anything: a miss would have crashed the binary, taking every later case with it, rather than failing its own. - Guards before the first read of each lookup: - almasterindex: linkOf(), ALMasterLinks::of() and the index read back from disk (kept in a variable, so the pointer into it stays good); - almasterwatch: the folders asked about, and each period; - almasterburst: every dueAt(); - almasterlinks: of() and find(), and the links left after a remove, which nothing sized; - almodulelook: each module named, and its names; - alscriptenvelope: the code line in its text, the lines either side, and each upload header parsed; - alnotecardformat: each notecard named; - alscriptformatter: the lines a range asked for. - almessagemap: every match shared one Match, so a match that failed showed the one before it, and args[1] was in bounds only because an earlier match had filled args. Each match has its own, and each checks its whole args, which also checks the module name is left blank, as the case says. - alscriptformatter: "a line asked for" checked a line not asked for, as written, which is what formatEach promises; the message says so. 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. almodulelook's names guard has none: every module the look finds has a name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… they check Old test bugs #462's idiom pass found, fixed. - allsltoslua: - "state_entry's, kept" looked for ll.OwnerSay, which the conversion writes as print, so the order it checked was against npos and could not fail; it finds both, then checks the order; - two order checks passed when what they ordered was missing; each first checks it is there; - the "handler over both" checks ran only if some span started on LSL line 4; the span is required now. - allsloptimizer: - "Mono: what it wrote weighs what it says" weighed nothing: when Mono came to weigh a list's shapes against the helpers they bring in, its "l + a" check was loosened to "l = l" and given that message. Its weight is now checked against the weigher's, and "l = l" keeps a message of its own; - each ALLSLTraits::of() is required before it is read. - alscriptweight: - three messages were built before the call that fills their reason (a definitions load, two rewrites): each call comes first now; - "where it is declared" and "where it ends" stood one check off their checks; count's end was checked as at least line 8, and is line 10, its end; - a handler looked up by name is required before it is read. - allslreferencecheck (lscript): "Tailslide compiles it" was built before the compile filled its reason. 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, in both solvers' runs where the file has two. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

coderabbitai · 2026-10-13T08:02:26Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 2e6eff37-0075-45b0-bf88-5655c393214f

📥 Commits

Reviewing files that changed from the base of the PR and between 7312a9e and 36be676.

📒 Files selected for processing (25)

indra/alscript/tests/aldiskcache_test.cpp
indra/alscript/tests/alincludesearch_test.cpp
indra/alscript/tests/allslconsts_test.cpp
indra/alscript/tests/allsleffects_test.cpp
indra/alscript/tests/allsloptimizer_test.cpp
indra/alscript/tests/allslservice_test.cpp
indra/alscript/tests/allsltoslua_test.cpp
indra/alscript/tests/alluauconfig_test.cpp
indra/alscript/tests/alluaufragment_test.cpp
indra/alscript/tests/alluauservice_test.cpp
indra/alscript/tests/almasterburst_test.cpp
indra/alscript/tests/almasterindex_test.cpp
indra/alscript/tests/almasterlinks_test.cpp
indra/alscript/tests/almasterwatch_test.cpp
indra/alscript/tests/almessagemap_test.cpp
indra/alscript/tests/almodulelook_test.cpp
indra/alscript/tests/alnotecardformat_test.cpp
indra/alscript/tests/alpreprocessor_test.cpp
indra/alscript/tests/alscriptenvelope_test.cpp
indra/alscript/tests/alscriptfixes_test.cpp
indra/alscript/tests/alscriptformatter_test.cpp
indra/alscript/tests/alscriptlexicon_test.cpp
indra/alscript/tests/alscriptlintpass_test.cpp
indra/alscript/tests/alscriptweight_test.cpp
indra/lscript/tests/allslreferencecheck_test.cpp

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

📝 Summary

Summary by CodeRabbit

Tests
- Expanded checks for script diagnostics, code fixes, formatting, optimization, and source mapping.
- Added coverage for additional refactoring and lint scenarios, including extract-to-local, operator corrections, and deprecated-call rewrites.
- Strengthened checks around script loading, lookups, configuration, and timing behavior, helping catch unexpected results earlier.
- No end-user functionality changes are included in this update.

The pull request updates tests across script tooling, master tracking, fixes, and linting. It adds explicit checks before result access, stores operation results for assertions, and strengthens selected test expectations. No production behavior or public declarations change.

Changes: Script and compiler tests

Layer / File(s)	Summary
Script tooling and compiler test checks `indra/alscript/tests/aldiskcache_test.cpp`, `alincludesearch_test.cpp`, `allslconsts_test.cpp`, `allsleffects_test.cpp`, `allsloptimizer_test.cpp`, `allslservice_test.cpp`, `allsltoslua_test.cpp`, `alluauconfig_test.cpp`, `alluaufragment_test.cpp`, `alluauservice_test.cpp`, `almessagemap_test.cpp`, `almodulelook_test.cpp`, `alnotecardformat_test.cpp`, `alpreprocessor_test.cpp`, `alscriptenvelope_test.cpp`, `alscriptformatter_test.cpp`, `alscriptlexicon_test.cpp`, `alscriptweight_test.cpp`, `indra/lscript/tests/allslreferencecheck_test.cpp`	Tests now check lookup and collection results before accessing them, retain operation results for assertions, and strengthen selected output and optimizer checks. Fix tests also add or clarify several expected cases.
Master tracking test checks `indra/alscript/tests/almasterburst_test.cpp`, `almasterindex_test.cpp`, `almasterlinks_test.cpp`, `almasterwatch_test.cpp`	Tests validate optional deadlines, link lookups, and polling periods before inspecting their values. The watch test also checks the number and contents of look requests.
Fix and lint test coverage `indra/alscript/tests/alscriptfixes_test.cpp`, `alscriptlintpass_test.cpp`	Fixtures now keep builtin-load and definition-load errors separately. Tests scope diagnostic context and add fix-availability and Luau extraction cases; existing expected transformations remain in place.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: taylnos

Merge Risk: ⚪ Minimal · up to 36be6

This change only adjusts tests so they check what they claim to check, and it has no production impact. I found no concrete merge-blocking risk.

Pre-merge checks · 4 · 1

Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 25 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description check		The description is detailed and follows the repository template. It explains the test fixes, affected areas, validation results, related issue context, checklist status, and merge-order dependency.
Title check		The title clearly and concisely identifies the main change: fixing existing alscript and lscript test bugs.

Fix all pre-merge checks with AI

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

_{A rabbit checks each path with care,
Then tests the values waiting there.
It gathers fixes, neat and bright,
And guards each lookup from the night.
The carrot cache now passes green,
While moonlit tests keep fields pristine.}

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

Its message was built from error before loadBuiltins filled it, so a failed load said nothing; the load comes first now. Found by CodeRabbit's review of #462, the same mistake the other follow-up commits fix elsewhere. A missing builtins file now fails the check with the load's reason, in both solvers' runs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

RyeMutt and others added 5 commits October 13, 2026 03:19

github-actions Bot added c/cpp alscript labels Oct 13, 2026

RyeMutt added this pull request to stack #464 October 13, 2026 08:03

RyeMutt merged commit 12ce979 into develop Oct 13, 2026
16 checks passed

RyeMutt deleted the rye/tm21-test-fixes branch October 13, 2026 09:12

RyeMutt mentioned this pull request Oct 13, 2026

Test framework, stage 3: llrender's tests in Catch2 #468

Merged

5 of 9 tasks

Navigation Menu

Repository navigation

Test framework: fix the old test bugs in alscript's and lscript's tests - #463

Test framework: fix the old test bugs in alscript's and lscript's tests#463
RyeMutt merged 6 commits into
rye/tests-tm21from
rye/tm21-test-fixes

RyeMutt commented Oct 13, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 13, 2026 •

edited

Loading

Summary by CodeRabbit

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

RyeMutt commented Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Checklist

Additional Notes

Uh oh!

coderabbitai Bot commented Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

RyeMutt commented Oct 13, 2026 •

edited

Loading

coderabbitai Bot commented Oct 13, 2026 •

edited

Loading