Repository navigation
Test framework: fix the old test bugs in llcommon's tests, and LLApp::instance() after the app is gone - #460
Merged
Merged
Conversation
~LLApp ended LLCommon and left sApplication pointing at the app it was destroying, so LLApp::instance() handed out a deleted object to anything that asked afterwards. Every caller found runs while the app is alive, in the viewer and in the tests, so nothing reached it yet, but a test binary makes and ends apps in turn: lleventcoro's and lllogin's process apps end in the harness's teardown, and llapp_test ends one in every case. ~LLApp now clears it as it ends, when the app ending is the one it names. A new llapp_test case deletes its app and checks instance() is null; it fails without the fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- llstreamtools:
- skip_to_end_of_next_keyword's, get_word's, remove_last_char's and
fullread's return values were taken and never checked; they are;
- fullread compared one byte short of what it read, so the final
newline was never checked;
- the get_word/skip_to_next_word comparison's comments described a
hyphen the input never held: both streams used an underscore and
landed on the same word. The hyphen stream now shows get_word
splitting only at whitespace where skip_to_next_word splits at it;
- get_brace_count's case was empty, and there is no such function;
- wrong messages and comments: a lone quote is removed, not kept; a
stale "remainder of Second"; six unused locals.
- llstring:
- format's no-substitution branch checked s1 after format rewrote it,
not the key, and passed only because "?" and "[]" map to
themselves; it checks the key;
- the F32 and F64 conversions' limit checks were commented out, the
F64 one calling convertToF32. Each now accepts its type's largest
magnitude and rejects one past it;
- the first case tested std::string's own constructors, nothing of
ours, and goes; so do an unused local and stale comments.
- stringize: the wstringize case asserted nothing off Windows, its #else
commented out. It checks every type it can on every platform, and the
Windows checks stay as they were.
- llstring_utf: comments that no longer described the code: the
conversions it has (three, not six), isPictographBase's range (U+2000
is rightly rejected), and walkers and helpers since removed.
Each new or corrected check was shown to fail under a small change to the
test's own input or expected value. llstreamtools_test passes 123
assertions in 25 cases (103 in 26), llstring_test 371 in 46 (383 in 47:
the std::string case's 18 went), stringize_test 17 (10).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- llsdserialize:
- the cross-compatible "map" case began from an empty array, which
became a map only at its first key; it begins from an empty map;
- comments copied between cases, cut off, or claiming exceptions and
failures that never happen are corrected, and the binary size
labels say which way the size field is wrong;
- srand(ii) on every pass is kept, deliberately: it makes each pass's
buffer reproducible whatever drew from rand() before, as a comment
now says;
- found here: the navmesh case leaked its unzipped buffer whenever a
check failed. A unique_ptr with a free() deleter owns it.
- llsdserialize_python: when waitpid() failed with ECHILD the case
passed without the exit code it exists to check. It skips, saying the
code was taken by another waitpid(), as LLProcess allows a SIGCHLD
handler may; any other failure fails.
- llsd: a REQUIRE(true) inside the branch it meant to show was taken is
a flag set there and REQUIREd after.
- lleventdispatcher: three matchers had actual and expected swapped, so
a failure would have been reported backwards.
- lazyeventapi: "without a LazyEventAPI" passed whatever happened, and
its comment leaned on case 1 having run first. It makes and destroys
its own registrar, and shows the pump is not made after.
- llpounceable: the compile-fail case was empty and always passed. What
it documents is a compile error, so it is a comment, and the case
goes; uncommenting the line still fails to compile.
Each new or corrected check was shown to fail under a small change to the
test's own input or expected value, and the reaping of the Python child
by another waitpid() to skip each case. llpounceable_test has six cases
(seven).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… they say they check - alregex: a check passed whenever firstByteRange() answered false. It now expects false where any byte may start a match, and a new check expects the range where a word must. - alrecoverystore: three front() calls on lists whose size nothing checked, and two writes whose result nothing checked; each is REQUIREd or CHECKed. A taken entry was never checked to be there. - llinstancetracker: a key snapshot's iterator was dereferenced three times with its size unchecked; the keys are compared whole. A throwing constructor's object leaked when it did not throw, and nothing checked that it threw: CHECK_THROWS_AS. - llsingleton: the first case was empty and goes. A `throw;` with no exception active could not run: initSingleton()'s getInstance() returns the instance being made, which the case now checks. require_order's REQUIREs skipped deleteAll() when one failed, leaving the singletons for later cases; they are CHECKs. - llunits: the U32 unit was built from the F32 value where the case meant the signed integer one, and an explicit cast stood where the case meant an implicit conversion. accept_explicit_quatloos always returned true; it returns what it was given. - lluuid: the XOR case claimed ^= and commutativity and tested neither; it tests both. A set was iterated with its size unchecked. - lltrace: comments named cups and powers of two the code doesn't use. - lltimer: bounds of 0.04 s and 5 ms that a loaded machine could miss. The expiry case sleeps 250 ms against bounds of 0.2 s; each totalTime() read is held between two steady_clock reads with 1 ms of slack, so a thread kept off the processor no longer fails it, while a clock 0.5% fast or slow still does. Each new or corrected check was shown to fail under a small change to the test's own input or expected value. alrecoverystore_test passes 301 assertions (295), alregex_test 137 (133), lluuid_test 36,997 (36,993), lltimer_test 13 (12), llinstancetracker_test 30 (32: three dereferences are one comparison), llsingleton_test 37 in 14 cases (36 in 15). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…say they check
- lldeadmantimer: in the CPU-metrics block a second isExpired() call
left out the CPU arguments, so the two checks after it could not
fail.
- lldate: the parser boundary block, compiled out, used inputs that were
commented out, and its leap-day input was a valid date. The parser
does not reject out-of-range fields (timegm normalises them), so the
block stays out, now whole case and all, and says why. Found here: a
case claimed to check the RFC 1123 day of week and checked only a
length.
- llfile: sizeof a const char*, the pointer's size; and a case named for
isfile() after a write never asked it.
- commonmisc: &bin[0] on a vector not checked non-empty is a whole
comparison; a max_bytes shorter than the text it parses is that
text's length (the parser ignores it for quoted strings, so nothing
changes); dead stubs, an #if 0 block, an empty namespace and stale
headers go.
- llprocess:
- one find("ghi") was checked twice where the second meant from
offset 6;
- the "History:" dump printed only for a failed REQUIRE, inside a try
that a CHECK never left; it is a scoped INFO, so any failure shows it;
- five polling loops raced readfile(), which fails on an empty file,
against the script's truncating write: they poll with
readfile_if_present(), which reads that as not yet;
- Windows only, not compiled here: the autokill helper's process
handles leaked when a REQUIRE failed; they are owned.
- llleap: the reader script's end-of-input test could never be true, and
the mismatch report gave the last differing chunk, not the first.
test_or_split's search records each failing size, as Catch2 records a
failed REQUIRE before it throws; each now names its size.
Each new or corrected check was shown to fail under a small change to the
test's own input or expected value, and llprocess's script, made to
pause between truncating and writing, to pass. lldate_test passes 40
assertions in 9 cases (39 in 10: the empty boundary case is compiled out
with its block), llfile_test 149 (148).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Warning Review limit reachedYou'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 59 minutes. View limit details
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. Comment |
5 of 9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes the bugs in llcommon's own tests that the Catch2 conversion in #459 found and kept, since a conversion must not change what a test does. Each was a test that:
It also fixes one bug in production code that the conversion found:
LLApp::instance()dangling after the app is deleted.It stacks on #459 (
rye/tests-tm20). Apart fromllapp.cpp, it changes only test files.Production: 98982d2,
LLApp::instance()finds no app once it is gone.~LLAppended LLCommon but leftsApplicationpointing at the app it was destroying. Anything asking afterwards got a deleted object.~LLAppclearssApplicationas it ends, if it names the app being destroyed.Strings (dac4b3b)
std::stringis removed.LLSD and events (bd86330)
waitpid()reaped the child, the case passed without checking the exit code. It now skips and says why.REQUIRE(true).Registries, singletons, units, UUIDs, timers (b111002)
firstByteRange()said no.front()called on lists whose size was never checked;throw;;^=and commutativity claimed, never tested.Timers, dates, files, processes, LEAP (cdd2de1)
sizeofof a pointer;isfile()never asked.&bin[0];max_bytes;find()checked twice;Related Issues
Issue Link: none. The bugs were found by #459's idiom pass and listed in c8b7e58's message.
Checklist
Additional Notes
ctest -LE manualpass, and so do all 10 pending combined runs.🤖 Generated with Claude Code