diff --git a/docs/gotchas/tooling.md b/docs/gotchas/tooling.md index 4bcd9ecc25c..795fae8efb9 100644 --- a/docs/gotchas/tooling.md +++ b/docs/gotchas/tooling.md @@ -33,6 +33,26 @@ When a section grows to 10+ items, graduate it to its own doc. - **The watch service and the lock file do not share a version vocabulary.** A watch's `baselineVersion`, and the `currentVersion` it reports back, are the *asset-delivery content hash* for the place — that's what the service's Roblox driver reads, and it compares them by string equality. `deploy.nevermore.lock.json` holds the *Open Cloud place version* (an integer). They are never equal, so handing the lock's number over as a baseline reads as drift on the very first poll and dispatches a rebuild of the build that just shipped. The CLI therefore sends no baseline at all — the service's first poll adopts what it sees, which is what a baseline was for — and treats every version the service reports as an opaque "something moved" token, asking Open Cloud what the place is actually at before deciding to rebuild. Anything comparing a service version against a lock version is wrong even when the types line up. +- **Open Cloud truncates long engine logs, so anything that must be read back exactly belongs in the script's return value.** A script printing 20,001 lines came back with 7,627 of them — the last contiguous block, with the head dropped — and the size of the window varies run to run (6,715 and 3,236 lines on other runs), so there is no line count to stay under. A Luau execution task's return value is not subject to this: it arrives on the task as `output.results`, a flat array of the returned values (`return t, "s", 42` → three entries), and Roblox serializes them itself, so a returned Lua table is real nested JSON. Do **not** `HttpService:JSONEncode` the result — that lands as a double-encoded string. + +- **An oversize return value annihilates the task rather than truncating the value.** A ~2.1MB return value arrives complete and intact; a ~4.2MB one fails the whole task — state `FAILED`, no `output`, and no error message saying why. So code reading a return value has to treat "the task reported no output" as *unknown* rather than empty, and fall back to the logs; `getTaskReturnValues` in `open-cloud-client.ts` draws exactly that line (`undefined` = nothing came back, `[]` = the script returned nothing). + +- **`Jest.runCLI` resolves with a wrapper, not the results.** jest-lua's `runCLI(...)` resolves with `{ globalConfig, results }`; every count (`numFailedTests`, `numTotalTests`, ...) lives on the inner `results`, which is the `AggregatedResult`. Reading them off the outer table gets `nil` for all of them. This is not a loud failure: `nil` counts default to zero, and zero failures over zero tests is indistinguishable from a clean run — a `numFailedTests > 0` check written against the wrapper sat in `NevermoreTestRunnerUtils` for its whole life and never fired once, so test failures were only ever caught by scraping jest's printed summary. `_resultsFromJest` now accepts either shape and **fails closed** when it can recognize neither, because "shape I cannot read" must never be spelled the same way as "nothing failed". Note `numTotalTests == 0` is a readable result (a package with no specs) — only a *missing* count means the shape is unknown. + +- **`AggregatedResult.success` is inverted in jest-lua. Never read it.** `TestScheduler.lua:434` assigns `aggregatedResults.success = anyTestFailures or aggregatedResults.snapshot.failure or anyReporterErrors`, where upstream jest negates that whole expression. The field is therefore true exactly when the run **failed**, and false on a clean run — so `result.success ~= false` fails every passing suite, which is how a batch of four came back `0 passed, 4 failed` with three of them green. Do not read it in the other sense either: that depends on the missing `not` staying missing. Read the underlying signals instead — `numFailedTests`, `numFailedTestSuites`, `numRuntimeErrorTestSuites`, `wasInterrupted`, and `snapshot.failure`. (`anyReporterErrors` has no other trace on the result and is simply unavailable.) + +- **`numFailedTestSuites` already counts the suites that failed to run, so never add `numRuntimeErrorTestSuites` to it.** `helpers.lua` `addResult` increments `numRuntimeErrorTestSuites` for any suite with a `testExecError`, and then increments `numFailedTestSuites` through the `numFailingTests > 0 or testExecError` branch of a *separate* statement. Summing the two reports one broken suite as two — and `2 of 1 suite(s) failed` for a single-spec package. Report `numFailedTestSuites`, and keep `numRuntimeErrorTestSuites` as its own verdict term: a suite that is both skipped and broken lands in `numPendingTestSuites`, so it is the only count that sees it. + +- **Validate jest's counts with the invariant, not with a list of field names.** `total == passed + failed + skipped` holds by construction — `helpers.lua:91` derives `numTotalTests` as `numPassingTests + numFailingTests + numPendingTests + numTodoTests` and every aggregate accumulates the same per-suite fields. Checking the sum catches a rename or a move of *any* count, including ones the checking code does not know the name of; validating names one at a time only ever covers the names known when it was written. This matters because a missing count reads as `0`, and zero failures over zero tests is spelled exactly like a clean run. `NevermoreTestResults.fromJest` asserts the sum and fails closed on a mismatch. + +- **A failure reason must never be built by formatting counts that may be zero.** The bug above surfaced as the reason `0 test(s) and 0 test suite(s) failed` — a string that asserts nothing failed while failing the package, and which reads as neither a pass nor a failure. `_resultsFromJest` now assembles the reason from the causes that actually hold and falls back to naming its own confusion, so a reason that says nothing failed is unconstructible. Both CLI readers additionally warn when a run reports failure over counts where nothing failed: clean counts across every package with every package failed is the signature of a wrong verdict, not of broken tests. + +- **A module-scope `require("Jest")` makes everything in the file untestable.** All three bugs above shipped green out of one file, because `NevermoreTestRunnerUtils` requires Jest at module scope and so cannot load in any harness that does not have Jest and the Nevermore loader. The verdict logic now lives in `NevermoreTestResults`, which requires nothing at all, takes plain tables, returns plain tables and does no logging — so it runs under Lune (`cd src/nevermore-test-runner && npm test`). If a package's logic keeps producing bugs a test would catch, look at what its file drags in before writing another integration check. Note a dependency-free Luau module does not need the loader line, and 312 of the repo's ~1250 package modules already omit it. + +- **Documenting a *local* Luau function with `--[=[ … ]=]` breaks `lint:moonwave`.** moonwave-extractor treats any `--[=[` block as a doc comment and rejects one it cannot attach to a class: `error: Function requires @within tag`. It also aborts on the first diagnostic, so one bad block hides every other. Use plain `--[[ … ]]` for local helpers and keep `--[=[ … ]=]` for public members (or add `@within` explicitly, as `@prop` blocks do). + +- **A structured channel that is plumbed but not flowing looks exactly like one that works.** The first version of the results-return path shipped inert: the runner returned its table, the batch runner captured it, the parser preferred it — and because the counts were all zero and nothing said where they came from, the verdict silently fell back to log scraping and every check passed. Both readers now state provenance unconditionally (`countsSource`, plus an `info` line naming how many packages returned counts), warn when a run fell back to scraping, and warn when the two channels disagree about the same run's totals. When adding a channel that has a fallback, make using the fallback louder than using the channel. + - **`--script-text` loses everything after the first line when invoked through `npx` on Windows**: the `npx.cmd` shim truncates a multi-line argument, so `nevermore test --cloud --script-text '\n'` silently runs only line 1 (and prints `(no output)` when line 1 produced none). Either write the script as a single line with `;` separators, or bypass the shim: `node tools/nevermore-cli/dist/nevermore.js test --cloud --script-text '...'`, which passes newlines through intact. ## Claude Code hooks diff --git a/docs/testing/testing.md b/docs/testing/testing.md index 0c71c3f0bee..23c20be5ef7 100644 --- a/docs/testing/testing.md +++ b/docs/testing/testing.md @@ -394,13 +394,19 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end ``` Replace `mypackage` with the key used in your Rojo project tree. +Returning `results` is what tells the CLI how the run went. Test output is log text and the +engine truncates a long run's logs, so counts scraped back out of it are exactly what goes +missing on the runs where they matter most; returning the table sends them out as a value +instead. A script that returns nothing still works — the CLI falls back to reading the logs. + ### NevermoreTestRunnerUtils The `@quenty/nevermore-test-runner` package provides `NevermoreTestRunnerUtils`, which handles the test execution lifecycle: @@ -408,6 +414,9 @@ The `@quenty/nevermore-test-runner` package provides `NevermoreTestRunnerUtils`, - If a `jest.config` is found under the given root, it runs Jest tests - If no `jest.config` is found, boot success is the test (smoke test) - Detects Open Cloud vs local execution context and exits appropriately +- Returns a [TestRunResults](/api/NevermoreTestRunnerUtils) table — counts, a capped failure + list, and the run's verdict — or `nil` when no test run was attempted, which is how a real + game server tells itself apart from a test place and falls through to its normal boot ## Running tests diff --git a/package.json b/package.json index 2e24f2231c7..4b479ed931a 100644 --- a/package.json +++ b/package.json @@ -23,7 +23,7 @@ "build:ts": "pnpm -r --filter \"./tools/**\" --filter \"!./tools/nevermore-vscode\" run build", "format": "stylua --config-path=stylua.toml src games plugins", "format:ts": "prettier --ignore-path .gitignore --write \"tools/**/*.{ts,tsx,js,jsx}\"", - "lint:luau": "luau-lsp analyze --sourcemap=sourcemap.json --base-luaurc=.luaurc --defs=globalTypes.d.lua --flag:LuauSolverV2=false --ignore=**/node_modules/** --ignore=**/*.story.lua --ignore=**/*.client.lua --ignore=**/*.server.lua src", + "lint:luau": "luau-lsp analyze --sourcemap=sourcemap.json --base-luaurc=.luaurc --defs=globalTypes.d.lua --flag:LuauSolverV2=false --ignore=**/node_modules/** --ignore=**/*.story.lua --ignore=**/*.test.luau --ignore=**/*.client.lua --ignore=**/*.server.lua src", "lint:moonwave": "npx lerna exec --parallel -- moonwave-extractor extract src", "lint:prettier": "prettier --ignore-path .gitignore --check \"tools/**/*.{ts,tsx,js,jsx}\"", "lint:selene": "npx lerna exec --parallel -- selene --no-summary --num-threads=1 --config=../../selene.toml src", @@ -55,4 +55,4 @@ "released" ] } -} \ No newline at end of file +} diff --git a/src/access/test/scripts/Server/ServerMain.server.lua b/src/access/test/scripts/Server/ServerMain.server.lua index f213acd7871..d8795c450af 100644 --- a/src/access/test/scripts/Server/ServerMain.server.lua +++ b/src/access/test/scripts/Server/ServerMain.server.lua @@ -8,8 +8,9 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/animations/test/scripts/Server/ServerMain.server.lua b/src/animations/test/scripts/Server/ServerMain.server.lua index fc30c0aded6..2698bc776c8 100644 --- a/src/animations/test/scripts/Server/ServerMain.server.lua +++ b/src/animations/test/scripts/Server/ServerMain.server.lua @@ -10,6 +10,7 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/binder/test/scripts/Server/ServerMain.server.lua b/src/binder/test/scripts/Server/ServerMain.server.lua index 0f6e645beb7..3058d67736b 100644 --- a/src/binder/test/scripts/Server/ServerMain.server.lua +++ b/src/binder/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/blend/test/scripts/Server/ServerMain.server.lua b/src/blend/test/scripts/Server/ServerMain.server.lua index 77142ee2b17..2ca29f12439 100644 --- a/src/blend/test/scripts/Server/ServerMain.server.lua +++ b/src/blend/test/scripts/Server/ServerMain.server.lua @@ -11,6 +11,7 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/brine/test/scripts/Server/ServerMain.server.lua b/src/brine/test/scripts/Server/ServerMain.server.lua index 00a6ef6504a..66b441deaf8 100644 --- a/src/brine/test/scripts/Server/ServerMain.server.lua +++ b/src/brine/test/scripts/Server/ServerMain.server.lua @@ -8,8 +8,9 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/brio/test/scripts/Server/ServerMain.server.lua b/src/brio/test/scripts/Server/ServerMain.server.lua index a1f12f16de5..13af6f2db06 100644 --- a/src/brio/test/scripts/Server/ServerMain.server.lua +++ b/src/brio/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/camera/test/scripts/Server/ServerMain.server.lua b/src/camera/test/scripts/Server/ServerMain.server.lua index 77afe747faf..896957f6c8c 100644 --- a/src/camera/test/scripts/Server/ServerMain.server.lua +++ b/src/camera/test/scripts/Server/ServerMain.server.lua @@ -11,6 +11,7 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/characterutils/test/scripts/Server/ServerMain.server.lua b/src/characterutils/test/scripts/Server/ServerMain.server.lua index 687c98c3468..529011ca219 100644 --- a/src/characterutils/test/scripts/Server/ServerMain.server.lua +++ b/src/characterutils/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/chatproviderservice/test/scripts/Server/ServerMain.server.lua b/src/chatproviderservice/test/scripts/Server/ServerMain.server.lua index 32b4a9b02d4..8e8a6236c28 100644 --- a/src/chatproviderservice/test/scripts/Server/ServerMain.server.lua +++ b/src/chatproviderservice/test/scripts/Server/ServerMain.server.lua @@ -10,8 +10,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/clienttranslator/test/scripts/Server/ServerMain.server.lua b/src/clienttranslator/test/scripts/Server/ServerMain.server.lua index 95c7a8d4f9e..1ff06254022 100644 --- a/src/clienttranslator/test/scripts/Server/ServerMain.server.lua +++ b/src/clienttranslator/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/clipcharacters/test/scripts/Server/ServerMain.server.lua b/src/clipcharacters/test/scripts/Server/ServerMain.server.lua index 90f3855eb41..464c6b35ebc 100644 --- a/src/clipcharacters/test/scripts/Server/ServerMain.server.lua +++ b/src/clipcharacters/test/scripts/Server/ServerMain.server.lua @@ -10,8 +10,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/cmdrservice/test/scripts/Server/ServerMain.server.lua b/src/cmdrservice/test/scripts/Server/ServerMain.server.lua index 2c55543dfce..956dbebfca8 100644 --- a/src/cmdrservice/test/scripts/Server/ServerMain.server.lua +++ b/src/cmdrservice/test/scripts/Server/ServerMain.server.lua @@ -11,8 +11,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/conditions/test/scripts/Server/ServerMain.server.lua b/src/conditions/test/scripts/Server/ServerMain.server.lua index 6e0e361e32f..db034967b48 100644 --- a/src/conditions/test/scripts/Server/ServerMain.server.lua +++ b/src/conditions/test/scripts/Server/ServerMain.server.lua @@ -10,8 +10,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local AdorneeConditionUtils = require("AdorneeConditionUtils") diff --git a/src/coreguienabler/test/scripts/Server/ServerMain.server.lua b/src/coreguienabler/test/scripts/Server/ServerMain.server.lua index fcc85386fff..83bc7e951ef 100644 --- a/src/coreguienabler/test/scripts/Server/ServerMain.server.lua +++ b/src/coreguienabler/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/datastore/test/scripts/Server/ServerMain.server.lua b/src/datastore/test/scripts/Server/ServerMain.server.lua index a9c37c94c2a..713609724d3 100644 --- a/src/datastore/test/scripts/Server/ServerMain.server.lua +++ b/src/datastore/test/scripts/Server/ServerMain.server.lua @@ -12,8 +12,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local Maid = require("Maid") diff --git a/src/deathreport/test/scripts/Server/ServerMain.server.lua b/src/deathreport/test/scripts/Server/ServerMain.server.lua index d676a161df0..28a5b9b5b72 100644 --- a/src/deathreport/test/scripts/Server/ServerMain.server.lua +++ b/src/deathreport/test/scripts/Server/ServerMain.server.lua @@ -11,8 +11,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/ellipticcurvecryptography/test/scripts/Server/ServerMain.server.lua b/src/ellipticcurvecryptography/test/scripts/Server/ServerMain.server.lua index 3f83e22044a..0a98b2e587c 100644 --- a/src/ellipticcurvecryptography/test/scripts/Server/ServerMain.server.lua +++ b/src/ellipticcurvecryptography/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/elo/test/scripts/Server/ServerMain.server.lua b/src/elo/test/scripts/Server/ServerMain.server.lua index bd4bf9bc4db..61946aee0aa 100644 --- a/src/elo/test/scripts/Server/ServerMain.server.lua +++ b/src/elo/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/experiencecalculator/test/scripts/Server/ServerMain.server.lua b/src/experiencecalculator/test/scripts/Server/ServerMain.server.lua index 5c3102d9b77..162858ad744 100644 --- a/src/experiencecalculator/test/scripts/Server/ServerMain.server.lua +++ b/src/experiencecalculator/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/fakeskybox/test/scripts/Server/ServerMain.server.lua b/src/fakeskybox/test/scripts/Server/ServerMain.server.lua index 83f8a1cc2b2..b6efe1f469a 100644 --- a/src/fakeskybox/test/scripts/Server/ServerMain.server.lua +++ b/src/fakeskybox/test/scripts/Server/ServerMain.server.lua @@ -8,8 +8,9 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/friendutils/test/scripts/Server/ServerMain.server.lua b/src/friendutils/test/scripts/Server/ServerMain.server.lua index 4a8e10a83d1..5c0289880c3 100644 --- a/src/friendutils/test/scripts/Server/ServerMain.server.lua +++ b/src/friendutils/test/scripts/Server/ServerMain.server.lua @@ -7,6 +7,7 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/fzy/test/scripts/Server/ServerMain.server.lua b/src/fzy/test/scripts/Server/ServerMain.server.lua index 2e567cef21b..56096e4c8a3 100644 --- a/src/fzy/test/scripts/Server/ServerMain.server.lua +++ b/src/fzy/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/gameconfig/test/scripts/Server/ServerMain.server.lua b/src/gameconfig/test/scripts/Server/ServerMain.server.lua index 8f10bb3a0a2..81386268eb6 100644 --- a/src/gameconfig/test/scripts/Server/ServerMain.server.lua +++ b/src/gameconfig/test/scripts/Server/ServerMain.server.lua @@ -11,8 +11,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/gameproductservice/test/scripts/Server/ServerMain.server.lua b/src/gameproductservice/test/scripts/Server/ServerMain.server.lua index bc1722fe3ae..1989225e2d4 100644 --- a/src/gameproductservice/test/scripts/Server/ServerMain.server.lua +++ b/src/gameproductservice/test/scripts/Server/ServerMain.server.lua @@ -11,8 +11,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/gameversionutils/test/scripts/Server/ServerMain.server.lua b/src/gameversionutils/test/scripts/Server/ServerMain.server.lua index b4ec36fa904..44688bc009a 100644 --- a/src/gameversionutils/test/scripts/Server/ServerMain.server.lua +++ b/src/gameversionutils/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/genericscreenguiprovider/test/scripts/Server/ServerMain.server.lua b/src/genericscreenguiprovider/test/scripts/Server/ServerMain.server.lua index 888ae5a1966..77f22f5a23d 100644 --- a/src/genericscreenguiprovider/test/scripts/Server/ServerMain.server.lua +++ b/src/genericscreenguiprovider/test/scripts/Server/ServerMain.server.lua @@ -11,8 +11,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/grouputils/test/scripts/Server/ServerMain.server.lua b/src/grouputils/test/scripts/Server/ServerMain.server.lua index bb7612f6a0f..595cc171137 100644 --- a/src/grouputils/test/scripts/Server/ServerMain.server.lua +++ b/src/grouputils/test/scripts/Server/ServerMain.server.lua @@ -7,6 +7,7 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/humanoidtracker/test/scripts/Server/ServerMain.server.lua b/src/humanoidtracker/test/scripts/Server/ServerMain.server.lua index 7a467b17ffa..699607a7657 100644 --- a/src/humanoidtracker/test/scripts/Server/ServerMain.server.lua +++ b/src/humanoidtracker/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/ik/test/scripts/Server/ServerMain.server.lua b/src/ik/test/scripts/Server/ServerMain.server.lua index 327590ed201..9a9cbbb12f7 100644 --- a/src/ik/test/scripts/Server/ServerMain.server.lua +++ b/src/ik/test/scripts/Server/ServerMain.server.lua @@ -12,8 +12,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/influxdbclient/test/scripts/Server/ServerMain.server.lua b/src/influxdbclient/test/scripts/Server/ServerMain.server.lua index 008f28019f0..7f664e0c0ba 100644 --- a/src/influxdbclient/test/scripts/Server/ServerMain.server.lua +++ b/src/influxdbclient/test/scripts/Server/ServerMain.server.lua @@ -11,8 +11,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end require("InfluxDBClient") diff --git a/src/inputkeymaputils/test/scripts/Server/ServerMain.server.lua b/src/inputkeymaputils/test/scripts/Server/ServerMain.server.lua index 94c6338b93c..df440ca2cd8 100644 --- a/src/inputkeymaputils/test/scripts/Server/ServerMain.server.lua +++ b/src/inputkeymaputils/test/scripts/Server/ServerMain.server.lua @@ -10,8 +10,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/instanceutils/test/scripts/Server/ServerMain.server.lua b/src/instanceutils/test/scripts/Server/ServerMain.server.lua index 1adead010df..3adec347446 100644 --- a/src/instanceutils/test/scripts/Server/ServerMain.server.lua +++ b/src/instanceutils/test/scripts/Server/ServerMain.server.lua @@ -10,6 +10,7 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/lipsum/test/scripts/Server/ServerMain.server.lua b/src/lipsum/test/scripts/Server/ServerMain.server.lua index d8038ccd8e9..17bd54dc582 100644 --- a/src/lipsum/test/scripts/Server/ServerMain.server.lua +++ b/src/lipsum/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/maid/test/scripts/Server/ServerMain.server.lua b/src/maid/test/scripts/Server/ServerMain.server.lua index dfe7e86c2da..dbd0e03e5db 100644 --- a/src/maid/test/scripts/Server/ServerMain.server.lua +++ b/src/maid/test/scripts/Server/ServerMain.server.lua @@ -7,8 +7,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local Maid = require("Maid") diff --git a/src/marketplaceutils/test/scripts/Server/ServerMain.server.lua b/src/marketplaceutils/test/scripts/Server/ServerMain.server.lua index b83f5dbde5e..5cb6d8db415 100644 --- a/src/marketplaceutils/test/scripts/Server/ServerMain.server.lua +++ b/src/marketplaceutils/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/nevermore-cli-manifest/test/scripts/Server/ServerMain.server.lua b/src/nevermore-cli-manifest/test/scripts/Server/ServerMain.server.lua index 43618fc8a35..b186cfd6aa1 100644 --- a/src/nevermore-cli-manifest/test/scripts/Server/ServerMain.server.lua +++ b/src/nevermore-cli-manifest/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/nevermore-test-runner/package.json b/src/nevermore-test-runner/package.json index c8b187c93f0..7ddb2158277 100644 --- a/src/nevermore-test-runner/package.json +++ b/src/nevermore-test-runner/package.json @@ -22,7 +22,8 @@ }, "license": "MIT", "scripts": { - "preinstall": "npx only-allow pnpm" + "preinstall": "npx only-allow pnpm", + "test": "lune run test/results.test.luau" }, "contributors": [ "Quenty" diff --git a/src/nevermore-test-runner/src/Server/NevermoreTestResults.lua b/src/nevermore-test-runner/src/Server/NevermoreTestResults.lua new file mode 100644 index 00000000000..99c96aecd4d --- /dev/null +++ b/src/nevermore-test-runner/src/Server/NevermoreTestResults.lua @@ -0,0 +1,358 @@ +--!strict +--[=[ + @class NevermoreTestResults + + Turns what Jest resolved with into the results table a test run returns. + + Deliberately dependency-free: no loader, no Jest, no `print` or `warn`. This + logic has produced three bugs — counts read off the wrong table, jest-lua's + inverted `success` field, and double-counted failed suites — every one of them + shipped green because a module-scope Jest require made it impossible to unit + test. It takes plain tables and returns plain tables so a test can drive it + anywhere, including under Lune (see `test/results.test.luau`). + + The caller does the logging. +]=] + +--[=[ + Tag identifying a [TestRunResults] table to whatever reads a run's return + values. A test place also runs probe scripts that return whatever they like, + so a reader matches on this rather than on the table's shape. + + @prop FORMAT string + @within NevermoreTestResults +]=] +local FORMAT = "nevermore-test-results@1" + +-- Results travel back as a value, and an oversize value fails the whole run +-- outright instead of arriving truncated, so the failure list is capped. The +-- full text of every failure is still in the logs. +local MAX_FAILURES = 25 +local MAX_NAME_LENGTH = 300 +local MAX_MESSAGE_LENGTH = 500 + +-- Every count the verdict reads or reports. All are initialised to 0 by +-- jest-lua's makeEmptyAggregatedTestResult and only ever incremented, so a real +-- AggregatedResult carries all of them as numbers. Requiring the whole set is +-- what makes a rename detectable: the first version of this required only +-- numTotalTests, so a rename of any other field would have zeroed the counts and +-- reported a pass, which is the bug it was written to prevent. +local REQUIRED_COUNTS = { + "numTotalTests", + "numPassedTests", + "numFailedTests", + "numPendingTests", + "numTodoTests", + "numTotalTestSuites", + "numPassedTestSuites", + "numFailedTestSuites", + "numRuntimeErrorTestSuites", +} + +local NevermoreTestResults = {} + +NevermoreTestResults.FORMAT = FORMAT + +--[=[ + One failed test, or one suite that failed before its tests could run. + + @interface TestFailure + .name string -- Full test name, or the suite's script path for a suite-level failure + .message string? -- First failure message, truncated + @within NevermoreTestResults +]=] +export type TestFailure = { + name: string, + message: string?, +} + +--[=[ + What a test run produced. Only ever plain strings, numbers, booleans and + tables of those: the cloud and the Studio bridge marshal exotic values + differently, and this subset survives both unchanged. + + @interface TestRunResults + .format string -- Always [NevermoreTestResults.FORMAT] + .success boolean -- Whether the run is a pass + .ranJest boolean -- False for a smoke test, where the counts are all zero because nothing counted + .passed number + .failed number + .skipped number -- Pending plus todo + .total number + .suitesPassed number + .suitesFailed number + .suitesTotal number + .failures { TestFailure } -- Capped; `omittedFailures` says how many did not fit + .omittedFailures number + .error string? -- Why the run failed; absent on a pass + @within NevermoreTestResults +]=] +export type TestRunResults = { + format: string, + success: boolean, + ranJest: boolean, + passed: number, + failed: number, + skipped: number, + total: number, + suitesPassed: number, + suitesFailed: number, + suitesTotal: number, + failures: { TestFailure }, + omittedFailures: number, + error: string?, +} + +local function truncate(text: string, limit: number): string + if #text <= limit then + return text + end + + return string.sub(text, 1, limit) .. "..." +end + +--[[ + Finds the AggregatedResult in whatever Jest.runCLI resolved with. + + jest-lua resolves `{ globalConfig, results }`, so the counts live one level in. + Reading the wrapper finds none of them, and a missing count that defaults to + zero is indistinguishable from a clean run — which is how a wrong read of this + shipped and reported every failing suite as a pass. Both shapes are accepted, + and every count the verdict touches must be a number, so nothing can be read + off a table that only half matches. +]] +local function findAggregatedResult(resolved: any): any? + local function isAggregatedResult(candidate: any): boolean + if typeof(candidate) ~= "table" then + return false + end + for _, field in REQUIRED_COUNTS do + if type(candidate[field]) ~= "number" then + return false + end + end + return true + end + + if isAggregatedResult(resolved) then + return resolved + end + + if typeof(resolved) == "table" and isAggregatedResult(resolved.results) then + return resolved.results + end + + return nil +end + +local function collectFailures(result: any): ({ TestFailure }, number) + local failures: { TestFailure } = {} + local omitted = 0 + + local function add(name: string, message: string?) + if #failures >= MAX_FAILURES then + omitted += 1 + return + end + + table.insert(failures, { + name = truncate(name, MAX_NAME_LENGTH), + message = if message then truncate(message, MAX_MESSAGE_LENGTH) else nil, + }) + end + + local testResults = result.testResults + if type(testResults) ~= "table" then + return failures, omitted + end + + for _, suite in testResults do + if type(suite) ~= "table" then + continue + end + + local execError = suite.testExecError + if type(execError) == "table" then + add(tostring(suite.testFilePath or ""), tostring(execError.message or "suite failed to run")) + end + + local assertions = suite.testResults + if type(assertions) ~= "table" then + continue + end + + for _, assertion in assertions do + if type(assertion) ~= "table" or assertion.status ~= "failed" then + continue + end + + local message: string? = nil + local messages = assertion.failureMessages + if type(messages) == "table" and type(messages[1]) == "string" then + message = messages[1] + end + + add(tostring(assertion.fullName or assertion.title or ""), message) + end + end + + return failures, omitted +end + +--[=[ + Builds an all-zero passing result. Every other result is this with counts + filled in, so a caller never has to handle a missing field. + + @param ranJest boolean + @return TestRunResults +]=] +function NevermoreTestResults.new(ranJest: boolean): TestRunResults + return { + format = FORMAT, + success = true, + ranJest = ranJest, + passed = 0, + failed = 0, + skipped = 0, + total = 0, + suitesPassed = 0, + suitesFailed = 0, + suitesTotal = 0, + failures = {}, + omittedFailures = 0, + } +end + +--[=[ + Builds a failed result carrying no counts, for a run that never produced any. + + @param ranJest boolean + @param message string -- Why the run failed. Required: a failure with no reason is unreadable. + @return TestRunResults +]=] +function NevermoreTestResults.failed(ranJest: boolean, message: string): TestRunResults + local results = NevermoreTestResults.new(ranJest) + results.success = false + results.error = "[NevermoreTestRunner] " .. truncate(message, MAX_MESSAGE_LENGTH) + + return results +end + +--[=[ + Reads the verdict and the counts out of whatever `Jest.runCLI` resolved with. + + Fails closed on anything it cannot read. Defaulting an unreadable count to + zero instead is what let two wrong reads of this ship: no failures over no + tests reads exactly like a clean run. + + @param resolved any -- The value Jest.runCLI's promise resolved with + @return TestRunResults +]=] +function NevermoreTestResults.fromJest(resolved: any): TestRunResults + local result = findAggregatedResult(resolved) + if not result then + return NevermoreTestResults.failed( + true, + "Jest resolved without a readable AggregatedResult, so this run has no counts and cannot be called a pass" + ) + end + + local results = NevermoreTestResults.new(true) + results.passed = result.numPassedTests + results.failed = result.numFailedTests + results.skipped = result.numPendingTests + result.numTodoTests + results.total = result.numTotalTests + results.suitesPassed = result.numPassedTestSuites + -- Not added to numRuntimeErrorTestSuites. jest-lua increments both for one + -- suite that failed to run (helpers.lua addResult: the runtime-error counter + -- unconditionally, then numFailedTestSuites via the `numFailingTests > 0 or + -- testExecError` branch), so summing them reports one broken suite as two — + -- and "2 of 1 suite(s) failed" for a single-spec package. + results.suitesFailed = result.numFailedTestSuites + results.suitesTotal = result.numTotalTestSuites + + -- jest-lua derives numTotalTests as passing + failing + pending + todo per + -- suite (helpers.lua addResult), so this holds by construction for every real + -- result. It is asserted rather than assumed because it is the one check that + -- catches a field this code reads being renamed or moved: validating names + -- one by one only ever covers the names known when it was written, and both + -- bugs that shipped here were a count silently arriving as zero. + local counted = results.passed + results.failed + results.skipped + if counted ~= results.total then + return NevermoreTestResults.failed( + true, + string.format( + "Jest's counts do not add up (%d passed + %d failed + %d skipped = %d, but it reports %d total), " + .. "so they cannot be trusted to say whether this run passed", + results.passed, + results.failed, + results.skipped, + counted, + results.total + ) + ) + end + + results.failures, results.omittedFailures = collectFailures(result) + + -- A snapshot check can fail a run without failing a test, so the counts alone + -- do not cover it. + local snapshotFailed = typeof(result.snapshot) == "table" and result.snapshot.failure == true + -- A suite both skipped and broken lands in numPendingTestSuites, never in + -- numFailedTestSuites, so this is the only term that catches it. + local suitesErrored = result.numRuntimeErrorTestSuites + + -- `result.success` is deliberately not consulted. jest-lua inverted it: its + -- TestScheduler assigns `anyTestFailures or snapshot.failure or + -- anyReporterErrors` where upstream jest negates that whole expression + -- (TestScheduler.lua:434), so the field is true exactly when the run failed. + -- Reading it either way is a trap — the sense flips the day the missing `not` + -- is restored — so the underlying signals are read instead. The one signal + -- that leaves no other trace is a reporter error, which the AggregatedResult + -- does not expose at all. + -- + -- A suite that dies before its first test contributes no failed test, and an + -- interrupted run's counts describe only what it got through, so neither is + -- visible in the test counts alone either. + results.success = results.failed == 0 + and results.suitesFailed == 0 + and suitesErrored == 0 + and result.wasInterrupted ~= true + and not snapshotFailed + + if not results.success then + -- Built from the causes that actually hold, never formatted from counts + -- that may be zero. "0 test(s) and 0 test suite(s) failed" was a real + -- failure reason this used to emit, and a reason saying nothing failed is + -- unreadable as either a pass or a failure — which is what made the bug + -- behind it hard to see. + local causes = {} + if results.failed > 0 then + table.insert(causes, string.format("%d test(s) failed", results.failed)) + end + if results.suitesFailed > 0 then + table.insert(causes, string.format("%d test suite(s) failed", results.suitesFailed)) + end + if suitesErrored > 0 then + table.insert(causes, string.format("%d test suite(s) failed to run", suitesErrored)) + end + if result.wasInterrupted == true then + table.insert(causes, "the run was interrupted") + end + if snapshotFailed then + table.insert(causes, "a snapshot check failed") + end + if #causes == 0 then + -- Unreachable while every condition above is also a cause here. If it + -- is ever reached, the two lists have drifted apart, and saying so is + -- worth more than a reason built out of zeros. + table.insert(causes, "the run failed for a reason this runner could not identify") + end + + results.error = "[NevermoreTestRunner] " .. table.concat(causes, ", ") + end + + return results +end + +return NevermoreTestResults diff --git a/src/nevermore-test-runner/src/Server/NevermoreTestRunnerUtils.lua b/src/nevermore-test-runner/src/Server/NevermoreTestRunnerUtils.lua index 2e949be4a39..ed10f68140e 100644 --- a/src/nevermore-test-runner/src/Server/NevermoreTestRunnerUtils.lua +++ b/src/nevermore-test-runner/src/Server/NevermoreTestRunnerUtils.lua @@ -8,14 +8,42 @@ - If a jest.config is found under the given root, runs Jest tests - If no jest.config is found, boot success is the test (smoke test) - Detects Open Cloud execution via OpenCloudService to control behavior + + A run reports itself by *returning* [NevermoreTestResults.TestRunResults], not + by throwing. The engine truncates a long run's log output, so counts scraped + back out of that text are exactly what goes missing on the runs where they + matter most. A test script hands the table straight out of its top level: + + ```lua + local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) + if results then + return results + end + ``` + + Reading Jest's result into that table is [NevermoreTestResults]'s job, which + is a separate module so it can be unit tested without Jest present. ]=] local require = require(script.Parent.loader).load(script) +local NevermoreTestResults = require("NevermoreTestResults") + local Jest = (require :: any)("Jest") local NevermoreTestRunnerUtils = {} +--[=[ + Tag identifying a results table to whatever reads a run's return values. + + @prop RESULTS_FORMAT string + @within NevermoreTestRunnerUtils +]=] +NevermoreTestRunnerUtils.RESULTS_FORMAT = NevermoreTestResults.FORMAT + +export type TestFailure = NevermoreTestResults.TestFailure +export type TestRunResults = NevermoreTestResults.TestRunResults + --[=[ Returns true if running inside an Open Cloud Luau Execution context. ]=] @@ -39,50 +67,59 @@ end Runs Jest tests if a jest.config is found under root. Otherwise treats boot success as the test (smoke test). - In Open Cloud, errors propagate naturally and the session terminates. - Outside Open Cloud (e.g. run-in-roblox), we call ProcessService:ExitAsync() - so Studio exits with the correct code. + Returns nil when no test run was attempted, which is how a real game server + tells itself apart from a test place: script sources are unreadable there, so + the caller falls through to its normal boot instead. + + Outside Open Cloud (e.g. Studio via studio-bridge) nothing reads the returned + results, so ProcessService:ExitAsync() carries the verdict out instead. - @param root -- The instance to scan for jest.config (e.g. the package folder in ServerScriptService) + @param root Instance -- The instance to scan for jest.config (e.g. the package folder in ServerScriptService) + @return NevermoreTestResults.TestRunResults? ]=] -function NevermoreTestRunnerUtils.runTestsIfNeededAsync(root: Instance): boolean +function NevermoreTestRunnerUtils.runTestsIfNeededAsync(root: Instance): TestRunResults? assert(typeof(root) == "Instance", "Bad root") local isOpenCloud = NevermoreTestRunnerUtils.isOpenCloud() local canReadSource = NevermoreTestRunnerUtils.canReadScriptSource() if not canReadSource then - return false + return nil end if isOpenCloud then print("[NevermoreTestRunner] Running in Open Cloud execution context") - NevermoreTestRunnerUtils._runTestsAsync(root) - return true + return NevermoreTestRunnerUtils._runTestsAsync(root) + end + + print("[NevermoreTestRunner] Running in local execution context") + local ok, returned = pcall(function(): any + return NevermoreTestRunnerUtils._runTestsAsync(root) + end) + + local results: TestRunResults + if ok then + results = returned else - print("[NevermoreTestRunner] Running in local execution context") - local ok, err = pcall(function(): any - return NevermoreTestRunnerUtils._runTestsAsync(root) - end) - local processService = (game :: any):GetService("ProcessService") - if processService then - if ok then - (processService :: any):ExitAsync(0) - else - warn(tostring(err)); - (processService :: any):ExitAsync(1) - end - end + results = NevermoreTestResults.failed(false, tostring(returned)) + warn(results.error) + end - return true + local exitCode = if results.success then 0 else 1 + local processService = (game :: any):GetService("ProcessService") + if processService then + (processService :: any):ExitAsync(exitCode) end + + return results end -function NevermoreTestRunnerUtils._runTestsAsync(root: Instance) +function NevermoreTestRunnerUtils._runTestsAsync(root: Instance): TestRunResults local config = root:FindFirstChild("jest.config", true) if not config or not config.Parent then print("[NevermoreTestRunner] No jest.config found — smoke test passed (boot success)") - return + + return NevermoreTestResults.new(false) end local projectRoot = config.Parent @@ -102,12 +139,35 @@ function NevermoreTestRunnerUtils._runTestsAsync(root: Instance) elseif typeof(result) == "string" then message = result end - error("[NevermoreTestRunner] " .. message) + + local rejected = NevermoreTestResults.failed(true, message) + warn(rejected.error) + + return rejected end - if typeof(result) == "table" and result.numFailedTests and result.numFailedTests > 0 then - error(string.format("[NevermoreTestRunner] %d test(s) failed", result.numFailedTests)) + local results = NevermoreTestResults.fromJest(result) + + -- Printed as well as returned. It is the one line that proves the counts were + -- read off a result this runner understood, whatever happens to the return + -- channel on the way out. + print( + string.format( + "[NevermoreTestRunner] Results: %d passed, %d failed, %d skipped, %d total; %d of %d suite(s) failed", + results.passed, + results.failed, + results.skipped, + results.total, + results.suitesFailed, + results.suitesTotal + ) + ) + + if results.error then + warn(results.error) end + + return results end return NevermoreTestRunnerUtils diff --git a/src/nevermore-test-runner/test/results.test.luau b/src/nevermore-test-runner/test/results.test.luau new file mode 100644 index 00000000000..ef63b362f72 --- /dev/null +++ b/src/nevermore-test-runner/test/results.test.luau @@ -0,0 +1,241 @@ +--[[ + Unit tests for NevermoreTestResults, run under Lune: + + lune run src/nevermore-test-runner/test/results + + Self-contained on purpose — it needs no runner, no mocks and no Roblox, which + is the whole reason the module under test has no dependencies. Three bugs have + shipped from this logic (counts read off Jest's wrapper table, jest-lua's + inverted `success` field, and failed suites counted twice), all of them green + in CI, because it used to share a file with a module-scope Jest require and so + could not be driven from a test at all. + + Exits non-zero on the first failure. +]] + +local process = require("@lune/process") + +local NevermoreTestResults = require("../src/Server/NevermoreTestResults") + +local failures = 0 +local passes = 0 + +local function check(name: string, ok: boolean, detail: string?) + if ok then + passes += 1 + print(` ok {name}`) + else + failures += 1 + print(`FAIL {name}{if detail then ": " .. detail else ""}`) + end +end + +local function checkEqual(name: string, actual: any, expected: any) + check(name, actual == expected, `expected {tostring(expected)}, got {tostring(actual)}`) +end + +--[[ + A realistic AggregatedResult. jest-lua derives numTotalTests as + passing + failing + pending + todo, so the default is internally consistent + and every override has to keep it that way. +]] +local function aggregated(overrides: { [string]: any }?): { [string]: any } + local result: { [string]: any } = { + numTotalTests = 311, + numPassedTests = 311, + numFailedTests = 0, + numPendingTests = 0, + numTodoTests = 0, + numTotalTestSuites = 19, + numPassedTestSuites = 19, + numFailedTestSuites = 0, + numRuntimeErrorTestSuites = 0, + wasInterrupted = false, + snapshot = { failure = false }, + testResults = {}, + -- jest-lua sets this to the *failure* condition, not to success + -- (TestScheduler.lua:434). A passing run therefore carries `false`. + success = false, + } + if overrides then + for key, value in overrides do + result[key] = value + end + end + return result +end + +--[[ Jest.runCLI resolves with { globalConfig, results }, not the result itself. ]] +local function resolved(result: { [string]: any }): { [string]: any } + return { globalConfig = {}, results = result } +end + +print("NevermoreTestResults.fromJest") + +do + local results = NevermoreTestResults.fromJest(resolved(aggregated())) + + checkEqual("passes a run where nothing failed", results.success, true) + checkEqual("does not read jest-lua's inverted success field", results.error, nil) + checkEqual("reads passed off the inner result", results.passed, 311) + checkEqual("reads total off the inner result", results.total, 311) + checkEqual("reads suite totals off the inner result", results.suitesTotal, 19) + checkEqual("marks the run as having run jest", results.ranJest, true) + checkEqual("tags the results", results.format, NevermoreTestResults.FORMAT) +end + +do + -- The shape upstream jest would resolve with, in case the wrapper ever goes. + local results = NevermoreTestResults.fromJest(aggregated()) + + checkEqual("accepts a bare AggregatedResult too", results.success, true) + checkEqual("reads counts off a bare AggregatedResult", results.passed, 311) +end + +do + local results = NevermoreTestResults.fromJest(resolved(aggregated({ + numTotalTests = 311, + numPassedTests = 309, + numFailedTests = 2, + numFailedTestSuites = 1, + numPassedTestSuites = 18, + }))) + + checkEqual("fails a run with failed tests", results.success, false) + checkEqual("reports the failed count", results.failed, 2) + checkEqual("names both causes", results.error, "[NevermoreTestRunner] 2 test(s) failed, 1 test suite(s) failed") +end + +do + -- One suite that failed to run. jest-lua increments numRuntimeErrorTestSuites + -- AND numFailedTestSuites for it, so adding them reports one suite as two. + local results = NevermoreTestResults.fromJest(resolved(aggregated({ + numTotalTests = 0, + numPassedTests = 0, + numTotalTestSuites = 1, + numPassedTestSuites = 0, + numFailedTestSuites = 1, + numRuntimeErrorTestSuites = 1, + }))) + + checkEqual("fails a run whose suite could not run", results.success, false) + checkEqual("does not double-count a broken suite", results.suitesFailed, 1) + check( + "never reports more failed suites than exist", + results.suitesFailed <= results.suitesTotal, + `{results.suitesFailed} of {results.suitesTotal}` + ) +end + +do + -- A suite both skipped and broken lands in numPendingTestSuites, so + -- numFailedTestSuites stays 0 and only the runtime-error count sees it. + local results = NevermoreTestResults.fromJest(resolved(aggregated({ + numTotalTests = 0, + numPassedTests = 0, + numTotalTestSuites = 1, + numPassedTestSuites = 0, + numRuntimeErrorTestSuites = 1, + }))) + + checkEqual("fails a run on the runtime-error count alone", results.success, false) + checkEqual("says the suite could not run", results.error, "[NevermoreTestRunner] 1 test suite(s) failed to run") +end + +do + local interrupted = NevermoreTestResults.fromJest(resolved(aggregated({ wasInterrupted = true }))) + checkEqual("fails an interrupted run", interrupted.success, false) + checkEqual("says it was interrupted", interrupted.error, "[NevermoreTestRunner] the run was interrupted") + + local snapshot = NevermoreTestResults.fromJest(resolved(aggregated({ snapshot = { failure = true } }))) + checkEqual("fails a run whose snapshot check failed", snapshot.success, false) + checkEqual("says the snapshot check failed", snapshot.error, "[NevermoreTestRunner] a snapshot check failed") +end + +do + -- Every failure reason must name something. "0 test(s) and 0 test suite(s) + -- failed" was a real reason this used to emit. + for _, result in + { + aggregated({ wasInterrupted = true }), + aggregated({ snapshot = { failure = true } }), + aggregated({ numRuntimeErrorTestSuites = 1 }), + } + do + local results = NevermoreTestResults.fromJest(resolved(result)) + check( + "never builds a reason out of zeros", + results.error ~= nil and not string.find(results.error :: string, "0 test", 1, true), + tostring(results.error) + ) + end +end + +print("NevermoreTestResults.fromJest — failing closed") + +do + -- Anything but a readable AggregatedResult. Every one of these used to be + -- read as zero failures over zero tests, which is a pass. + -- + -- A list of pairs, not a map: `["nil"] = nil` stores no entry at all, so a map + -- would drop the nil case silently — a test that never runs. + local missingFailedTests = aggregated() + missingFailedTests.numFailedTests = nil + + local renamedFailedTests = aggregated() + renamedFailedTests.numFailedTests = nil + renamedFailedTests.numFailingTests = 0 + + local unreadable: { { description: string, value: any } } = { + { description = "the wrapper alone", value = { globalConfig = {} } }, + { description = "nil", value = nil }, + { description = "a string", value = "done" }, + { description = "a table with no counts", value = { success = true } }, + { description = "a result missing numFailedTests", value = missingFailedTests }, + { description = "a result whose numFailedTests was renamed", value = renamedFailedTests }, + } + + -- #unreadable would stop at the nil-valued entry, so the count is explicit. + checkEqual("covers every unreadable shape", #unreadable, 6) + + for index = 1, 6 do + local case = unreadable[index] + local results = NevermoreTestResults.fromJest(case.value) + check(`fails closed on {case.description}`, results.success == false, "reported a pass") + check(`explains itself on {case.description}`, results.error ~= nil, "no reason given") + end +end + +do + -- The invariant that catches a rename of any count, including ones this code + -- does not know the name of yet. + local results = NevermoreTestResults.fromJest( + resolved(aggregated({ numTotalTests = 311, numPassedTests = 0, numFailedTests = 0 })) + ) + + checkEqual("fails closed when the counts do not add up", results.success, false) + check( + "says the counts do not add up", + results.error ~= nil and string.find(results.error :: string, "do not add up", 1, true) ~= nil, + tostring(results.error) + ) +end + +print("NevermoreTestResults.new / .failed") + +do + local smoke = NevermoreTestResults.new(false) + checkEqual("a smoke test passes", smoke.success, true) + checkEqual("a smoke test did not run jest", smoke.ranJest, false) + checkEqual("a passing result carries no error", smoke.error, nil) + + local failed = NevermoreTestResults.failed(true, "something broke") + checkEqual("a failed result does not pass", failed.success, false) + checkEqual("a failed result carries its reason", failed.error, "[NevermoreTestRunner] something broke") +end + +print(`\n{passes} passed, {failures} failed`) + +if failures > 0 then + process.exit(1) +end diff --git a/src/observablecollection/test/scripts/Server/ServerMain.server.lua b/src/observablecollection/test/scripts/Server/ServerMain.server.lua index 5dd0159edd1..68e7a2dbb55 100644 --- a/src/observablecollection/test/scripts/Server/ServerMain.server.lua +++ b/src/observablecollection/test/scripts/Server/ServerMain.server.lua @@ -11,8 +11,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local ObservableSortedList = require("ObservableSortedList") diff --git a/src/octree/test/scripts/Server/ServerMain.server.lua b/src/octree/test/scripts/Server/ServerMain.server.lua index 46888db3384..7767587a4ed 100644 --- a/src/octree/test/scripts/Server/ServerMain.server.lua +++ b/src/octree/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/pagesutils/test/scripts/Server/ServerMain.server.lua b/src/pagesutils/test/scripts/Server/ServerMain.server.lua index 7e109de67ec..cdf33d0a046 100644 --- a/src/pagesutils/test/scripts/Server/ServerMain.server.lua +++ b/src/pagesutils/test/scripts/Server/ServerMain.server.lua @@ -7,6 +7,7 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/permissionprovider/test/scripts/Server/ServerMain.server.lua b/src/permissionprovider/test/scripts/Server/ServerMain.server.lua index f7c2bedaa12..f0abf407c91 100644 --- a/src/permissionprovider/test/scripts/Server/ServerMain.server.lua +++ b/src/permissionprovider/test/scripts/Server/ServerMain.server.lua @@ -10,8 +10,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/player-mock/test/scripts/Server/ServerMain.server.lua b/src/player-mock/test/scripts/Server/ServerMain.server.lua index 75fed2b0331..c9ead16c31c 100644 --- a/src/player-mock/test/scripts/Server/ServerMain.server.lua +++ b/src/player-mock/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/playerbinder/test/scripts/Server/ServerMain.server.lua b/src/playerbinder/test/scripts/Server/ServerMain.server.lua index d6a9e8b2d60..5e3959336e3 100644 --- a/src/playerbinder/test/scripts/Server/ServerMain.server.lua +++ b/src/playerbinder/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/playerhumanoidbinder/test/scripts/Server/ServerMain.server.lua b/src/playerhumanoidbinder/test/scripts/Server/ServerMain.server.lua index d8723db24ec..3d4c7ffc20b 100644 --- a/src/playerhumanoidbinder/test/scripts/Server/ServerMain.server.lua +++ b/src/playerhumanoidbinder/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/playersservicepromises/test/scripts/Server/ServerMain.server.lua b/src/playersservicepromises/test/scripts/Server/ServerMain.server.lua index 459072fdabb..5b5598c3d03 100644 --- a/src/playersservicepromises/test/scripts/Server/ServerMain.server.lua +++ b/src/playersservicepromises/test/scripts/Server/ServerMain.server.lua @@ -7,6 +7,7 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/playerthumbnailutils/test/scripts/Server/ServerMain.server.lua b/src/playerthumbnailutils/test/scripts/Server/ServerMain.server.lua index f209e718062..9ef0076387f 100644 --- a/src/playerthumbnailutils/test/scripts/Server/ServerMain.server.lua +++ b/src/playerthumbnailutils/test/scripts/Server/ServerMain.server.lua @@ -7,6 +7,7 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/playerutils/test/scripts/Server/ServerMain.server.lua b/src/playerutils/test/scripts/Server/ServerMain.server.lua index 61b843901cc..e61f4b28763 100644 --- a/src/playerutils/test/scripts/Server/ServerMain.server.lua +++ b/src/playerutils/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/promise/test/scripts/Server/ServerMain.server.lua b/src/promise/test/scripts/Server/ServerMain.server.lua index 5d6b937777d..8eb6128b72f 100644 --- a/src/promise/test/scripts/Server/ServerMain.server.lua +++ b/src/promise/test/scripts/Server/ServerMain.server.lua @@ -11,4 +11,4 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +return NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) diff --git a/src/promisemaid/test/scripts/Server/ServerMain.server.lua b/src/promisemaid/test/scripts/Server/ServerMain.server.lua index 1cb1ac5a92f..97aa68b8663 100644 --- a/src/promisemaid/test/scripts/Server/ServerMain.server.lua +++ b/src/promisemaid/test/scripts/Server/ServerMain.server.lua @@ -11,4 +11,4 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +return NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) diff --git a/src/ragdoll/test/scripts/Server/ServerMain.server.lua b/src/ragdoll/test/scripts/Server/ServerMain.server.lua index fef5ced0360..da116cd649e 100644 --- a/src/ragdoll/test/scripts/Server/ServerMain.server.lua +++ b/src/ragdoll/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/randomutils/test/scripts/Server/ServerMain.server.lua b/src/randomutils/test/scripts/Server/ServerMain.server.lua index 0669b51b22f..de473a0160f 100644 --- a/src/randomutils/test/scripts/Server/ServerMain.server.lua +++ b/src/randomutils/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/receiptprocessing/test/scripts/Server/ServerMain.server.lua b/src/receiptprocessing/test/scripts/Server/ServerMain.server.lua index d2cfb559120..fcce8a51d5a 100644 --- a/src/receiptprocessing/test/scripts/Server/ServerMain.server.lua +++ b/src/receiptprocessing/test/scripts/Server/ServerMain.server.lua @@ -11,8 +11,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/remoting/test/scripts/Server/ServerMain.server.lua b/src/remoting/test/scripts/Server/ServerMain.server.lua index a4b0dcec25f..115dc595a6e 100644 --- a/src/remoting/test/scripts/Server/ServerMain.server.lua +++ b/src/remoting/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/resetservice/test/scripts/Server/ServerMain.server.lua b/src/resetservice/test/scripts/Server/ServerMain.server.lua index 3c92dde6e8e..6d5480d5c02 100644 --- a/src/resetservice/test/scripts/Server/ServerMain.server.lua +++ b/src/resetservice/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/roblox-api-dump/test/scripts/Server/ServerMain.server.lua b/src/roblox-api-dump/test/scripts/Server/ServerMain.server.lua index cff81985ffe..bbcc49c2811 100644 --- a/src/roblox-api-dump/test/scripts/Server/ServerMain.server.lua +++ b/src/roblox-api-dump/test/scripts/Server/ServerMain.server.lua @@ -9,8 +9,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/rogue-humanoid/test/scripts/Server/ServerMain.server.lua b/src/rogue-humanoid/test/scripts/Server/ServerMain.server.lua index 7af167750ef..b5fad0fcfd0 100644 --- a/src/rogue-humanoid/test/scripts/Server/ServerMain.server.lua +++ b/src/rogue-humanoid/test/scripts/Server/ServerMain.server.lua @@ -10,8 +10,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/rogue-properties/test/scripts/Server/ServerMain.server.lua b/src/rogue-properties/test/scripts/Server/ServerMain.server.lua index 91f58a8f0bd..6cf5d713455 100644 --- a/src/rogue-properties/test/scripts/Server/ServerMain.server.lua +++ b/src/rogue-properties/test/scripts/Server/ServerMain.server.lua @@ -10,8 +10,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/rx/test/scripts/Server/ServerMain.server.lua b/src/rx/test/scripts/Server/ServerMain.server.lua index ed294235cf6..21721a18406 100644 --- a/src/rx/test/scripts/Server/ServerMain.server.lua +++ b/src/rx/test/scripts/Server/ServerMain.server.lua @@ -10,6 +10,7 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/saveslot/test/scripts/Server/ServerMain.server.lua b/src/saveslot/test/scripts/Server/ServerMain.server.lua index 241b9daf62d..04b30b106aa 100644 --- a/src/saveslot/test/scripts/Server/ServerMain.server.lua +++ b/src/saveslot/test/scripts/Server/ServerMain.server.lua @@ -11,8 +11,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/scoredactionservice/test/scripts/Server/ServerMain.server.lua b/src/scoredactionservice/test/scripts/Server/ServerMain.server.lua index ffef087e321..8f170e5e0bd 100644 --- a/src/scoredactionservice/test/scripts/Server/ServerMain.server.lua +++ b/src/scoredactionservice/test/scripts/Server/ServerMain.server.lua @@ -7,8 +7,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local Maid = require("Maid") diff --git a/src/screenshothudservice/test/scripts/Server/ServerMain.server.lua b/src/screenshothudservice/test/scripts/Server/ServerMain.server.lua index a2471046965..372d1c71809 100644 --- a/src/screenshothudservice/test/scripts/Server/ServerMain.server.lua +++ b/src/screenshothudservice/test/scripts/Server/ServerMain.server.lua @@ -11,6 +11,7 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/secrets/test/scripts/Server/ServerMain.server.lua b/src/secrets/test/scripts/Server/ServerMain.server.lua index 276cbad989a..3ce8f268a87 100644 --- a/src/secrets/test/scripts/Server/ServerMain.server.lua +++ b/src/secrets/test/scripts/Server/ServerMain.server.lua @@ -11,8 +11,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/servicebag/test/scripts/Server/ServerMain.server.lua b/src/servicebag/test/scripts/Server/ServerMain.server.lua index c37d4b050a9..8264b6c24a4 100644 --- a/src/servicebag/test/scripts/Server/ServerMain.server.lua +++ b/src/servicebag/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/settings-inputkeymap/test/scripts/Server/ServerMain.server.lua b/src/settings-inputkeymap/test/scripts/Server/ServerMain.server.lua index 91facc28364..b10196b247b 100644 --- a/src/settings-inputkeymap/test/scripts/Server/ServerMain.server.lua +++ b/src/settings-inputkeymap/test/scripts/Server/ServerMain.server.lua @@ -10,8 +10,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/settings/test/scripts/Server/ServerMain.server.lua b/src/settings/test/scripts/Server/ServerMain.server.lua index 600792a3a79..7840d00f24f 100644 --- a/src/settings/test/scripts/Server/ServerMain.server.lua +++ b/src/settings/test/scripts/Server/ServerMain.server.lua @@ -10,8 +10,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/snackbar/test/scripts/Server/ServerMain.server.lua b/src/snackbar/test/scripts/Server/ServerMain.server.lua index c52336cfb63..4a77609cde4 100644 --- a/src/snackbar/test/scripts/Server/ServerMain.server.lua +++ b/src/snackbar/test/scripts/Server/ServerMain.server.lua @@ -10,8 +10,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/soundgroups/test/scripts/Server/ServerMain.server.lua b/src/soundgroups/test/scripts/Server/ServerMain.server.lua index da9fcc5c622..803ac560f1c 100644 --- a/src/soundgroups/test/scripts/Server/ServerMain.server.lua +++ b/src/soundgroups/test/scripts/Server/ServerMain.server.lua @@ -10,8 +10,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/steputils/test/scripts/Server/ServerMain.server.lua b/src/steputils/test/scripts/Server/ServerMain.server.lua index b6c7b64755f..4387d1e4126 100644 --- a/src/steputils/test/scripts/Server/ServerMain.server.lua +++ b/src/steputils/test/scripts/Server/ServerMain.server.lua @@ -11,4 +11,4 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +return NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) diff --git a/src/streamingutils/test/scripts/Server/ServerMain.server.lua b/src/streamingutils/test/scripts/Server/ServerMain.server.lua index 1cf33b50050..bce6485f37a 100644 --- a/src/streamingutils/test/scripts/Server/ServerMain.server.lua +++ b/src/streamingutils/test/scripts/Server/ServerMain.server.lua @@ -10,6 +10,7 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/string/test/scripts/Server/ServerMain.server.lua b/src/string/test/scripts/Server/ServerMain.server.lua index 90b715c01f4..95348c8017a 100644 --- a/src/string/test/scripts/Server/ServerMain.server.lua +++ b/src/string/test/scripts/Server/ServerMain.server.lua @@ -11,4 +11,4 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +return NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) diff --git a/src/teleportserviceutils/test/scripts/Server/ServerMain.server.lua b/src/teleportserviceutils/test/scripts/Server/ServerMain.server.lua index 869e1e281be..d3f2f31ad42 100644 --- a/src/teleportserviceutils/test/scripts/Server/ServerMain.server.lua +++ b/src/teleportserviceutils/test/scripts/Server/ServerMain.server.lua @@ -11,8 +11,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/src/throttle/test/scripts/Server/ServerMain.server.lua b/src/throttle/test/scripts/Server/ServerMain.server.lua index 576a85808a5..318b3f35991 100644 --- a/src/throttle/test/scripts/Server/ServerMain.server.lua +++ b/src/throttle/test/scripts/Server/ServerMain.server.lua @@ -7,6 +7,7 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/tie/test/scripts/Server/ServerMain.server.lua b/src/tie/test/scripts/Server/ServerMain.server.lua index 69f7460714f..09e3723e74f 100644 --- a/src/tie/test/scripts/Server/ServerMain.server.lua +++ b/src/tie/test/scripts/Server/ServerMain.server.lua @@ -10,8 +10,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local Action = require("Action") diff --git a/src/transitionmodel/test/scripts/Server/ServerMain.server.lua b/src/transitionmodel/test/scripts/Server/ServerMain.server.lua index f898a8409ba..1ae40cd2aba 100644 --- a/src/transitionmodel/test/scripts/Server/ServerMain.server.lua +++ b/src/transitionmodel/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/src/userserviceutils/test/scripts/Server/ServerMain.server.lua b/src/userserviceutils/test/scripts/Server/ServerMain.server.lua index 45cde651995..f4b99f57ba0 100644 --- a/src/userserviceutils/test/scripts/Server/ServerMain.server.lua +++ b/src/userserviceutils/test/scripts/Server/ServerMain.server.lua @@ -7,6 +7,7 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/tools/nevermore-cli/src/utils/job-context/batch-script-job-context.ts b/tools/nevermore-cli/src/utils/job-context/batch-script-job-context.ts index c4bc5a56303..f811fa71223 100644 --- a/tools/nevermore-cli/src/utils/job-context/batch-script-job-context.ts +++ b/tools/nevermore-cli/src/utils/job-context/batch-script-job-context.ts @@ -135,6 +135,13 @@ export class BatchScriptJobContext implements JobContext { success: result.success, durationMs: result.durationMs, errorMessage: result.error, + // returnValues stays absent: one execution covers every package, so + // nothing it returns belongs to any single one of them. The batch runner + // folds each package's own results into the batch summary, which the log + // parser has already split back out — so they are handed over decoded + // rather than left for the caller to find in a return value that is not + // this package's. + testResults: result.testResults, }; } diff --git a/tools/nevermore-cli/src/utils/job-context/cloud-job-context.test.ts b/tools/nevermore-cli/src/utils/job-context/cloud-job-context.test.ts new file mode 100644 index 00000000000..70a3e1d6635 --- /dev/null +++ b/tools/nevermore-cli/src/utils/job-context/cloud-job-context.test.ts @@ -0,0 +1,112 @@ +/** + * Unit tests for CloudJobContext.runScriptAsync — validates what a finished + * Open Cloud task reports back as a ScriptRunResult, in particular that a task + * which produced no output stays distinguishable from one that returned nothing. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { type Reporter } from '@quenty/cli-output-helpers/reporting'; +import { CloudJobContext } from './cloud-job-context.js'; +import { + type LuauTask, + type OpenCloudClient, +} from '../open-cloud/open-cloud-client.js'; +import { type Deployment } from './job-context.js'; + +function createReporter(): Reporter { + return { + onPackagePhaseChange: vi.fn(), + onPackageProgressUpdate: vi.fn(), + onPackageStart: vi.fn(), + onPackageResult: vi.fn(), + } as unknown as Reporter; +} + +/** + * A client whose task completes as `completedTask`. The real deployment handle + * is private to the context, so the test passes the three fields runScriptAsync + * reads off it. + */ +function createContext(completedTask: Partial) { + const task = { + path: 'universes/1/places/2/versions/3/luau-execution-session-tasks/4', + createTime: '2026-01-01T00:00:00Z', + updateTime: '2026-01-01T00:01:00Z', + user: 'users/1', + state: 'COMPLETE', + script: 'return 1', + ...completedTask, + } as LuauTask; + + const client = { + createExecutionTaskAsync: vi.fn(async () => task), + pollTaskCompletionAsync: vi.fn(async () => task), + } as unknown as OpenCloudClient; + + const context = new CloudJobContext(createReporter(), client); + const deployment = { + universeId: 1, + placeId: 2, + version: 3, + } as unknown as Deployment; + + return { context, deployment }; +} + +describe('CloudJobContext.runScriptAsync', () => { + afterEach(() => { + vi.useRealTimers(); + }); + + it('reports the values the script returned', async () => { + // Fake timers keep the client-side timeout race from leaving a live timer + // behind; nothing in the run itself waits on one. + vi.useFakeTimers(); + const { context, deployment } = createContext({ + state: 'COMPLETE', + output: { results: [{ slug: 'maid', counts: { passed: 1014 } }] }, + }); + + const result = await context.runScriptAsync(deployment, { + scriptContent: 'return results', + packageName: 'maid', + }); + + expect(result.success).toBe(true); + expect(result.returnValues).toEqual([ + { slug: 'maid', counts: { passed: 1014 } }, + ]); + }); + + it('reports an empty result when the task returned nothing', async () => { + vi.useFakeTimers(); + const { context, deployment } = createContext({ + state: 'COMPLETE', + output: {}, + }); + + const result = await context.runScriptAsync(deployment, { + scriptContent: 'print("hi")', + packageName: 'maid', + }); + + expect(result.returnValues).toEqual([]); + }); + + it('leaves returnValues absent when a failed task carried no output', async () => { + vi.useFakeTimers(); + const { context, deployment } = createContext({ + state: 'FAILED', + output: undefined, + }); + + const result = await context.runScriptAsync(deployment, { + scriptContent: 'return huge', + packageName: 'maid', + }); + + expect(result.success).toBe(false); + expect(result.taskState).toBe('FAILED'); + expect(result.returnValues).toBeUndefined(); + }); +}); diff --git a/tools/nevermore-cli/src/utils/job-context/cloud-job-context.ts b/tools/nevermore-cli/src/utils/job-context/cloud-job-context.ts index fe5455e60c2..2375bee5e5a 100644 --- a/tools/nevermore-cli/src/utils/job-context/cloud-job-context.ts +++ b/tools/nevermore-cli/src/utils/job-context/cloud-job-context.ts @@ -1,5 +1,6 @@ import { type Reporter } from '@quenty/cli-output-helpers/reporting'; import { + getTaskReturnValues, type LuauTask, type OpenCloudClient, } from '../open-cloud/open-cloud-client.js'; @@ -134,6 +135,7 @@ export class CloudJobContext extends BaseJobContext { success: completedTask.state === 'COMPLETE', taskState: completedTask.state, errorMessage, + returnValues: getTaskReturnValues(completedTask), }; } diff --git a/tools/nevermore-cli/src/utils/job-context/job-context.ts b/tools/nevermore-cli/src/utils/job-context/job-context.ts index c5b95969442..5b14913b413 100644 --- a/tools/nevermore-cli/src/utils/job-context/job-context.ts +++ b/tools/nevermore-cli/src/utils/job-context/job-context.ts @@ -1,4 +1,5 @@ import { type BuildPlaceOptions, type BuiltPlace } from '../build/build.js'; +import { type StructuredTestResults } from '../testing/structured-test-results.js'; export type { BuiltPlace } from '../build/build.js'; @@ -28,6 +29,38 @@ export interface ScriptRunResult { taskState?: string; /** Error message from the execution backend, if any. */ errorMessage?: string; + /** + * Everything the executed script returned, in order — the structured channel + * out of a run, as opposed to its printed output. Engine logs are truncated + * by Open Cloud on long runs, so anything a caller must read back exactly + * belongs here rather than in the log text. + * + * `undefined` means the transport never delivered a return channel: a cloud + * task that ended without an `output` (a FAILED task carries none, and an + * oversize return value fails the task rather than truncating the value), a + * bridge run that timed out or disconnected, or a context that does not + * carry return values at all. That is deliberately distinct from `[]`, which + * means the script ran and returned nothing — a caller that needs the value + * can fall back to parsing logs in the first case but not the second. + * + * Values are JSON-shaped, but the two transports spell exotic Luau types + * differently: Open Cloud auto-serializes them, while the Studio bridge + * marshals them into `{ type, value }` wrappers (`SerializedReturnValue`). + * Plain tables of strings, numbers and booleans come back identically on + * both, so structured results should stay inside that subset. + */ + returnValues?: unknown[]; + /** + * The run's structured test results, when the context resolved them itself + * rather than leaving them in `returnValues` for the caller to decode. + * + * Only aggregated batch mode sets this: one execution covers every package, so + * its return value belongs to no single one of them and the per-package + * results have to be recovered from the batch summary first. A context that + * sets this has also reported where the counts came from, so the caller does + * not report it a second time. + */ + testResults?: StructuredTestResults; } /** diff --git a/tools/nevermore-cli/src/utils/job-context/local-job-context.test.ts b/tools/nevermore-cli/src/utils/job-context/local-job-context.test.ts new file mode 100644 index 00000000000..2f5883f0ed5 --- /dev/null +++ b/tools/nevermore-cli/src/utils/job-context/local-job-context.test.ts @@ -0,0 +1,89 @@ +/** + * Unit tests for LocalJobContext.runScriptAsync — validates that a bridge run + * hands its script return values on as a ScriptRunResult, and that a run which + * never completed reports none rather than an empty result. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { type Reporter } from '@quenty/cli-output-helpers/reporting'; +import { LocalJobContext } from './local-job-context.js'; +import { type Deployment } from './job-context.js'; + +function createReporter(): Reporter { + return { + onPackagePhaseChange: vi.fn(), + onPackageProgressUpdate: vi.fn(), + onPackageStart: vi.fn(), + onPackageResult: vi.fn(), + } as unknown as Reporter; +} + +/** + * The deployment handle is private to the context, so the test stands in a + * bridge with the one method runScriptAsync calls on it. + */ +function createDeployment( + executeAsync: () => Promise<{ + success: boolean; + logs: string; + returnValues?: unknown[]; + }> +): Deployment { + return { + bridge: { executeAsync }, + cachedLogs: '', + } as unknown as Deployment; +} + +describe('LocalJobContext.runScriptAsync', () => { + it('reports the values the script returned', async () => { + const context = new LocalJobContext(createReporter()); + const deployment = createDeployment(async () => ({ + success: true, + logs: 'ran', + returnValues: [{ counts: { passed: 7 } }], + })); + + const result = await context.runScriptAsync(deployment, { + scriptContent: 'return results', + packageName: 'maid', + }); + + expect(result.success).toBe(true); + expect(result.returnValues).toEqual([{ counts: { passed: 7 } }]); + expect(await context.getLogsAsync(deployment)).toBe('ran'); + }); + + it('leaves returnValues absent when the bridge reported none', async () => { + const context = new LocalJobContext(createReporter()); + const deployment = createDeployment(async () => ({ + success: false, + logs: '[StudioBridge] Timed out after 200ms', + })); + + const result = await context.runScriptAsync(deployment, { + scriptContent: 'while true do end', + packageName: 'maid', + }); + + expect(result.returnValues).toBeUndefined(); + }); + + it('leaves returnValues absent when the bridge throws', async () => { + const context = new LocalJobContext(createReporter()); + const deployment = createDeployment(async () => { + throw new Error('no connected client'); + }); + + const result = await context.runScriptAsync(deployment, { + scriptContent: 'return results', + packageName: 'maid', + }); + + expect(result.success).toBe(false); + expect(result.returnValues).toBeUndefined(); + expect(await context.getLogsAsync(deployment)).toContain( + 'no connected client' + ); + }); +}); diff --git a/tools/nevermore-cli/src/utils/job-context/local-job-context.ts b/tools/nevermore-cli/src/utils/job-context/local-job-context.ts index 3c113823826..bbc268dad9b 100644 --- a/tools/nevermore-cli/src/utils/job-context/local-job-context.ts +++ b/tools/nevermore-cli/src/utils/job-context/local-job-context.ts @@ -69,7 +69,7 @@ export class LocalJobContext extends BaseJobContext { timeoutMs, }); localDeployment.cachedLogs = result.logs; - return { success: result.success }; + return { success: result.success, returnValues: result.returnValues }; } catch (error) { const errorMessage = error instanceof Error ? error.message : String(error); diff --git a/tools/nevermore-cli/src/utils/open-cloud/open-cloud-client.test.ts b/tools/nevermore-cli/src/utils/open-cloud/open-cloud-client.test.ts index e5148aa1a7c..ed1c5687aa3 100644 --- a/tools/nevermore-cli/src/utils/open-cloud/open-cloud-client.test.ts +++ b/tools/nevermore-cli/src/utils/open-cloud/open-cloud-client.test.ts @@ -1,6 +1,10 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import * as fs from 'fs/promises'; -import { OpenCloudClient } from './open-cloud-client.js'; +import { + OpenCloudClient, + getTaskReturnValues, + type LuauTask, +} from './open-cloud-client.js'; import type { RateLimiter } from './rate-limiter.js'; vi.mock('@quenty/cli-output-helpers', () => ({ @@ -367,3 +371,47 @@ describe('OpenCloudClient.resolveLatestPlaceVersionAsync', () => { ).rejects.toThrowError(/unparseable version path/); }); }); + +describe('getTaskReturnValues', () => { + function makeTask(overrides: Partial): LuauTask { + return { + path: 'universes/1/places/2/versions/3/luau-execution-session-tasks/4', + createTime: '2026-01-01T00:00:00Z', + updateTime: '2026-01-01T00:01:00Z', + user: 'users/1', + state: 'COMPLETE', + script: 'return 1', + ...overrides, + }; + } + + it('returns the values natively typed, one entry per returned value', () => { + // Roblox serializes the return value itself, so a returned table arrives as + // real nested JSON — there is no JSON string to parse a second time. + const task = makeTask({ + output: { + results: [{ slug: 'maid', counts: { passed: 1014 } }, 'str', 42, true], + }, + }); + + expect(getTaskReturnValues(task)).toEqual([ + { slug: 'maid', counts: { passed: 1014 } }, + 'str', + 42, + true, + ]); + }); + + it('reports an empty result when the task returned nothing', () => { + expect(getTaskReturnValues(makeTask({ output: {} }))).toEqual([]); + }); + + it('reports undefined when a failed task carried no output at all', () => { + // An oversize return value fails the task with no output and no error + // message, so "nothing came back to read" has to stay distinguishable from + // "the script returned nothing" — only the former can fall back to logs. + const task = makeTask({ state: 'FAILED', output: undefined }); + + expect(getTaskReturnValues(task)).toBeUndefined(); + }); +}); diff --git a/tools/nevermore-cli/src/utils/open-cloud/open-cloud-client.ts b/tools/nevermore-cli/src/utils/open-cloud/open-cloud-client.ts index 3deaf3169e1..f04b3ce7299 100644 --- a/tools/nevermore-cli/src/utils/open-cloud/open-cloud-client.ts +++ b/tools/nevermore-cli/src/utils/open-cloud/open-cloud-client.ts @@ -22,12 +22,38 @@ export interface LuauTask { | 'FAILED'; script: string; timeout?: string; - /** Script return value (populated on COMPLETE). */ - output?: { results?: Array<{ value?: string }> }; + /** + * What the script returned, populated on COMPLETE. `results` is a flat array + * of the returned values — `return t, "s", 42` arrives as three entries — + * natively typed: Roblox serializes them itself, so a returned Lua table is + * real nested JSON here, not a JSON string. (Which is also why a script must + * not JSONEncode its result: that lands as a double-encoded string.) + * + * Absent whenever the task produced no result. A FAILED task carries no + * `output` at all, with no error message explaining why — and an oversize + * return value (~4MB observed; ~2MB still arrives complete) fails the task + * exactly that way rather than truncating the value. + */ + output?: { results?: unknown[] }; /** Error details (populated on FAILED). */ error?: { code?: string; message?: string }; } +/** + * The values a finished task's script returned, or `undefined` when the task + * reported no result at all. + * + * The distinction matters: `undefined` means nothing came back to read, so a + * caller can fall back to the task's logs, while `[]` means the task did report + * a result and the script returned nothing — no fallback will find more. + */ +export function getTaskReturnValues(task: LuauTask): unknown[] | undefined { + if (!task.output) { + return undefined; + } + return task.output.results ?? []; +} + export interface OpenCloudClientOptions { apiKey: string | (() => Promise); rateLimiter: RateLimiter; diff --git a/tools/nevermore-cli/src/utils/testing/parsers/batch-log-parser.test.ts b/tools/nevermore-cli/src/utils/testing/parsers/batch-log-parser.test.ts index acbf1909099..eb1ca442c48 100644 --- a/tools/nevermore-cli/src/utils/testing/parsers/batch-log-parser.test.ts +++ b/tools/nevermore-cli/src/utils/testing/parsers/batch-log-parser.test.ts @@ -1,4 +1,5 @@ -import { describe, expect, it } from 'vitest'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { OutputHelper } from '@quenty/cli-output-helpers'; import { countTracebacks, @@ -6,6 +7,24 @@ import { parseBatchTestLogs, } from './batch-log-parser.js'; +/** Collect what the parser said out loud, so silence can be asserted on. */ +function captureWarnings(): () => string[] { + const warnings: string[] = []; + vi.spyOn(OutputHelper, 'warn').mockImplementation((message: string) => { + warnings.push(message); + }); + vi.spyOn(OutputHelper, 'info').mockImplementation(() => {}); + return () => warnings; +} + +afterEach(() => { + vi.restoreAllMocks(); +}); + +const RETURNED_COUNTS = + '"counts":{"passed":273,"failed":2,"skipped":1,"total":276,' + + '"suitesPassed":8,"suitesFailed":1,"suitesTotal":9},"ranJest":true'; + const SLUG_MAP = new Map([['egghunt2026', 'egghunt2026']]); function buildLogs(section: string): string { @@ -151,6 +170,324 @@ describe('parseBatchTestLogs', () => { expect(result?.testCounts?.failed).toBe(0); expect(result?.tracebackCount).toBe(1); }); + + it('prefers the counts the runner returned over the ones in the section', () => { + // The whole point of returning them: these are the numbers a truncated log + // window cannot take away. + const logs = [ + '===BATCH_TEST_BEGIN egghunt2026===', + '(the section this run printed is gone)', + '===BATCH_TEST_END egghunt2026 PASS 1000===', + '===BATCH_TEST_SUMMARY===', + '[{"slug":"egghunt2026","success":true,"durationMs":1000,"counts":' + + '{"passed":275,"failed":0,"skipped":2,"total":277,' + + '"suitesPassed":9,"suitesFailed":0,"suitesTotal":9}}]', + ].join('\n'); + + const result = parseBatchTestLogs(logs, SLUG_MAP).get('egghunt2026'); + + expect(result?.testCounts).toEqual({ passed: 275, failed: 0, total: 277 }); + }); + + it('ignores malformed counts instead of reporting zeroes', () => { + const logs = buildLogs('Tests: 275 passed, 275 total').replace( + '"durationMs":1000', + '"durationMs":1000,"counts":{"passed":"lots"}' + ); + + const result = parseBatchTestLogs(logs, SLUG_MAP).get('egghunt2026'); + + expect(result?.testCounts).toEqual({ passed: 275, failed: 0, total: 275 }); + }); + + it('reports why the runner failed a package rather than guessing', () => { + // A failing suite used to reach here as a Luau error. It arrives as a + // verdict now, and saying "Luau error" about it sends you hunting for one. + const logs = [ + '===BATCH_TEST_BEGIN egghunt2026===', + 'Tests: 2 failed, 273 passed, 275 total', + '===BATCH_TEST_END egghunt2026 FAIL 1000===', + '===BATCH_TEST_SUMMARY===', + '[{"slug":"egghunt2026","success":false,"durationMs":1000,' + + '"error":"[NevermoreTestRunner] 2 test(s) and 0 test suite(s) failed",' + + '"counts":{"passed":273,"failed":2,"skipped":0,"total":275,' + + '"suitesPassed":8,"suitesFailed":0,"suitesTotal":9}}]', + ].join('\n'); + + const result = parseBatchTestLogs(logs, SLUG_MAP).get('egghunt2026'); + + expect(result?.success).toBe(false); + expect(result?.error).toContain('[NevermoreTestRunner] 2 test(s)'); + expect(result?.error).not.toContain('Luau error'); + expect(result?.testCounts).toEqual({ passed: 273, failed: 2, total: 275 }); + }); + + it('does not believe a pass reported alongside failed tests', () => { + const logs = [ + '===BATCH_TEST_BEGIN egghunt2026===', + 'Tests: 275 passed, 275 total', + '===BATCH_TEST_END egghunt2026 PASS 1000===', + '===BATCH_TEST_SUMMARY===', + '[{"slug":"egghunt2026","success":true,"durationMs":1000,"counts":' + + '{"passed":273,"failed":2,"skipped":0,"total":275,' + + '"suitesPassed":8,"suitesFailed":1,"suitesTotal":9}}]', + ].join('\n'); + + const result = parseBatchTestLogs(logs, SLUG_MAP).get('egghunt2026'); + + expect(result?.success).toBe(false); + expect(result?.error).toContain('2 failed test(s)'); + }); + + it('still reads a summary from a package that returned no counts', () => { + // A test script written before results were returned. + const result = parseBatchTestLogs( + buildLogs('Tests: 275 passed, 275 total'), + SLUG_MAP + ).get('egghunt2026'); + + expect(result?.success).toBe(true); + expect(result?.testCounts).toEqual({ passed: 275, failed: 0, total: 275 }); + }); + + it('records where each package’s counts came from', () => { + // The structured channel and log scraping produce identical-looking output, + // so a channel that quietly stopped flowing is invisible without this. + const scraped = parseBatchTestLogs( + buildLogs('Tests: 275 passed, 275 total'), + SLUG_MAP + ).get('egghunt2026'); + expect(scraped?.countsSource).toBe('scraped'); + expect(scraped?.testResults).toBeUndefined(); + + const returned = parseBatchTestLogs( + buildLogs('Tests: 2 failed, 273 passed, 276 total').replace( + '"durationMs":1000', + `"durationMs":1000,${RETURNED_COUNTS}` + ), + SLUG_MAP + ).get('egghunt2026'); + expect(returned?.countsSource).toBe('returned'); + expect(returned?.testResults).toMatchObject({ + ranJest: true, + passed: 273, + failed: 2, + skipped: 1, + total: 276, + suitesFailed: 1, + }); + }); + + it('warns out loud when a package fell back to log scraping', () => { + const warnings = captureWarnings(); + + parseBatchTestLogs(buildLogs('Tests: 275 passed, 275 total'), SLUG_MAP); + + expect( + warnings().some( + (w) => + w.includes('returned no test results') && w.includes('egghunt2026') + ) + ).toBe(true); + }); + + it('says nothing about a fallback when every package returned its counts', () => { + const warnings = captureWarnings(); + + parseBatchTestLogs( + buildLogs('Tests: 2 failed, 273 passed, 276 total').replace( + '"durationMs":1000', + `"durationMs":1000,${RETURNED_COUNTS}` + ), + SLUG_MAP + ); + + expect(warnings().some((w) => w.includes('returned no test results'))).toBe( + false + ); + }); + + it('passes every package whose returned counts are clean', () => { + // The regression this guards: a runner consulted jest-lua's inverted + // AggregatedResult.success and failed all four packages in a batch, three of + // which had every test passing. + const warnings = captureWarnings(); + const fourPackages = new Map([ + ['access', 'access'], + ['animations', 'animations'], + ['binder', 'binder'], + ['blend', 'blend'], + ]); + + const clean = (slug: string, passed: number) => + [ + `===BATCH_TEST_BEGIN ${slug}===`, + `Tests: ${passed} passed, ${passed} total`, + `===BATCH_TEST_END ${slug} PASS 100===`, + ].join('\n'); + + const entry = (slug: string, passed: number) => + `{"slug":"${slug}","success":true,"durationMs":100,"ranJest":true,` + + `"counts":{"passed":${passed},"failed":0,"skipped":0,"total":${passed},` + + `"suitesPassed":1,"suitesFailed":0,"suitesTotal":1}}`; + + const logs = [ + clean('access', 311), + clean('animations', 8), + clean('binder', 99), + clean('blend', 3), + '===BATCH_TEST_SUMMARY===', + `[${entry('access', 311)},${entry('animations', 8)},` + + `${entry('binder', 99)},${entry('blend', 3)}]`, + ].join('\n'); + + const parsed = parseBatchTestLogs(logs, fourPackages); + + expect([...parsed.values()].filter((r) => r.success)).toHaveLength(4); + expect(parsed.get('access')?.testCounts).toEqual({ + passed: 311, + failed: 0, + total: 311, + }); + expect(warnings()).toHaveLength(0); + }); + + it('warns when a failure’s own counts show nothing failed', () => { + const warnings = captureWarnings(); + + const logs = [ + '===BATCH_TEST_BEGIN egghunt2026===', + 'Tests: 311 passed, 311 total', + '===BATCH_TEST_END egghunt2026 FAIL 100===', + '===BATCH_TEST_SUMMARY===', + '[{"slug":"egghunt2026","success":false,"durationMs":100,"ranJest":true,' + + '"error":"[NevermoreTestRunner] something","counts":{"passed":311,' + + '"failed":0,"skipped":0,"total":311,"suitesPassed":19,"suitesFailed":0,' + + '"suitesTotal":19}}]', + ].join('\n'); + + const result = parseBatchTestLogs(logs, SLUG_MAP).get('egghunt2026'); + + // The verdict stands — it is not the parser's to overturn — but it is loud. + expect(result?.success).toBe(false); + expect( + warnings().some((w) => w.includes('own counts show nothing failed')) + ).toBe(true); + }); + + it('accepts returned counts in place of a jest report the log lost', () => { + // The CI case this whole channel exists for: dozens of packages share one log + // window, so a section keeps its END marker long after its jest summary was + // dropped. Requiring the report there failed the package anyway. + const logs = [ + '===BATCH_TEST_BEGIN egghunt2026===', + ' ✓ one line of output that survived', + '===BATCH_TEST_END egghunt2026 PASS 1000===', + '===BATCH_TEST_SUMMARY===', + '[{"slug":"egghunt2026","success":true,"durationMs":1000,"ranJest":true,' + + '"counts":{"passed":311,"failed":0,"skipped":0,"total":311,' + + '"suitesPassed":19,"suitesFailed":0,"suitesTotal":19}}]', + ].join('\n'); + + const result = parseBatchTestLogs(logs, SLUG_MAP).get('egghunt2026'); + + expect(result?.success).toBe(true); + expect(result?.error).toBeUndefined(); + expect(result?.testCounts).toEqual({ passed: 311, failed: 0, total: 311 }); + }); + + it('still demands a jest report from a package that returned no counts', () => { + // Nothing structural to stand in for it, so the stand-in stays. + const logs = [ + '===BATCH_TEST_BEGIN egghunt2026===', + ' ✓ one line of output that survived', + '===BATCH_TEST_END egghunt2026 PASS 1000===', + '===BATCH_TEST_SUMMARY===', + '[{"slug":"egghunt2026","success":true,"durationMs":1000}]', + ].join('\n'); + + const result = parseBatchTestLogs(logs, SLUG_MAP).get('egghunt2026'); + + expect(result?.success).toBe(false); + expect(result?.error).toContain('nothing proves any test ran'); + }); + + it('judges a package whose section was lost entirely on its counts', () => { + const warnings = captureWarnings(); + const twoPackages = new Map([ + ['alpha', 'alpha'], + ['beta', 'beta'], + ]); + const logs = [ + '===BATCH_TEST_BEGIN alpha===', + 'Tests: 10 passed, 10 total', + '===BATCH_TEST_END alpha PASS 10===', + '===BATCH_TEST_SUMMARY===', + '[{"slug":"alpha","success":true,"durationMs":10,"ranJest":true,' + + '"counts":{"passed":10,"failed":0,"skipped":0,"total":10,"suitesPassed":1,' + + '"suitesFailed":0,"suitesTotal":1}},' + + '{"slug":"beta","success":true,"durationMs":20,"ranJest":true,' + + '"counts":{"passed":7,"failed":0,"skipped":0,"total":7,"suitesPassed":1,' + + '"suitesFailed":0,"suitesTotal":1}}]', + ].join('\n'); + + const results = parseBatchTestLogs(logs, twoPackages); + + // beta's section never arrived; its counts did. + expect(results.get('beta')?.success).toBe(true); + expect(results.get('beta')?.testCounts).toEqual({ + passed: 7, + failed: 0, + total: 7, + }); + // Narrower than a verdict with a section: nothing checked it for tracebacks. + expect( + warnings().some((w) => w.includes('not checked for Luau tracebacks')) + ).toBe(true); + }); + + it('still fails a lost section when no counts came back either', () => { + const twoPackages = new Map([ + ['alpha', 'alpha'], + ['beta', 'beta'], + ]); + const logs = [ + '===BATCH_TEST_BEGIN alpha===', + 'Tests: 10 passed, 10 total', + '===BATCH_TEST_END alpha PASS 10===', + '===BATCH_TEST_SUMMARY===', + '[{"slug":"alpha","success":true},{"slug":"beta","success":true}]', + ].join('\n'); + + const results = parseBatchTestLogs(logs, twoPackages); + + expect(results.get('beta')?.success).toBe(false); + expect(results.get('beta')?.error).toContain( + 'no output could be attributed' + ); + }); + + it('warns when the returned counts and the log disagree', () => { + // The exact failure that shipped once: the runner read the wrong level of + // jest's result, so every count came back zero and read as a clean run. + const warnings = captureWarnings(); + + const result = parseBatchTestLogs( + buildLogs('Tests: 275 passed, 275 total').replace( + '"durationMs":1000', + '"durationMs":1000,"counts":{"passed":0,"failed":0,"skipped":0,' + + '"total":0,"suitesPassed":0,"suitesFailed":0,"suitesTotal":0},"ranJest":true' + ), + SLUG_MAP + ); + + expect( + warnings().some((w) => + w.includes('returned counts disagree with the log') + ) + ).toBe(true); + expect(result.get('egghunt2026')?.countsSource).toBe('returned'); + }); }); describe('findSummaryEntries', () => { diff --git a/tools/nevermore-cli/src/utils/testing/parsers/batch-log-parser.ts b/tools/nevermore-cli/src/utils/testing/parsers/batch-log-parser.ts index b1d0b60f979..c4ce1530919 100644 --- a/tools/nevermore-cli/src/utils/testing/parsers/batch-log-parser.ts +++ b/tools/nevermore-cli/src/utils/testing/parsers/batch-log-parser.ts @@ -5,6 +5,7 @@ import { parseTestCounts, } from '../test-log-parser.js'; import { formatTracebacks, parseTracebacks } from './traceback-parser.js'; +import { type StructuredTestResults } from '../structured-test-results.js'; export interface BatchPackageResult { slug: string; @@ -21,17 +22,81 @@ export interface BatchPackageResult { * test — often after the test that scheduled it already passed. */ tracebackCount: number; + /** + * Where `testCounts` came from. `returned` means the run handed them over as a + * value; `scraped` means they were read out of log text, which is the channel + * truncation destroys. Absent when there are no counts at all. + * + * Recorded because the two are otherwise indistinguishable in the output, and + * a structured channel that quietly stopped flowing looks exactly like one + * that never existed. + */ + countsSource?: 'returned' | 'scraped'; + /** + * What this package's own run returned, recovered from the batch summary. + * + * One execution covers every package, so its return value belongs to none of + * them — the batch runner splits the per-package results into the summary + * instead, and this puts them back in the shape a single run's results have. + * Lossy by design: the summary carries counts, not failure text, which is + * per-package and already in that package's log section. + */ + testResults?: StructuredTestResults; } const BEGIN_MARKER = '===BATCH_TEST_BEGIN '; const END_MARKER = '===BATCH_TEST_END '; const SUMMARY_MARKER = '===BATCH_TEST_SUMMARY==='; +/** + * Counts the batch runner folds in from what a package's test script returned. + * + * Present only for a package whose script returns its results. Carried in the + * summary rather than left to the logs because the summary prints last and + * survives a truncated log window, which is where scraped counts are lost. + */ +interface SummaryCounts { + passed: number; + failed: number; + skipped: number; + total: number; + suitesPassed: number; + suitesFailed: number; + suitesTotal: number; +} + interface SummaryEntry { slug: string; success: boolean; durationMs?: number; error?: string; + counts?: SummaryCounts; + /** False for a smoke test, so zero counts are not read as "no tests found". */ + ranJest?: boolean; +} + +/** Read counts off a summary entry, ignoring anything malformed. */ +function readSummaryCounts(entry: SummaryEntry): SummaryCounts | undefined { + const counts = entry.counts; + if (typeof counts !== 'object' || counts === null) { + return undefined; + } + + const fields: (keyof SummaryCounts)[] = [ + 'passed', + 'failed', + 'skipped', + 'total', + 'suitesPassed', + 'suitesFailed', + 'suitesTotal', + ]; + for (const field of fields) { + if (!Number.isFinite(counts[field])) { + return undefined; + } + } + return counts; } /** @@ -156,6 +221,9 @@ export function parseBatchTestLogs( const summaryResults = new Map(); const summaryDurations = new Map(); + const summaryErrors = new Map(); + const summaryCounts = new Map(); + const summaryRanJest = new Map(); if (summaryLineIndex >= 0 && summaryLineIndex + 1 < lines.length) { const entries = findSummaryEntries(lines, summaryLineIndex + 1); if (entries === undefined) { @@ -172,16 +240,24 @@ export function parseBatchTestLogs( if (typeof entry.durationMs === 'number') { summaryDurations.set(entry.slug, entry.durationMs); } + if (typeof entry.error === 'string' && entry.error.length > 0) { + summaryErrors.set(entry.slug, entry.error); + } + const counts = readSummaryCounts(entry); + if (counts) { + summaryCounts.set(entry.slug, counts); + summaryRanJest.set(entry.slug, entry.ranJest === true); + } } - // Log any pcall failures from the Luau template + // Log any failures the Luau template reported const failures = entries.filter((e) => !e.success); if (failures.length > 0) { console.error( - `[batch-log-parser] Luau pcall failures: ${JSON.stringify(failures)}` + `[batch-log-parser] Luau runner failures: ${JSON.stringify(failures)}` ); } console.error( - `[batch-log-parser] Parsed ${entries.length} summary entries, ${failures.length} pcall failures` + `[batch-log-parser] Parsed ${entries.length} summary entries, ${failures.length} reported failures` ); } } @@ -238,6 +314,14 @@ export function parseBatchTestLogs( // attached once rather than repeated per package across a large log. let unattributedClaimed = false; + /** Packages whose run returned its counts, and those left to log scraping. */ + const structuredSlugs: string[] = []; + const scrapedSlugs: string[] = []; + /** Packages reported as failed by a run whose counts show nothing failed. */ + const unexplainedFailures: string[] = []; + /** Packages judged on returned counts alone, their log section having been lost. */ + const sectionlessButCounted: string[] = []; + for (const [packageName, slug] of slugMap) { const attributedLogs = logSections.get(slug); let sectionLogs = attributedLogs ?? ''; @@ -246,28 +330,61 @@ export function parseBatchTestLogs( unattributedClaimed = true; } const summarySuccess = summaryResults.get(slug); + const counts = summaryCounts.get(slug); const reasons: string[] = []; - // The pcall result only proves the script did not throw. It is a floor, not - // a verdict — everything below can fail a run it called successful. + // The summary verdict is a floor, not the whole verdict — everything below + // can fail a run it called successful. It covers both a script that threw + // and one whose returned results said it failed, so the reason comes from + // the runner rather than being guessed at here. let success = summarySuccess ?? false; if (summarySuccess === false) { - reasons.push('the batch runner reported a Luau error'); + reasons.push( + summaryErrors.get(slug) ?? 'the batch runner reported this as failed' + ); } else if (summarySuccess === undefined) { // Absent from the summary is a different fault from failing in it, and // conflating them sends you hunting for a Luau error that never happened. reasons.push('this package is missing from the batch summary'); } + // Returned counts are structural, so a summary that called this a pass + // while reporting failed tests is not believed — no log line involved. + if (success && counts && (counts.failed > 0 || counts.suitesFailed > 0)) { + success = false; + reasons.push( + `the batch summary reported a pass alongside ${counts.failed} failed ` + + `test(s) and ${counts.suitesFailed} failed test suite(s)` + ); + } + + // The reverse contradiction: failure reported over counts where nothing + // failed. Left as a failure but said out loud — a runner reading the wrong + // field produces exactly this, for every package at once. + if ( + !success && + counts && + counts.failed === 0 && + counts.suitesFailed === 0 + ) { + unexplainedFailures.push(slug); + } + if (attributedLogs !== undefined) { + // Demanding a jest report in the section is a stand-in for proof the + // runner ran, and it is the first thing a truncated log window costs. + // Returned counts are that proof directly, so they retire the stand-in — + // without this the whole structured channel changed nothing in a CI batch, + // where dozens of packages share one log window and a section keeps its + // END marker long after its jest summary was dropped. const outcome = evaluateTestOutcome(attributedLogs, { - requireTestReport: true, + requireTestReport: counts === undefined, }); if (!outcome.success) { success = false; reasons.push(...outcome.failureReasons); } - } else { + } else if (counts === undefined) { // Judged unreadable rather than judged by content: whatever broke // attribution is reason enough not to trust which package a line is from. success = false; @@ -275,6 +392,11 @@ export function parseBatchTestLogs( `no output could be attributed to this package ` + `(${rawLogs.length} chars received, ${beginMarkersSeen} BEGIN markers found)` ); + } else { + // The counts say what happened even though the log does not. Tracebacks + // cannot be checked without the text, though, so this is a narrower + // verdict than a package with a section gets — hence the warning. + sectionlessButCounted.push(slug); } if (partialSections.has(slug)) { @@ -305,9 +427,40 @@ export function parseBatchTestLogs( const error = reasons.length > 0 ? reasons.join('; ') : undefined; - const testCounts = attributedLogs + // Counts the runner returned outrank counts scraped from the section: the + // same numbers when the log survived, real numbers when it did not. + const scrapedCounts = attributedLogs ? parseTestCounts(attributedLogs) : undefined; + const testCounts = counts + ? { passed: counts.passed, failed: counts.failed, total: counts.total } + : scrapedCounts; + const countsSource = counts + ? ('returned' as const) + : scrapedCounts + ? ('scraped' as const) + : undefined; + + if (counts) { + structuredSlugs.push(slug); + } else { + scrapedSlugs.push(slug); + } + + // Two channels reporting the same run must agree. When they do not, one of + // them is lying about what happened and neither total can be trusted on its + // own — said out loud because a returned count that is quietly wrong reads + // like a clean run, which is how a broken structured read stays invisible. + if (counts && scrapedCounts && counts.total !== scrapedCounts.total) { + OutputHelper.warn( + `[batch-log-parser] ${slug}: returned counts disagree with the log — ` + + `the run returned ${counts.passed} passed / ${counts.failed} failed / ` + + `${counts.total} total, its jest report says ${scrapedCounts.passed} / ` + + `${scrapedCounts.failed} / ${scrapedCounts.total}. Reporting the returned ` + + `counts; one of the two channels is wrong.` + ); + } + // Prefer the JSON summary (authoritative, immune to log reordering); // fall back to the END-marker value if the summary was truncated. const durationMs = summaryDurations.get(slug) ?? markerDurations.get(slug); @@ -318,13 +471,95 @@ export function parseBatchTestLogs( durationMs, testCounts, tracebackCount, + countsSource, + testResults: counts + ? { + success: summarySuccess === true, + ranJest: summaryRanJest.get(slug) === true, + ...counts, + failures: [], + omittedFailures: 0, + error: summaryErrors.get(slug), + } + : undefined, error, }); } + reportCountsProvenance( + structuredSlugs, + scrapedSlugs, + unexplainedFailures, + sectionlessButCounted + ); + return results; } +/** + * Say where this batch's counts came from, every run. + * + * The structured channel exists because Open Cloud truncates a long run's logs. + * A channel that is plumbed but not flowing produces output identical to one + * that is working, so silence here is not evidence of anything — the count is + * stated unconditionally and the fallback is a warning, not a debug line. + */ +function reportCountsProvenance( + structuredSlugs: string[], + scrapedSlugs: string[], + unexplainedFailures: string[], + sectionlessButCounted: string[] +): void { + const total = structuredSlugs.length + scrapedSlugs.length; + if (total === 0) { + return; + } + + // Not a failure — this is the case the structured channel was built for — but + // these packages were judged without their log text, so nothing checked them + // for tracebacks, which jest cannot count and only the log shows. + if (sectionlessButCounted.length > 0) { + OutputHelper.warn( + `[batch-log-parser] ${sectionlessButCounted.length} package(s) were judged on ` + + `their returned counts alone, with no log section to read: ` + + `${sectionlessButCounted.join( + ', ' + )}. Their counts are exact; they were ` + + `not checked for Luau tracebacks, which only the log shows.` + ); + } + + OutputHelper.info( + `[batch-log-parser] Counts returned by the run for ${structuredSlugs.length} ` + + `of ${total} package(s); ${scrapedSlugs.length} scraped from logs.` + ); + + // Failing every package while every package's counts are clean is a signature, + // not a coincidence: it means the runner's verdict, not the tests, is wrong. + if (unexplainedFailures.length > 0) { + OutputHelper.warn( + `[batch-log-parser] ${unexplainedFailures.length} of ${total} package(s) were ` + + `failed by a run whose own counts show nothing failed: ` + + `${unexplainedFailures.join( + ', ' + )}. The failures stand — a runner may know ` + + `something its counts cannot express — but a verdict no count supports is ` + + `far more likely to be the runner reading the wrong field.` + ); + } + + if (scrapedSlugs.length > 0) { + OutputHelper.warn( + `[batch-log-parser] ${scrapedSlugs.length} package(s) returned no test ` + + `results, so their counts were scraped from log text — the channel Open ` + + `Cloud truncates on long runs: ${scrapedSlugs.join( + ', ' + )}. Their test ` + + `script should end with "return results" (see docs/testing/testing.md).` + ); + } +} + /** * Find the summary array in the lines following the summary marker. * diff --git a/tools/nevermore-cli/src/utils/testing/runner/test-runner.test.ts b/tools/nevermore-cli/src/utils/testing/runner/test-runner.test.ts index dd6b12a1e29..eb011eb6f73 100644 --- a/tools/nevermore-cli/src/utils/testing/runner/test-runner.test.ts +++ b/tools/nevermore-cli/src/utils/testing/runner/test-runner.test.ts @@ -1,6 +1,290 @@ -import { describe, expect, it } from 'vitest'; +import * as fs from 'fs/promises'; +import * as os from 'os'; +import * as path from 'path'; +import { + afterAll, + afterEach, + beforeAll, + describe, + expect, + it, + vi, +} from 'vitest'; +import { type DeployTarget } from '@quenty/nevermore-deploy'; +import { OutputHelper } from '@quenty/cli-output-helpers'; -import { mergeFailureReasons } from './test-runner.js'; +import { mergeFailureReasons, runSingleTestAsync } from './test-runner.js'; +import { TEST_RESULTS_FORMAT } from '../structured-test-results.js'; +import { + type Deployment, + type JobContext, + type ScriptRunResult, +} from '../../job-context/job-context.js'; + +const PASSING_LOGS = [ + 'Test Suites: 3 passed, 3 total', + 'Tests: 25 passed, 25 total', +].join('\n'); + +function structuredResults(overrides: Record = {}) { + return { + format: TEST_RESULTS_FORMAT, + success: true, + ranJest: true, + passed: 25, + failed: 0, + skipped: 0, + total: 25, + suitesPassed: 3, + suitesFailed: 0, + suitesTotal: 3, + failures: [], + omittedFailures: 0, + ...overrides, + }; +} + +/** Collect what the runner said out loud, so silence can be asserted on. */ +function captureWarnings(): () => string[] { + const warnings: string[] = []; + vi.spyOn(OutputHelper, 'warn').mockImplementation((message: string) => { + warnings.push(message); + }); + vi.spyOn(OutputHelper, 'info').mockImplementation(() => {}); + return () => warnings; +} + +afterEach(() => { + vi.restoreAllMocks(); +}); + +/** + * A context that runs nothing: it hands back the run result and logs the test + * wants to reason about. Everything else is the minimum the runner touches. + */ +function createContext(run: ScriptRunResult, logs: string): JobContext { + return { + buildPlaceAsync: async (options) => ({ + rbxlPath: 'unused.rbxl', + target: options.target, + }), + deployBuiltPlaceAsync: async () => ({} as Deployment), + runScriptAsync: async () => run, + getLogsAsync: async () => logs, + releaseAsync: async () => {}, + releaseBuiltPlaceAsync: async () => {}, + disposeAsync: async () => {}, + }; +} + +describe('runSingleTestAsync', () => { + let packagePath: string; + + beforeAll(async () => { + packagePath = await fs.mkdtemp(path.join(os.tmpdir(), 'nevermore-test-')); + await fs.writeFile( + path.join(packagePath, 'ServerMain.server.lua'), + 'return nil\n' + ); + }); + + afterAll(async () => { + await fs.rm(packagePath, { recursive: true, force: true }); + }); + + async function runAsync(run: ScriptRunResult, logs: string) { + return runSingleTestAsync(createContext(run, logs), { + packagePath, + packageName: 'maid', + target: { + scriptTemplate: 'ServerMain.server.lua', + } as unknown as DeployTarget, + }); + } + + it('fails a run whose returned results say it failed', async () => { + // The runner used to announce this by throwing, which failed the task. Now + // it returns the verdict, and these logs are what a truncated window leaves + // behind: a jest report from a suite that passed and no sign of the one + // that did not. + const result = await runAsync( + { + success: true, + returnValues: [structuredResults({ success: false, failed: 2 })], + }, + PASSING_LOGS + ); + + expect(result.success).toBe(false); + expect(result.error).toContain('2 test(s) failed'); + }); + + it('reports the counts the run returned, not the ones in the log', async () => { + const result = await runAsync( + { + success: true, + returnValues: [structuredResults({ passed: 400, total: 400 })], + }, + PASSING_LOGS + ); + + expect(result.success).toBe(true); + expect(result.testCounts).toEqual({ passed: 400, failed: 0, total: 400 }); + }); + + it('accepts returned results as proof the runner ran', async () => { + // Demanding a jest report in the logs is a stand-in for that proof, and it + // is the first thing a truncated log window costs. + const result = await runAsync( + { success: true, returnValues: [structuredResults()] }, + '(logs truncated)' + ); + + expect(result.success).toBe(true); + }); + + it('still fails a run with a traceback in its logs', async () => { + // Jest cannot count a deferred-callback crash, so passing results are no + // reason to stop reading the logs. + const result = await runAsync( + { success: true, returnValues: [structuredResults()] }, + `${PASSING_LOGS}\nStack Begin\nScript 'maid.spec', Line 4\nStack End` + ); + + expect(result.success).toBe(false); + }); + + it('falls back to the logs for a script that returned nothing', async () => { + // A test script written before results were returned. It must keep working, + // and it must not start passing when its logs say otherwise. + const failing = await runAsync( + { success: true, returnValues: [] }, + 'Tests: 2 failed, 23 passed, 25 total' + ); + expect(failing.success).toBe(false); + expect(failing.error).toContain('2 test(s) failed'); + + const passing = await runAsync( + { success: true, returnValues: [] }, + PASSING_LOGS + ); + expect(passing.success).toBe(true); + expect(passing.testCounts).toEqual({ passed: 25, failed: 0, total: 25 }); + }); + + it('fails a script that returned nothing and logged nothing', async () => { + const result = await runAsync( + { success: true, returnValues: undefined }, + '' + ); + + expect(result.success).toBe(false); + expect(result.error).toContain('nothing proves any test ran'); + }); + + it('records where the counts came from', async () => { + const returned = await runAsync( + { success: true, returnValues: [structuredResults()] }, + PASSING_LOGS + ); + expect(returned.countsSource).toBe('returned'); + + const scraped = await runAsync( + { success: true, returnValues: [] }, + PASSING_LOGS + ); + expect(scraped.countsSource).toBe('scraped'); + }); + + it('warns out loud when the run returned no results', async () => { + // Silence here is what let the structured channel ship inert: a run that + // fell back to scraping produced output identical to one that did not. + const warnings = captureWarnings(); + + await runAsync({ success: true, returnValues: [] }, PASSING_LOGS); + + expect(warnings().some((w) => w.includes('returned no test results'))).toBe( + true + ); + }); + + it('fails a package with specs whose run never reached jest', async () => { + // A Rojo regression that stops shipping jest.config turns a real suite into + // a smoke test. Before results were returned, the required jest report caught + // that; a smoke-test result retires the report and would otherwise pass with + // zero tests. + await fs.mkdir(path.join(packagePath, 'src'), { recursive: true }); + await fs.writeFile( + path.join(packagePath, 'src', 'jest.config.lua'), + 'return {}\n' + ); + + const result = await runAsync( + { + success: true, + returnValues: [ + structuredResults({ ranJest: false, passed: 0, total: 0 }), + ], + }, + '[NevermoreTestRunner] No jest.config found — smoke test passed (boot success)' + ); + + expect(result.success).toBe(false); + expect(result.error).toContain('none of its specs ran'); + + await fs.rm(path.join(packagePath, 'src'), { + recursive: true, + force: true, + }); + }); + + it('passes a package with no specs that smoke tested', async () => { + // No jest.config on disk, so a smoke test is the whole contract. + const result = await runAsync( + { + success: true, + returnValues: [ + structuredResults({ ranJest: false, passed: 0, total: 0 }), + ], + }, + '[NevermoreTestRunner] No jest.config found — smoke test passed (boot success)' + ); + + expect(result.success).toBe(true); + }); + + it('takes results a context resolved for it, without re-reporting', async () => { + // Aggregated batch mode: one execution covers every package, so the batch + // log parser splits the per-package results out and has already reported + // provenance for the whole batch. + const warnings = captureWarnings(); + + const result = await runAsync( + { + success: true, + testResults: { + success: false, + ranJest: true, + passed: 273, + failed: 2, + skipped: 0, + total: 275, + suitesPassed: 8, + suitesFailed: 1, + suitesTotal: 9, + failures: [], + omittedFailures: 0, + }, + }, + '(this package’s section was truncated away)' + ); + + expect(result.success).toBe(false); + expect(result.countsSource).toBe('returned'); + expect(result.testCounts).toEqual({ passed: 273, failed: 2, total: 275 }); + expect(warnings()).toHaveLength(0); + }); +}); describe('mergeFailureReasons', () => { it('does not repeat reasons the context already reported', () => { diff --git a/tools/nevermore-cli/src/utils/testing/runner/test-runner.ts b/tools/nevermore-cli/src/utils/testing/runner/test-runner.ts index 6b94c96f4f1..42bdb440428 100644 --- a/tools/nevermore-cli/src/utils/testing/runner/test-runner.ts +++ b/tools/nevermore-cli/src/utils/testing/runner/test-runner.ts @@ -8,6 +8,14 @@ import { parseTestLogs, parseTestCounts, } from '../test-log-parser.js'; +import { + type StructuredTestResults, + describeUnexplainedVerdict, + findStructuredTestResults, + structuredFailureReasons, + toParsedTestCounts, +} from '../structured-test-results.js'; +import { OutputHelper } from '@quenty/cli-output-helpers'; import { buildDeployMetadataAttributes, gatherGitDeployInfo, @@ -28,6 +36,12 @@ export interface SingleTestResult { durationMs?: number; /** Why the run failed, when the runner can say more than "it failed". */ error?: string; + /** + * Where `testCounts` came from. `returned` means the run handed them over as a + * value; `scraped` means they were read out of log text, which is the channel + * truncation destroys. Absent when there are no counts at all. + */ + countsSource?: 'returned' | 'scraped'; } /** @@ -126,22 +140,79 @@ export async function runSingleTestAsync( }); const rawLogs = await context.getLogsAsync(deployment); + + // The runner used to announce a failing suite by throwing, which failed the + // task. It returns its verdict now, so the verdict has to be read: without + // this, a failing suite whose report fell outside the truncated log window + // would come back a pass. + // + // A context that already resolved the results (aggregated batch, where one + // execution's return value belongs to no single package) hands them over + // directly; otherwise they are decoded from what the script returned. + const structured = + result.testResults ?? findStructuredTestResults(result.returnValues); + // A probe script is arbitrary Luau with no jest in it, so demanding a test // report would fail every --script-text run. Everything else must prove a - // runner spoke before it can pass. + // runner spoke before it can pass — and returned results are that proof, + // where a scraped report is only what survived truncation. const parsed = parseTestLogs(rawLogs, { - requireTestReport: scriptText === undefined, + requireTestReport: scriptText === undefined && structured === undefined, }); - const reasons = mergeFailureReasons( - result.errorMessage, - parsed.failureReasons - ); + // A run that reports it never reached jest, for a package that has a + // jest.config on disk, did not test anything — the built place lost the + // config. Before results were returned, the required jest report caught this + // as "nothing proves any test ran"; a smoke-test result would otherwise + // retire that check and pass with zero tests. + const smokeTestedWithSpecs = + structured !== undefined && + !structured.ranJest && + scriptText === undefined && + (await packageHasJestConfigAsync(packagePath)); + + const reasons = mergeFailureReasons(result.errorMessage, [ + ...(smokeTestedWithSpecs + ? [ + 'this package has a jest.config but the run reported no jest.config ' + + 'in the built place, so none of its specs ran', + ] + : []), + ...(structured ? structuredFailureReasons(structured) : []), + ...parsed.failureReasons, + ]); + + // Returned counts outrank scraped ones: same numbers when the log + // survived, real numbers when it did not. + const scrapedCounts = parseTestCounts(parsed.logs); + const testCounts = structured + ? toParsedTestCounts(structured) + : scrapedCounts; + + reportCountsProvenance({ + packageName, + structured, + scrapedCounts, + // A probe is not a test run, so it has no results to be missing. Neither + // is one package of an aggregated batch, whose context reported the whole + // batch's provenance in one line already. + silent: scriptText !== undefined || result.testResults !== undefined, + hadReturnChannel: result.returnValues !== undefined, + }); return { - success: result.success && parsed.success, + success: + result.success && + parsed.success && + (structured?.success ?? true) && + !smokeTestedWithSpecs, logs: parsed.logs, - testCounts: parseTestCounts(parsed.logs), + testCounts, + countsSource: structured + ? 'returned' + : scrapedCounts + ? 'scraped' + : undefined, durationMs: result.durationMs, error: reasons.length > 0 ? reasons.join('; ') : undefined, }; @@ -150,6 +221,121 @@ export async function runSingleTestAsync( } } +/** + * Say where a run's counts came from, and complain when they had to be scraped. + * + * The structured channel exists because Open Cloud truncates a long run's logs. + * A channel that is plumbed but not flowing produces output identical to one + * that works, so the fallback is a warning rather than silence — the first + * version of this shipped inert and looked green. + */ +function reportCountsProvenance(options: { + packageName: string; + structured?: StructuredTestResults; + scrapedCounts?: ParsedTestCounts; + silent: boolean; + hadReturnChannel: boolean; +}): void { + const { packageName, structured, scrapedCounts, silent, hadReturnChannel } = + options; + + if (silent) { + return; + } + + if (structured) { + OutputHelper.info( + `${packageName}: counts returned by the run — ` + + `${structured.passed} passed, ${structured.failed} failed, ` + + `${structured.total} total.` + ); + + const unexplained = describeUnexplainedVerdict(structured); + if (unexplained) { + OutputHelper.warn(`${packageName}: ${unexplained}`); + } + + // A suite that runs and counts nothing is not the same as one that passed. + if (structured.ranJest && structured.total === 0) { + OutputHelper.warn( + `${packageName}: jest ran but found no tests to run. If this package has ` + + `specs, they are not reaching the test place.` + ); + } + + // Two channels reporting the same run must agree. When they do not, one is + // lying and neither total stands on its own. + if (scrapedCounts && scrapedCounts.total !== structured.total) { + OutputHelper.warn( + `${packageName}: returned counts disagree with the log — the run returned ` + + `${structured.passed}/${structured.failed}/${structured.total} ` + + `(passed/failed/total), its jest report says ` + + `${scrapedCounts.passed}/${scrapedCounts.failed}/${scrapedCounts.total}. ` + + `Reporting the returned counts; one of the two channels is wrong.` + ); + } + return; + } + + OutputHelper.warn( + `${packageName}: the run returned no test results, so its counts were ` + + (scrapedCounts ? 'scraped from log text' : 'unavailable') + + ` — the channel Open Cloud truncates on long runs. ` + + (hadReturnChannel + ? 'The run delivered a return channel but nothing recognizable in it: the ' + + 'test script should end with "return results" (see docs/testing/testing.md).' + : 'No return channel was delivered at all, so the transport lost it.') + ); +} + +/** + * Whether this package ships a jest.config, i.e. whether it has specs to run. + * + * Deliberately a bounded walk, not a recursive glob: every package under `src/` + * has a symlinked, self-referential `node_modules`, so recursive search here + * hangs. Two levels covers the layouts in use (`src/jest.config.lua` for a + * package, `src/modules/jest.config.lua` for a game) without needing a list of + * them. + */ +export async function packageHasJestConfigAsync( + packagePath: string +): Promise { + async function scanAsync(dir: string, depth: number): Promise { + let entries; + try { + entries = await fs.readdir(dir, { withFileTypes: true }); + } catch { + return false; + } + + for (const entry of entries) { + if (entry.isFile() && entry.name.startsWith('jest.config')) { + return true; + } + } + + if (depth === 0) { + return false; + } + + for (const entry of entries) { + if (!entry.isDirectory()) { + continue; + } + // `test` holds the test place, never the specs the place runs. + if (entry.name === 'node_modules' || entry.name === 'test') { + continue; + } + if (await scanAsync(path.join(dir, entry.name), depth - 1)) { + return true; + } + } + return false; + } + + return scanAsync(packagePath, 2); +} + /** * Read a test script from the deploy target's configured script path. */ diff --git a/tools/nevermore-cli/src/utils/testing/structured-test-results.test.ts b/tools/nevermore-cli/src/utils/testing/structured-test-results.test.ts new file mode 100644 index 00000000000..0505261d009 --- /dev/null +++ b/tools/nevermore-cli/src/utils/testing/structured-test-results.test.ts @@ -0,0 +1,267 @@ +/** + * Unit tests for decoding the results a test run returns. The decoder stands + * between a transport and a pass/fail verdict, so the cases that matter are the + * ones where a value is not what it claims to be. + */ + +import { describe, expect, it } from 'vitest'; + +import { + TEST_RESULTS_FORMAT, + decodeStructuredTestResults, + describeUnexplainedVerdict, + findStructuredTestResults, + structuredFailureReasons, + toParsedTestCounts, +} from './structured-test-results.js'; + +function results(overrides: Record = {}) { + return { + format: TEST_RESULTS_FORMAT, + success: true, + ranJest: true, + passed: 25, + failed: 0, + skipped: 1, + total: 26, + suitesPassed: 3, + suitesFailed: 0, + suitesTotal: 3, + failures: [], + omittedFailures: 0, + ...overrides, + }; +} + +describe('decodeStructuredTestResults', () => { + it('decodes a results table', () => { + const decoded = decodeStructuredTestResults(results()); + + expect(decoded).toEqual({ + success: true, + ranJest: true, + passed: 25, + failed: 0, + skipped: 1, + total: 26, + suitesPassed: 3, + suitesFailed: 0, + suitesTotal: 3, + failures: [], + omittedFailures: 0, + error: undefined, + }); + }); + + it('ignores a value that is not tagged as results', () => { + // A probe run (--script-text) returns whatever it likes, and none of it is + // a verdict on anything. + expect(decodeStructuredTestResults({ success: true })).toBeUndefined(); + expect( + decodeStructuredTestResults({ format: 'something-else', success: true }) + ).toBeUndefined(); + expect( + decodeStructuredTestResults('nevermore-test-results@1') + ).toBeUndefined(); + expect(decodeStructuredTestResults(undefined)).toBeUndefined(); + expect(decodeStructuredTestResults([results()])).toBeUndefined(); + }); + + it('reads only an explicit true as a pass', () => { + // Anything else means the verdict did not survive the trip, and a lost + // verdict must not read as a passing one. + expect( + decodeStructuredTestResults(results({ success: 'yes' }))?.success + ).toBe(false); + expect( + decodeStructuredTestResults(results({ success: undefined }))?.success + ).toBe(false); + }); + + it('treats a malformed count as zero rather than as a number', () => { + const decoded = decodeStructuredTestResults( + results({ passed: 'lots', total: null, failed: Number.NaN }) + ); + + expect(decoded?.passed).toBe(0); + expect(decoded?.total).toBe(0); + expect(decoded?.failed).toBe(0); + }); + + it('keeps only well-formed failures', () => { + const decoded = decodeStructuredTestResults( + results({ + failures: [ + { name: 'Maid does a thing', message: 'expected true' }, + { name: 'Maid does another thing' }, + { message: 'no name' }, + 'not a failure', + ], + }) + ); + + expect(decoded?.failures).toEqual([ + { name: 'Maid does a thing', message: 'expected true' }, + { name: 'Maid does another thing', message: undefined }, + ]); + }); + + it('accepts an empty failure list however the transport spelled it', () => { + // An empty Lua table marshals as an array on the bridge and can arrive as an + // object from the cloud. + expect( + decodeStructuredTestResults(results({ failures: {} }))?.failures + ).toEqual([]); + }); +}); + +describe('findStructuredTestResults', () => { + it('finds results among other returned values', () => { + const found = findStructuredTestResults([ + 42, + 'log', + results({ passed: 9 }), + ]); + + expect(found?.passed).toBe(9); + }); + + it('is undefined when the script returned nothing', () => { + // A test script written before this convention returns nil, which is the + // case that has to keep falling back to the logs. + expect(findStructuredTestResults([])).toBeUndefined(); + expect(findStructuredTestResults(undefined)).toBeUndefined(); + }); +}); + +describe('structuredFailureReasons', () => { + it('says nothing about a passing run', () => { + expect( + structuredFailureReasons(decodeStructuredTestResults(results())!) + ).toEqual([]); + }); + + it('words counts the way the log parser does, so the overlap merges', () => { + const reasons = structuredFailureReasons( + decodeStructuredTestResults( + results({ success: false, failed: 2, suitesFailed: 1 }) + )! + ); + + expect(reasons).toContain('2 test(s) failed'); + expect(reasons).toContain('1 test suite(s) failed'); + }); + + it('explains a failure no count can account for', () => { + const reasons = structuredFailureReasons( + decodeStructuredTestResults(results({ success: false }))! + ); + + expect(reasons).toEqual(['the test runner reported the run as failed']); + }); + + it('carries the message the runner supplied', () => { + const reasons = structuredFailureReasons( + decodeStructuredTestResults( + results({ + success: false, + error: '[NevermoreTestRunner] Jest run failed', + }) + )! + ); + + expect(reasons).toContain('[NevermoreTestRunner] Jest run failed'); + }); +}); + +describe('structuredFailureReasons wording', () => { + it('never builds a reason out of zeros', () => { + // "0 test(s) and 0 test suite(s) failed" was a real failure reason once. A + // reason saying nothing failed is unreadable as either verdict, and it is + // what made the bug behind it hard to see. + for (const overrides of [ + { success: false }, + { success: false, failed: 0, suitesFailed: 0 }, + { + success: false, + error: '[NevermoreTestRunner] the run was interrupted', + }, + ]) { + const reasons = structuredFailureReasons( + decodeStructuredTestResults(results(overrides))! + ); + + expect(reasons.length).toBeGreaterThan(0); + for (const reason of reasons) { + expect(reason).not.toMatch(/\b0 test\(s\)/); + expect(reason).not.toMatch(/\b0 test suite\(s\)/); + } + } + }); + + it('does not restate the counts the reasons already carry', () => { + const reasons = structuredFailureReasons( + decodeStructuredTestResults( + results({ + success: false, + failed: 2, + suitesFailed: 1, + error: + '[NevermoreTestRunner] 2 test(s) failed, 1 test suite(s) failed', + }) + )! + ); + + expect(reasons).toEqual(['1 test suite(s) failed', '2 test(s) failed']); + }); +}); + +describe('describeUnexplainedVerdict', () => { + it('is silent about a passing run', () => { + // The direction that broke: a fully passing result must read as a pass and + // raise nothing at all. + expect( + describeUnexplainedVerdict(decodeStructuredTestResults(results())!) + ).toBeUndefined(); + }); + + it('is silent about a failure its counts explain', () => { + expect( + describeUnexplainedVerdict( + decodeStructuredTestResults(results({ success: false, failed: 2 }))! + ) + ).toBeUndefined(); + expect( + describeUnexplainedVerdict( + decodeStructuredTestResults( + results({ success: false, suitesFailed: 1 }) + )! + ) + ).toBeUndefined(); + }); + + it('describes a failure no count supports', () => { + // The exact signature of a runner reading the wrong field: plausible counts, + // and every package failed anyway. + const description = describeUnexplainedVerdict( + decodeStructuredTestResults( + results({ success: false, passed: 311, failed: 0, total: 311 }) + )! + ); + + expect(description).toContain('nothing failed'); + expect(description).toContain('311 passed'); + }); +}); + +describe('toParsedTestCounts', () => { + it('narrows to the counts the reporters render', () => { + expect(toParsedTestCounts(decodeStructuredTestResults(results())!)).toEqual( + { + passed: 25, + failed: 0, + total: 26, + } + ); + }); +}); diff --git a/tools/nevermore-cli/src/utils/testing/structured-test-results.ts b/tools/nevermore-cli/src/utils/testing/structured-test-results.ts new file mode 100644 index 00000000000..149e522f109 --- /dev/null +++ b/tools/nevermore-cli/src/utils/testing/structured-test-results.ts @@ -0,0 +1,199 @@ +import { type ParsedTestCounts } from './test-log-parser.js'; + +/** + * Tag NevermoreTestRunnerUtils stamps on the results table a test script + * returns. A test place also runs probe scripts (`--script-text`) that return + * whatever they like, so results are recognized by this rather than by shape. + */ +export const TEST_RESULTS_FORMAT = 'nevermore-test-results@1'; + +/** One failed test, or one suite that failed before its tests could run. */ +export interface StructuredTestFailure { + name: string; + message?: string; +} + +/** + * What a test run says about itself, returned as a value instead of printed. + * + * Open Cloud truncates a long run's logs, so counts scraped out of that text go + * missing on exactly the runs where they matter most. Mirrors the + * `TestRunResults` table in `NevermoreTestRunnerUtils`. + */ +export interface StructuredTestResults { + success: boolean; + /** False for a smoke test, whose counts are all zero because nothing counted. */ + ranJest: boolean; + passed: number; + failed: number; + /** Pending plus todo. */ + skipped: number; + total: number; + suitesPassed: number; + suitesFailed: number; + suitesTotal: number; + /** Capped by the runner; `omittedFailures` says how many did not fit. */ + failures: StructuredTestFailure[]; + omittedFailures: number; + /** Why the run failed when no individual test can say so. */ + error?: string; +} + +function readNumber(source: Record, key: string): number { + const value = source[key]; + return typeof value === 'number' && Number.isFinite(value) ? value : 0; +} + +function readFailures(value: unknown): StructuredTestFailure[] { + if (!Array.isArray(value)) { + return []; + } + + const failures: StructuredTestFailure[] = []; + for (const entry of value) { + if (typeof entry !== 'object' || entry === null) { + continue; + } + const record = entry as Record; + if (typeof record.name !== 'string') { + continue; + } + failures.push({ + name: record.name, + message: typeof record.message === 'string' ? record.message : undefined, + }); + } + return failures; +} + +/** + * Decode a single returned value into results, or undefined if it is not one. + * + * Fields are read defensively rather than trusted: the value crossed a + * transport, and the two transports spell exotic Luau types differently, so a + * malformed field must not become a verdict. + */ +export function decodeStructuredTestResults( + value: unknown +): StructuredTestResults | undefined { + if (typeof value !== 'object' || value === null || Array.isArray(value)) { + return undefined; + } + + const record = value as Record; + if (record.format !== TEST_RESULTS_FORMAT) { + return undefined; + } + + return { + // Only an explicit `true` is a pass. A results table that lost its verdict + // in transit must not read as one. + success: record.success === true, + ranJest: record.ranJest === true, + passed: readNumber(record, 'passed'), + failed: readNumber(record, 'failed'), + skipped: readNumber(record, 'skipped'), + total: readNumber(record, 'total'), + suitesPassed: readNumber(record, 'suitesPassed'), + suitesFailed: readNumber(record, 'suitesFailed'), + suitesTotal: readNumber(record, 'suitesTotal'), + failures: readFailures(record.failures), + omittedFailures: readNumber(record, 'omittedFailures'), + error: typeof record.error === 'string' ? record.error : undefined, + }; +} + +/** + * Find the results a run returned, if it returned any. + * + * `undefined` covers both "the transport delivered no return channel" and "the + * script returned nothing recognizable" — a caller falls back to the logs + * either way, which is what a test script written before this convention gets. + */ +export function findStructuredTestResults( + returnValues: unknown[] | undefined +): StructuredTestResults | undefined { + if (!returnValues) { + return undefined; + } + + for (const value of returnValues) { + const results = decodeStructuredTestResults(value); + if (results) { + return results; + } + } + return undefined; +} + +/** + * Describe a verdict the run's own counts do not support, for a warning. + * + * A run reporting failure while nothing in it failed is self-contradicting, and + * it is precisely what a runner reading the wrong field produces: the counts + * stay plausible, so nothing looks wrong except that every package fails at + * once. That shipped once — a runner consulted jest-lua's inverted + * `AggregatedResult.success` and failed every passing suite. + * + * Reported, never corrected. A runner that says it failed may know something its + * counts cannot express (an interrupted run, a snapshot check, a reporter + * error), so the verdict stands and the contradiction is made loud instead. + */ +export function describeUnexplainedVerdict( + results: StructuredTestResults +): string | undefined { + if (results.success || results.failed > 0 || results.suitesFailed > 0) { + return undefined; + } + + return ( + `the run reports failure but its own counts show nothing failed ` + + `(${results.passed} passed, 0 failed, ${results.total} total, ` + + `0 of ${results.suitesTotal} suite(s) failed)` + + (results.error + ? `. It says: ${results.error}` + : ' and gives no reason at all') + ); +} + +/** Counts in the shape the reporters already render. */ +export function toParsedTestCounts( + results: StructuredTestResults +): ParsedTestCounts { + return { + passed: results.passed, + failed: results.failed, + total: results.total, + }; +} + +/** + * Why the runner says the run failed. Empty when it says it passed. + * + * Phrased to match the log parser's wording so that merging the two sources + * collapses the overlap instead of reporting every failure twice. + */ +export function structuredFailureReasons( + results: StructuredTestResults +): string[] { + if (results.success) { + return []; + } + + const reasons: string[] = []; + if (results.suitesFailed > 0) { + reasons.push(`${results.suitesFailed} test suite(s) failed`); + } + if (results.failed > 0) { + reasons.push(`${results.failed} test(s) failed`); + } + + // The runner's own message restates those two lines when a count explains the + // failure, so it is only worth repeating when nothing else can say why — + // an interrupted run, a failed snapshot check, a result shape it could not + // read. That case is also the one where a reason is most needed. + if (reasons.length === 0) { + reasons.push(results.error ?? 'the test runner reported the run as failed'); + } + return reasons; +} diff --git a/tools/nevermore-cli/templates/batch-test-runner.luau b/tools/nevermore-cli/templates/batch-test-runner.luau index 2f66b85d95f..7172a68f33c 100644 --- a/tools/nevermore-cli/templates/batch-test-runner.luau +++ b/tools/nevermore-cli/templates/batch-test-runner.luau @@ -29,13 +29,58 @@ for _, child in ServerScriptService:GetChildren() do allPackages[child.Name] = child end +-- Tag on a NevermoreTestRunnerUtils results table. A test script returns one; +-- one written before the convention returns nil, and a probe returns whatever +-- it likes, so only a tagged table is read as results. +local RESULTS_FORMAT = "nevermore-test-results@1" + +type TestCounts = { + passed: number, + failed: number, + skipped: number, + total: number, + suitesPassed: number, + suitesFailed: number, + suitesTotal: number, +} + type Result = { success: boolean, slug: string, durationMs: number, error: string?, + -- Absent when the package's test script returned nothing structured, which + -- is the only case where a reader still has to fall back to its logs. + counts: TestCounts?, + -- False for a smoke test, whose counts are zero because nothing counted them. + -- Carried so a reader never has to guess whether zero means "no tests". + ranJest: boolean?, } +-- Counts only. This summary is printed as one log line and read back by the +-- CLI, so it stays short enough to survive whatever the engine does to a long +-- line; the failure text a run also returns is per-package and already in that +-- package's own log section. +local function toCounts(returned: any): TestCounts? + if type(returned) ~= "table" or returned.format ~= RESULTS_FORMAT then + return nil + end + + local counts: { [string]: number } = {} + for _, field in { "passed", "failed", "skipped", "total", "suitesPassed", "suitesFailed", "suitesTotal" } do + if type(returned[field]) ~= "number" then + return nil + end + counts[field] = returned[field] + end + + return counts :: any +end + +local function isStructuredFailure(returned: any): boolean + return type(returned) == "table" and returned.format == RESULTS_FORMAT and returned.success ~= true +end + local results: { Result } = {} for _, slug in packageSlugs do @@ -76,12 +121,12 @@ for _, slug in packageSlugs do -- is amortized across all packages and would otherwise dominate fast tests. local startClock = os.clock() - local testOk, testErr = pcall(function() + local testOk, returned = pcall(function() local fn, compileErr = loadstring(scriptSource, slug) if not fn then error("Compile error: " .. tostring(compileErr)) end - fn() + return fn() end) local durationMs = math.floor((os.clock() - startClock) * 1000 + 0.5) @@ -118,14 +163,30 @@ for _, slug in packageSlugs do RunService.Heartbeat:Wait() end - if testOk then - print("===BATCH_TEST_END " .. slug .. " PASS " .. tostring(durationMs) .. "===") - table.insert(results, { slug = slug, success = true, durationMs = durationMs }) - else - warn("[BatchTest] " .. slug .. ": " .. tostring(testErr)) - print("===BATCH_TEST_END " .. slug .. " FAIL " .. tostring(durationMs) .. "===") - table.insert(results, { slug = slug, success = false, durationMs = durationMs, error = tostring(testErr) }) + -- A test script that does not throw has still failed if the results it + -- returned say so, which is the whole reason it returns them: a failing suite + -- used to announce itself by erroring, and an error carries no counts. + local counts = if testOk then toCounts(returned) else nil + local success = testOk and not isStructuredFailure(returned) + local failureReason: string? = nil + if not testOk then + failureReason = tostring(returned) + elseif not success then + failureReason = tostring((returned :: any).error or "the test runner reported the run as failed") + end + + if failureReason then + warn("[BatchTest] " .. slug .. ": " .. failureReason) end + print("===BATCH_TEST_END " .. slug .. (if success then " PASS " else " FAIL ") .. tostring(durationMs) .. "===") + table.insert(results, { + slug = slug, + success = success, + durationMs = durationMs, + error = failureReason, + counts = counts, + ranJest = if counts then (returned :: any).ranJest == true else nil, + }) end -- Restore all packages diff --git a/tools/nevermore-cli/templates/game-template/src/scripts/Server/ServerMain.server.lua b/tools/nevermore-cli/templates/game-template/src/scripts/Server/ServerMain.server.lua index 3d5bff7f560..b3bac6ccbf9 100644 --- a/tools/nevermore-cli/templates/game-template/src/scripts/Server/ServerMain.server.lua +++ b/tools/nevermore-cli/templates/game-template/src/scripts/Server/ServerMain.server.lua @@ -10,8 +10,9 @@ local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root.game) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root.game) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/tools/nevermore-cli/templates/nevermore-library-package-template/test/scripts/Server/ServerMain.server.lua b/tools/nevermore-cli/templates/nevermore-library-package-template/test/scripts/Server/ServerMain.server.lua index a658fb4e6e9..75d8ad9dcfe 100644 --- a/tools/nevermore-cli/templates/nevermore-library-package-template/test/scripts/Server/ServerMain.server.lua +++ b/tools/nevermore-cli/templates/nevermore-library-package-template/test/scripts/Server/ServerMain.server.lua @@ -8,6 +8,7 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end diff --git a/tools/nevermore-cli/templates/nevermore-service-package-template/test/scripts/Server/ServerMain.server.lua b/tools/nevermore-cli/templates/nevermore-service-package-template/test/scripts/Server/ServerMain.server.lua index 682905eaf32..bd3be1dac69 100644 --- a/tools/nevermore-cli/templates/nevermore-service-package-template/test/scripts/Server/ServerMain.server.lua +++ b/tools/nevermore-cli/templates/nevermore-service-package-template/test/scripts/Server/ServerMain.server.lua @@ -8,8 +8,9 @@ local loader = root:FindFirstChild("LoaderUtils", true).Parent local require = require(loader).bootstrapGame(root) local NevermoreTestRunnerUtils = require("NevermoreTestRunnerUtils") -if NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) then - return +local results = NevermoreTestRunnerUtils.runTestsIfNeededAsync(root) +if results then + return results end local serviceBag = require("ServiceBag").new() diff --git a/tools/studio-bridge/src/bridge/bridge-session.test.ts b/tools/studio-bridge/src/bridge/bridge-session.test.ts index b93f39ec6ca..f7afd7bc447 100644 --- a/tools/studio-bridge/src/bridge/bridge-session.test.ts +++ b/tools/studio-bridge/src/bridge/bridge-session.test.ts @@ -174,6 +174,51 @@ describe('BridgeSession', () => { expect(result.success).toBe(false); expect(result.error).toBe('boom'); }); + + it('surfaces the values the script returned', async () => { + const handle = new MockTransportHandle(); + handle.sendActionAsync.mockResolvedValueOnce({ + type: 'scriptComplete', + sessionId: 'session-1', + payload: { + success: true, + returnValues: [{ counts: { passed: 3 } }, 42], + }, + }); + + const session = new BridgeSession(createSessionInfo(), handle); + const result = await session.execAsync('return results, 42'); + + expect(result.returnValues).toEqual([{ counts: { passed: 3 } }, 42]); + }); + + it('leaves returnValues undefined when the plugin reported none', async () => { + const handle = new MockTransportHandle(); + handle.sendActionAsync.mockResolvedValueOnce({ + type: 'scriptComplete', + sessionId: 'session-1', + payload: { success: true }, + }); + + const session = new BridgeSession(createSessionInfo(), handle); + const result = await session.execAsync('print("hello")'); + + expect(result.returnValues).toBeUndefined(); + }); + + it('leaves returnValues undefined on an error response', async () => { + const handle = new MockTransportHandle(); + handle.sendActionAsync.mockResolvedValueOnce({ + type: 'error', + sessionId: 'session-1', + payload: { code: 'SCRIPT_RUNTIME_ERROR', message: 'boom' }, + }); + + const session = new BridgeSession(createSessionInfo(), handle); + const result = await session.execAsync('error("boom")'); + + expect(result.returnValues).toBeUndefined(); + }); }); describe('queryStateAsync', () => { diff --git a/tools/studio-bridge/src/bridge/bridge-session.ts b/tools/studio-bridge/src/bridge/bridge-session.ts index f26a3cbdffe..c488ed51e3d 100644 --- a/tools/studio-bridge/src/bridge/bridge-session.ts +++ b/tools/studio-bridge/src/bridge/bridge-session.ts @@ -118,6 +118,7 @@ export class BridgeSession extends EventEmitter { success: result.payload.success, output, error: result.payload.error, + returnValues: result.payload.returnValues, }; } diff --git a/tools/studio-bridge/src/bridge/types.ts b/tools/studio-bridge/src/bridge/types.ts index 200ccfc6cd5..6480f29b905 100644 --- a/tools/studio-bridge/src/bridge/types.ts +++ b/tools/studio-bridge/src/bridge/types.ts @@ -11,10 +11,17 @@ import type { Capability, DataModelInstance, OutputLevel, + SerializedReturnValue, } from '../server/web-socket-protocol.js'; // Re-export protocol types used in the public API -export type { StudioState, Capability, DataModelInstance, OutputLevel }; +export type { + StudioState, + Capability, + DataModelInstance, + OutputLevel, + SerializedReturnValue, +}; export type SessionContext = 'edit' | 'client' | 'server'; export type SessionOrigin = 'user' | 'managed'; @@ -47,6 +54,12 @@ export interface ExecResult { success: boolean; output: Array<{ level: OutputLevel; body: string }>; error?: string; + /** + * Everything the script returned, in order. Absent when the plugin reported + * no return channel at all (older plugin build, or a run that never reached + * a return) — distinct from `[]`, which means it returned nothing. + */ + returnValues?: SerializedReturnValue[]; } export interface StateResult { diff --git a/tools/studio-bridge/src/commands/console/exec/execute.luau b/tools/studio-bridge/src/commands/console/exec/execute.luau index 734a588c2b9..e51e823095e 100644 --- a/tools/studio-bridge/src/commands/console/exec/execute.luau +++ b/tools/studio-bridge/src/commands/console/exec/execute.luau @@ -10,6 +10,9 @@ - Distinct error codes: SCRIPT_LOAD_ERROR, SCRIPT_RUNTIME_ERROR. - Sequential queueing: concurrent execute requests are processed one at a time in FIFO order. + - Return values: everything the script returns is marshalled onto + `payload.returnValues`, so a caller can read a result as a value + instead of scraping it back out of printed output. This module has no Roblox dependencies and is testable under Lune. ]] @@ -24,6 +27,108 @@ local _queue: { { payload: { [string]: any }, requestId: string?, sessionId: str {} local _processing = false +-- --------------------------------------------------------------------------- +-- Return value serialization +-- --------------------------------------------------------------------------- + +-- Produces the SerializedReturnValue shapes declared in +-- web-socket-protocol.ts. Action modules are loadstring'd standalone inside +-- the plugin and cannot require anything, so this cannot be shared with the +-- equivalent marshaller in query-data-model.luau. + +local MAX_DEPTH = 64 + +local function unsupported(typeName: string, text: string): { [string]: any } + return { type = "Unsupported", typeName = typeName, toString = text } +end + +local serializeValue: (value: any, depth: number, seen: { [any]: boolean }) -> any + +-- An array-like table becomes a JSON array, anything else a JSON object with +-- string keys. Mixed tables take the object branch on purpose: JSONEncode +-- rejects them, and a rejected encode drops the whole scriptComplete message. +local function serializeTable(value: { [any]: any }, depth: number, seen: { [any]: boolean }): any + local count = 0 + for _ in value do + count += 1 + end + + if count == #value then + local array = {} + for index = 1, count do + array[index] = serializeValue(value[index], depth + 1, seen) + end + return array + end + + local map: { [string]: any } = {} + for key, item in value do + map[if type(key) == "string" then key else tostring(key)] = serializeValue(item, depth + 1, seen) + end + return map +end + +function serializeValue(value: any, depth: number, seen: { [any]: boolean }): any + local valueType = typeof(value) + + if valueType == "nil" or valueType == "string" or valueType == "boolean" then + return value + elseif valueType == "number" then + -- inf and nan have no JSON spelling, and encoding one produces a + -- document the server cannot decode -- losing the entire message. + if value ~= value or value == math.huge or value == -math.huge then + return unsupported("number", tostring(value)) + end + return value + elseif valueType == "table" then + if depth > MAX_DEPTH then + return unsupported("table", "") + end + if seen[value] then + return unsupported("table", "") + end + seen[value] = true + local serialized = serializeTable(value, depth, seen) + seen[value] = nil + return serialized + elseif valueType == "Vector3" then + return { type = "Vector3", value = { value.X, value.Y, value.Z } } + elseif valueType == "Vector2" then + return { type = "Vector2", value = { value.X, value.Y } } + elseif valueType == "CFrame" then + return { type = "CFrame", value = { value:GetComponents() } } + elseif valueType == "Color3" then + return { type = "Color3", value = { value.R, value.G, value.B } } + elseif valueType == "UDim2" then + return { type = "UDim2", value = { value.X.Scale, value.X.Offset, value.Y.Scale, value.Y.Offset } } + elseif valueType == "UDim" then + return { type = "UDim", value = { value.Scale, value.Offset } } + elseif valueType == "BrickColor" then + return { type = "BrickColor", name = value.Name, value = value.Number } + elseif valueType == "EnumItem" then + return { type = "EnumItem", enum = tostring(value.EnumType), name = value.Name, value = value.Value } + elseif valueType == "Instance" then + return { type = "Instance", className = value.ClassName, path = value:GetFullName() } + else + return unsupported(valueType, tostring(value)) + end +end + +-- Marshal the values a script returned, in order. A returned nil becomes an +-- explicit marker: a JSON array cannot hold a hole, and dropping the entry +-- would shift every later value into the wrong position. +local function serializeReturnValues(packed: { n: number, [number]: any }): { any } + local returnValues = {} + for index = 2, packed.n do + local serialized = serializeValue(packed[index], 1, {}) + if serialized == nil then + serialized = { type = "Nil" } + end + returnValues[index - 1] = serialized + end + return returnValues +end + -- --------------------------------------------------------------------------- -- Core execution logic -- --------------------------------------------------------------------------- @@ -108,17 +213,27 @@ function ExecuteAction._handleExecute( originalWarn(...) end - local success, runtimeError = pcall(fn) + local returned = table.pack(pcall(fn)) + local success = returned[1] -- Restore originals env.print = originalPrint env.warn = originalWarn if not success then - return sendResult({ success = false, error = tostring(runtimeError), code = "SCRIPT_RUNTIME_ERROR", output = captured }) + return sendResult({ + success = false, + error = tostring(returned[2]), + code = "SCRIPT_RUNTIME_ERROR", + output = captured, + }) end - return sendResult({ success = true, output = captured }) + return sendResult({ + success = true, + output = captured, + returnValues = serializeReturnValues(returned), + }) end -- --------------------------------------------------------------------------- diff --git a/tools/studio-bridge/src/index.ts b/tools/studio-bridge/src/index.ts index dcde2a4af88..7408adf1852 100644 --- a/tools/studio-bridge/src/index.ts +++ b/tools/studio-bridge/src/index.ts @@ -55,6 +55,7 @@ export type { DataModelInstance, ErrorCode, SerializedValue, + SerializedReturnValue, } from './server/web-socket-protocol.js'; // Lower-level exports for advanced usage / testing diff --git a/tools/studio-bridge/src/server/studio-bridge-server.test.ts b/tools/studio-bridge/src/server/studio-bridge-server.test.ts index 61064734284..f4e6d16c57f 100644 --- a/tools/studio-bridge/src/server/studio-bridge-server.test.ts +++ b/tools/studio-bridge/src/server/studio-bridge-server.test.ts @@ -285,6 +285,55 @@ describe('StudioBridgeServer', () => { expect(result.logs).toContain('Script threw: boom'); }); + it('carries the script return values off the scriptComplete', async () => { + const ready = await createReadyServer(); + server = ready.server; + client = ready.client; + + const resultPromise = server.executeAsync({ + scriptContent: 'return { counts = { passed = 7 } }', + }); + + await new Promise((resolve) => { + client!.on('message', (raw) => { + const data = JSON.parse( + typeof raw === 'string' ? raw : raw.toString('utf-8') + ); + if (data.type === 'execute') resolve(); + }); + }); + + client!.send( + JSON.stringify({ + type: 'scriptComplete', + sessionId: ready.sessionId, + payload: { + success: true, + returnValues: [{ counts: { passed: 7 } }], + }, + }) + ); + + const result = await resultPromise; + expect(result.returnValues).toEqual([{ counts: { passed: 7 } }]); + }); + + it('leaves returnValues undefined when the run times out', async () => { + // Nothing reported a return channel, so the value is unknown rather than + // empty — a caller can still fall back to whatever output arrived. + const ready = await createReadyServer(); + server = ready.server; + client = ready.client; + + const result = await server.executeAsync({ + scriptContent: 'while true do end', + timeoutMs: 200, + }); + + expect(result.success).toBe(false); + expect(result.returnValues).toBeUndefined(); + }); + it('returns failure when client disconnects during execution', async () => { const ready = await createReadyServer(); server = ready.server; diff --git a/tools/studio-bridge/src/server/studio-bridge-server.ts b/tools/studio-bridge/src/server/studio-bridge-server.ts index 814b37a6e60..1e1824cec50 100644 --- a/tools/studio-bridge/src/server/studio-bridge-server.ts +++ b/tools/studio-bridge/src/server/studio-bridge-server.ts @@ -26,6 +26,7 @@ import { type Capability, type PluginMessage, type ServerMessage, + type SerializedReturnValue, encodeMessage, decodePluginMessage, } from './web-socket-protocol.js'; @@ -106,6 +107,14 @@ export interface ExecuteOptions { export interface StudioBridgeResult { success: boolean; logs: string; + /** + * Everything the script returned, in order, marshalled by the plugin. + * Absent whenever no `scriptComplete` carrying a return channel arrived — + * a timeout, a disconnect, a plugin error, or an older plugin build — which + * is what separates "we never learned what it returned" from `[]`, "it + * returned nothing". + */ + returnValues?: SerializedReturnValue[]; } type BridgeState = @@ -797,6 +806,7 @@ export class StudioBridgeServer { finish({ success: msg.payload.success, logs: logLines.join('\n'), + returnValues: msg.payload.returnValues, }); break; } diff --git a/tools/studio-bridge/src/server/web-socket-protocol.test.ts b/tools/studio-bridge/src/server/web-socket-protocol.test.ts index 853cf4e8016..21bda5b0ebe 100644 --- a/tools/studio-bridge/src/server/web-socket-protocol.test.ts +++ b/tools/studio-bridge/src/server/web-socket-protocol.test.ts @@ -47,6 +47,70 @@ describe('decodePluginMessage', () => { }); expect(msg).not.toHaveProperty('requestId'); }); + + it('carries returnValues through unchanged, nesting included', () => { + const msg = roundTripPlugin({ + type: 'scriptComplete', + sessionId: 'sess-1', + payload: { + success: true, + returnValues: [ + { slug: 'maid', counts: { passed: 1014, failed: 0 } }, + 'str', + 42, + true, + { type: 'Vector3', value: [1, 2, 3] }, + [1, 2], + ], + }, + }); + expect(msg?.type).toBe('scriptComplete'); + expect( + (msg as { payload: { returnValues?: unknown[] } }).payload.returnValues + ).toEqual([ + { slug: 'maid', counts: { passed: 1014, failed: 0 } }, + 'str', + 42, + true, + { type: 'Vector3', value: [1, 2, 3] }, + [1, 2], + ]); + }); + + it('leaves returnValues undefined when the plugin sent none', () => { + // Distinguishable from an empty array: nothing reported a return channel, + // so a caller can still fall back to the run's printed output. + const msg = roundTripPlugin({ + type: 'scriptComplete', + sessionId: 'sess-1', + payload: { success: true }, + }); + expect( + (msg as { payload: { returnValues?: unknown[] } }).payload.returnValues + ).toBeUndefined(); + }); + + it('preserves an empty returnValues array', () => { + const msg = roundTripPlugin({ + type: 'scriptComplete', + sessionId: 'sess-1', + payload: { success: true, returnValues: [] }, + }); + expect( + (msg as { payload: { returnValues?: unknown[] } }).payload.returnValues + ).toEqual([]); + }); + + it('ignores a returnValues field that is not an array', () => { + const msg = roundTripPlugin({ + type: 'scriptComplete', + sessionId: 'sess-1', + payload: { success: true, returnValues: 'nope' }, + }); + expect( + (msg as { payload: { returnValues?: unknown[] } }).payload.returnValues + ).toBeUndefined(); + }); }); describe('register', () => { diff --git a/tools/studio-bridge/src/server/web-socket-protocol.ts b/tools/studio-bridge/src/server/web-socket-protocol.ts index 99eb17a2fe0..4587b34e3da 100644 --- a/tools/studio-bridge/src/server/web-socket-protocol.ts +++ b/tools/studio-bridge/src/server/web-socket-protocol.ts @@ -69,6 +69,22 @@ export type SerializedValue = | { type: 'Instance'; className: string; path: string } | { type: 'Unsupported'; typeName: string; toString: string }; +/** + * A value returned by an executed script, marshalled for the wire: everything + * `SerializedValue` covers, plus tables, which the plugin walks recursively + * (array-like tables stay arrays, anything else becomes an object with string + * keys). + * + * `{ type: 'Nil' }` stands in for a `nil` return value: a JSON array cannot + * hold a hole, and dropping the entry would silently shift every value after + * it, misreporting which value was returned where. + */ +export type SerializedReturnValue = + | SerializedValue + | { type: 'Nil' } + | SerializedReturnValue[] + | { [key: string]: SerializedReturnValue }; + export interface DataModelInstance { name: string; className: string; @@ -99,6 +115,13 @@ export interface ScriptCompleteMessage extends BaseMessage { success: boolean; error?: string; output?: Array<{ level: string; body: string; timestamp: number }>; + /** + * Everything the script returned, in order, marshalled by the plugin. + * Absent when the plugin never reported a return channel (an older plugin + * build, or a script that failed before returning) — distinct from `[]`, + * which means the script ran and returned nothing. + */ + returnValues?: SerializedReturnValue[]; }; } @@ -354,6 +377,11 @@ export function decodePluginMessage(raw: string): PluginMessage | null { timestamp: typeof e.timestamp === 'number' ? e.timestamp : 0, })) : undefined; + // The values themselves are whatever the script returned, so they are + // carried through as-is; only the array wrapper is checked. + const returnValues = Array.isArray(payload.returnValues) + ? (payload.returnValues as SerializedReturnValue[]) + : undefined; return { type: 'scriptComplete', sessionId, @@ -363,6 +391,7 @@ export function decodePluginMessage(raw: string): PluginMessage | null { error: typeof payload.error === 'string' ? payload.error : undefined, output, + returnValues, }, }; } diff --git a/tools/studio-bridge/templates/studio-bridge-plugin-test/test/execute-handler.test.luau b/tools/studio-bridge/templates/studio-bridge-plugin-test/test/execute-handler.test.luau index 30c3e5eb169..9d7f96a97e5 100644 --- a/tools/studio-bridge/templates/studio-bridge-plugin-test/test/execute-handler.test.luau +++ b/tools/studio-bridge/templates/studio-bridge-plugin-test/test/execute-handler.test.luau @@ -403,4 +403,115 @@ table.insert(tests, { end, }) +-- =========================================================================== +-- Return value marshalling +-- =========================================================================== + +table.insert(tests, { + name = "returnValues: empty when the script returns nothing", + fn = function() + local result = ExecuteAction.handleExecute({ code = "local x = 1" }, "req-rv0", "sess-rv0") + assertNotNil(result.returnValues, "returnValues should be present on a successful run") + assertEqual(#result.returnValues, 0, "no values returned") + end, +}) + +table.insert(tests, { + name = "returnValues: carries every returned value in order", + fn = function() + local result = ExecuteAction.handleExecute({ code = "return 'str', 42, true" }, "req-rv1", "sess-rv1") + assertEqual(#result.returnValues, 3, "three values returned") + assertEqual(result.returnValues[1], "str") + assertEqual(result.returnValues[2], 42) + assertEqual(result.returnValues[3], true) + end, +}) + +table.insert(tests, { + name = "returnValues: nested tables survive as tables", + fn = function() + local result = ExecuteAction.handleExecute( + { code = "return { slug = 'maid', counts = { passed = 1014, failed = 0 } }" }, + "req-rv2", + "sess-rv2" + ) + assertEqual(#result.returnValues, 1, "one value returned") + local value = result.returnValues[1] + assertEqual(value.slug, "maid") + assertEqual(value.counts.passed, 1014) + assertEqual(value.counts.failed, 0) + end, +}) + +table.insert(tests, { + name = "returnValues: array-like tables stay arrays", + fn = function() + local result = ExecuteAction.handleExecute({ code = "return { 10, 20, 30 }" }, "req-rv3", "sess-rv3") + local value = result.returnValues[1] + assertEqual(#value, 3, "array length") + assertEqual(value[1], 10) + assertEqual(value[3], 30) + end, +}) + +table.insert(tests, { + name = "returnValues: non-string keys become strings so the table can be encoded", + fn = function() + local result = ExecuteAction.handleExecute({ code = "return { [1] = 'a', name = 'b' }" }, "req-rv4", "sess-rv4") + local value = result.returnValues[1] + assertEqual(value["1"], "a", "numeric key stringified") + assertEqual(value.name, "b") + end, +}) + +table.insert(tests, { + name = "returnValues: a returned nil keeps its position", + fn = function() + local result = ExecuteAction.handleExecute({ code = "return nil, 7" }, "req-rv5", "sess-rv5") + assertEqual(#result.returnValues, 2, "two values returned") + assertEqual(result.returnValues[1].type, "Nil", "nil marker") + assertEqual(result.returnValues[2], 7) + end, +}) + +table.insert(tests, { + name = "returnValues: a cycle is reported instead of recursing forever", + fn = function() + local result = + ExecuteAction.handleExecute({ code = "local t = {}; t.self = t; return t" }, "req-rv6", "sess-rv6") + local value = result.returnValues[1] + assertEqual(value.self.type, "Unsupported", "cycle is marked unsupported") + assertContains(value.self.toString, "cycle") + end, +}) + +table.insert(tests, { + name = "returnValues: non-finite numbers are marked, not emitted as invalid JSON", + fn = function() + local result = ExecuteAction.handleExecute({ code = "return math.huge, 0 / 0" }, "req-rv7", "sess-rv7") + assertEqual(result.returnValues[1].type, "Unsupported", "inf is marked") + assertEqual(result.returnValues[1].typeName, "number") + assertEqual(result.returnValues[2].type, "Unsupported", "nan is marked") + end, +}) + +table.insert(tests, { + name = "returnValues: functions are marked unsupported rather than dropped", + fn = function() + local result = ExecuteAction.handleExecute({ code = "return function() end" }, "req-rv8", "sess-rv8") + assertEqual(#result.returnValues, 1, "one value returned") + assertEqual(result.returnValues[1].type, "Unsupported") + assertEqual(result.returnValues[1].typeName, "function") + end, +}) + +table.insert(tests, { + name = "returnValues: absent on a failed run", + fn = function() + local result = ExecuteAction.handleExecute({ code = "error('boom')" }, "req-rv9", "sess-rv9") + assertFalse(result.success) + assertNil(result.returnValues, "a failed run reports no return values") + end, +}) + return tests