Return a test run's results instead of scraping them out of its logs - #797
Open
Quenty wants to merge 10 commits into
Open
Return a test run's results instead of scraping them out of its logs#797Quenty wants to merge 10 commits into
Quenty wants to merge 10 commits into
Conversation
An executed script's return value had nowhere to go: the plugin discarded everything pcall handed back, and a caller could only learn what a run produced by reading its printed output. Anything a caller must read back exactly — test counts, for instance — had to be scraped out of log text. The plugin's execute action now marshals every returned value onto scriptComplete's payload.returnValues, and it surfaces on both ExecResult and StudioBridgeResult. Tables are walked recursively (array-like tables stay arrays), Roblox datatypes reuse the SerializedValue shapes, and cycles, over-deep nesting and non-finite numbers are marked rather than producing JSON the server cannot decode. The field stays absent whenever no scriptComplete arrived, which keeps "we never learned what it returned" distinct from "it returned nothing". Nothing consumes the new field yet.
…sult
Test runs learn everything about themselves by scraping the engine's log
output, and Open Cloud truncates those logs on a long run — a script
printing 20,001 lines came back with 7,627 of them, and the window size
varies, so there is no threshold to stay under. Results have to travel as
a value instead of as text.
ScriptRunResult gains returnValues, filled in by both transports: the
cloud context reads the task's output.results, the local context takes
what the Studio bridge marshalled. LuauTask.output was typed as an array
of { value?: string } wrappers, which does not exist — the results are a
flat array of natively typed values, so a returned Lua table is real
nested JSON. Nothing read it, so nothing noticed.
returnValues is absent, not empty, when a transport delivered no result
at all: an oversize return value fails a cloud task outright rather than
truncating it (~4MB fails, ~2MB arrives intact), leaving no output and no
error message, and a later consumer needs to tell that apart from a
script that returned nothing so it knows when falling back to the logs is
worth it.
Nothing consumes the new field yet.
A test run learned everything about itself from its own printed output, and the engine truncates a long run's logs — so the counts went missing on exactly the runs where they matter most. The runner had them all along and threw them away: runTestsIfNeededAsync returned a boolean, and a failing suite was announced by calling error(), which means the failing case never reached a return at all. runTestsIfNeededAsync now returns a TestRunResults table — the verdict, the test and suite counts, and a capped list of failed test names with their first message — or nil when no test run was attempted, which is still how a real game server tells itself apart from a test place. Every path that used to error() fills the table in instead, including a rejected Jest run and an interrupted one. The table carries only strings, numbers, booleans and tables of those, which the cloud and the Studio bridge marshal identically, and the failure list is capped because an oversize return value fails the whole run rather than arriving truncated. Nothing outside Open Cloud reads the returned table, so the local path still carries its verdict out through ProcessService:ExitAsync — now driven by the results rather than by whether _runTestsAsync threw. Test scripts return what they get: local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) if results then return results end A script still written the old way returns nothing and keeps working, since a table is as truthy as the boolean it replaced.
…s logs The runner returns its results now, so the CLI reads them. That closes the hole the change opened: a failing suite used to fail the cloud task by throwing, and without reading the returned verdict a failure whose jest report fell outside the truncated log window would have come back a pass. runSingleTestAsync takes the verdict and the counts from the returned results when a run produced them, falling back to log scraping when it did not. It also accepts returned results as proof the runner ran, which the required jest report was only ever standing in for. Tracebacks still fail a run whatever the results say — jest cannot count a deferred-callback crash. The batch runner captures what each package's script returns and folds the counts into the summary it already prints, so both the PASS/FAIL marker and the summary's success field come from the run's own verdict rather than from whether the script threw. The parser prefers those counts over the ones in a package's log section, reports the reason the runner gave instead of calling every failure a Luau error, and refuses to believe a summary that claims a pass alongside failed tests. Only counts travel in the summary: it is one log line, and the failure text is already in the section. A package whose test script returns nothing behaves exactly as before.
Test Results
73 packages tested, 54 passed, 19 failed in 3m15s Deploy Results
1 package deployed, 1 passed, 0 failed in 22.9s · View logs |
Jest.runCLI resolves with { globalConfig, results } and every count lives on
the inner results. This module read them off the outer wrapper, where they
are all nil — so the numFailedTests > 0 check it used to gate error() on
never fired once in its whole life, and test failures were only ever caught
by scraping jest's printed summary. Carrying that read forward into the
returned results table made the table itself inert: zero failures over zero
tests, reported as a pass, on a run that failed.
Nothing about that was loud. A nil count defaults to zero, and zero failures
is what a clean run looks like, so a wrong read is spelled identically to
good news. _resultsFromJest now accepts either shape — wrapper or bare
AggregatedResult — so a jest-lua change cannot silently zero the counts
again, and it fails closed when it recognizes neither: a shape this cannot
read has no counts, and a run with no counts is not a pass. A zero
numTotalTests is still a readable result, since a package with no specs
legitimately ran and found nothing.
The counts are now also printed. That line is the one piece of evidence that
survives whatever happens to the return channel, and its absence is how you
tell a structured read that worked from one that did not.
The structured results channel shipped inert and every check passed. A run whose counts were scraped out of log text produces output identical to one that returned them, so the fallback was invisible: the counts came back all zeros, the verdict quietly fell back to scraping, and the only evidence was failure reasons whose wording happened to be the log parser's. Both readers now state provenance rather than implying it. BatchPackageResult and SingleTestResult carry countsSource, the batch parser reports how many packages returned counts on every run, and falling back to scraping is a warning naming the packages — louder than the success path, since it is the case someone has to act on. A run whose returned totals disagree with its own jest report warns too: that mismatch is exactly what the zeroed counts looked like, and nothing was watching for it. Aggregated batch mode was also dropping the counts it had. One execution covers every package, so the batch runner splits per-package results into the summary and the log parser reads them back — but runSingleTestAsync then re-scraped each section and threw the structured counts away. The parser hands them over on ScriptRunResult.testResults instead, which the runner prefers over decoding a return value that belongs to no single package, and which also tells it the batch already reported provenance so it does not repeat it per package. The summary carries ranJest so a zero count is never mistaken for "no tests found".
…field Every package failed, including ones where every test passed. The verdict included `result.success ~= false`, and jest-lua's AggregatedResult.success is inverted: TestScheduler assigns `anyTestFailures or snapshot.failure or anyReporterErrors` where upstream jest negates that whole expression (TestScheduler.lua:434). So it is false on a clean run and true on a failing one, and a passing suite was the only kind that could falsify the conjunct. That field is now not consulted at all. Reading it in the other sense would work today and break the day the missing `not` is restored, so the verdict reads the underlying signals instead: failed tests, failed and runtime-error suites, an interrupted run, and a failed snapshot check — which was the only thing `success` covered that the counts do not. A reporter error leaves no trace on the result and is simply unavailable; nothing else it contributed is lost. The failure reason was a design flaw in its own right: it formatted the counts into "%d test(s) and %d test suite(s) failed" whether or not they were zero, so a failed package announced itself as "0 test(s) and 0 test suite(s) failed". A reason asserting nothing failed reads as neither verdict, and it is what made the bug behind it hard to see. It is now assembled from the causes that actually hold, and names its own confusion when none of them do, so a reason built out of zeros cannot be constructed.
…rives Clean counts on every package with every package failed is the signature of a runner reading the wrong field, not of broken tests — and the readers had nothing to say about it. The failure reasons looked ordinary and the counts were right, so the only clue was that the numbers and the verdict disagreed and nobody was comparing them. Both readers now describe a failure its own counts do not account for: zero failed tests, zero failed suites, and a failure anyway. The verdict is not overturned — a runner may know something its counts cannot express, an interrupted run or a snapshot check — so this reports rather than corrects, and the batch parser names every package it happened to, since all of them at once is the shape that identifies the cause. structuredFailureReasons also stops repeating itself: the runner's own message restates the counts whenever a count explains the failure, so it is only included when nothing else can say why.
Quenty
force-pushed
the
users/jamesonnen/structured-test-results
branch
from
August 12, 2026 00:43
50dbc21 to
a79ce46
Compare
…ounting suites
Two ways this could still report a pass on a failing run, and the reason it
kept happening.
findAggregatedResult accepted any table with a numeric numTotalTests, and every
other count defaulted to 0 when it could not be read. So a jest-lua rename of
numFailedTests, numFailedTestSuites, numRuntimeErrorTestSuites or
wasInterrupted reproduced the original bug exactly: all counts zero, verdict
pass, green CI. Not hypothetical — the port already spells the per-suite field
numFailingTests next to the aggregate numFailedTests. Validating field names
one at a time only ever covers the names known when the check was written, so
the sum is asserted instead: total == passed + failed + skipped holds by
construction (helpers.lua:91 derives numTotalTests from exactly those
per-suite counts), and it catches a rename of any count, named or not. Every
count the verdict reads must now also be present and numeric, and a mismatch
fails closed.
suitesFailed added numFailedTestSuites to numRuntimeErrorTestSuites, but
jest-lua increments both for one suite that failed to run — the runtime-error
counter unconditionally, then numFailedTestSuites through the separate
`numFailingTests > 0 or testExecError` branch. One broken suite counted twice,
and a single-spec package reported "2 of 1 suite(s) failed". It reports
numFailedTestSuites now and keeps numRuntimeErrorTestSuites as its own verdict
term, which is the only count that sees a suite both skipped and broken.
Both bugs, and the two before them, were untestable rather than subtle: a
module-scope require("Jest") stops the whole file loading in any harness
without Jest and the Nevermore loader. The logic moved to NevermoreTestResults
— no loader, no Jest, no print or warn, plain tables in and out — and
NevermoreTestRunnerUtils keeps the Roblox-facing half and re-exports the types,
so callers see no change. A dependency-free Luau module needs no loader line;
312 of the repo's package modules already omit it.
The tests run under Lune, from the package: npm test. 44 assertions, no harness
and no mocks, covering both directions of every verdict term and the shapes
that must fail closed. Their proper home is a jest test target, which needs a
placeId this cannot mint; Lune needs no credentials, so Lune is what exists.
Also fixes a red CI job: a doc comment on a local function used --[=[ … ]=],
which moonwave-extractor treats as a doc block and rejects with "Function
requires @Within tag" — and it aborts on the first diagnostic, so nothing else
was being checked either. Local helpers use --[[ … ]] now, and *.test.luau is
excluded from lint:luau since @lune/* requires cannot resolve under luau-lsp.
… report The batch path never relaxed requireTestReport, so the structured channel changed nothing in the case it exists for. A CI batch puts dozens of packages through one log window: a section keeps its END marker long after its jest summary was dropped, and the parser then failed the package with "no jest report in output — nothing proves any test ran" while its summary carried 311/311. A package whose section was lost entirely failed with "no output could be attributed". Both now defer to returned counts, which are that proof directly rather than a stand-in for it, and both still apply unchanged to a package that returned none. Judging a package on counts alone is a narrower verdict than judging it with its log — nothing checks it for tracebacks, which jest cannot count and only the log shows — so those packages are named in a warning rather than passing quietly. ranJest is now consulted instead of merely transported. A package with a jest.config on disk whose run reports it never reached jest did not test anything: the built place lost the config, and a Rojo regression that stops shipping it would otherwise turn a real suite into a silent green, since a smoke-test result retires the required jest report. That fails, and jest running but finding no tests warns. A package with no jest.config still smoke tests as before.
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
1 participant
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.
A test run now returns a table with its verdict, test and suite counts, and a capped list of failed test names, instead of announcing a failing suite by throwing and leaving the counts to be scraped back out of log text that Open Cloud truncates on long runs.
nevermore testandnevermore batch testread that verdict, so a failing suite is detected structurally rather than by finding a jest summary line that may not have survived truncation. Every package's test script was updated to return its results; one that has not been updated returns nothing and behaves exactly as before.