fix: prevent exceptions from manual test #53
Conversation
📝 WalkthroughWalkthroughNull checks were added for the private field Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://app.smartcontext.ai/default/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Editor/Services/TestRunnerService.cs (1)
170-171: Race condition:_resultsis not thread-safe
TestFinishedis called from Unity’s runner thread, while consumers may await theExecuteTestsAsynctask on a different thread. Concurrent writes/reads to_resultswithout synchronisation can cause data races orInvalidOperationExceptionduring enumeration inBuildResultJson.If you expect heavy parallelism, wrap the list access in a lock or switch to a thread-safe collection (e.g.
ConcurrentBag<ITestResultAdaptor>). At minimum, document the thread-affinity assumption.
| if (_tcs == null) | ||
| return; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Good defensive guard, but remember to reset _tcs after the run
Early-returning when _tcs is null prevents the NRE, ✔️.
However, once RunFinished completes _tcs keeps the completed TaskCompletionSource.
If the same TestRunnerService instance later triggers Unity-initiated callbacks (e.g. during a manual run, or while you’re only retrieving the test list) the stale _tcs reference will no longer be null, so the callbacks will start accumulating results into a stale list and may attempt to TrySetResult again, leading to:
InvalidOperationException: TaskCompletionSource<T> can only be set once- Incorrect aggregation of results from multiple, unrelated runs.
A minimal fix is to clear the field when the run is over:
@@
var summary = BuildResultJson(_results, result);
_tcs?.TrySetResult(summary);
+ // Prevent stale state on subsequent, unrelated callbacks
+ _tcs = null;Optionally, you may also null-out _results for symmetry.
Also applies to: 167-169, 178-180
🤖 Prompt for AI Agents
In Editor/Services/TestRunnerService.cs around lines 148 to 150, after the early
return when _tcs is null, add code to reset _tcs to null once the run finishes
to avoid reusing a completed TaskCompletionSource and causing
InvalidOperationException or incorrect result aggregation. Similarly, apply this
reset pattern to lines 167-169 and 178-180. Optionally, also reset the _results
field to null for consistency after the run completes.
There was a problem hiding this comment.
Good finding and great fix
Approving the PR and merging
thank you @yulcat
Description
This PR fixes an issue where exceptions could occur during manual test execution. The fix adds null checks for the
_tcs(Task Completion Source) field in three key methods of theTestRunnerServiceclass:RunStarted- Checks if_tcsis null before proceedingTestFinished- Adds null check before adding resultsRunFinished- Ensures_tcsis not null before building and setting the resultThese changes prevent potential
NullReferenceExceptions that could occur when test callbacks are triggered but no task is waiting for the results, which can happen during manual test execution.Changes that Break Backward Compatibility
N/A
Documentation
N/A
Created with Palmier
Summary by CodeRabbit