feat(util): add parallel execution support to JAction + UI test coverage #608
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add JActionExecutionContext for per-execution state isolation - Add JActionExecution result struct with per-execution Cancelled state - Add JActionExecutionHandle for per-execution cancellation control - Execute/ExecuteAsync now snapshot tasks enabling safe parallel execution - Switch from ValueTask to UniTask for better Unity integration - Add comprehensive tests for parallel execution, timeouts, and edge cases - Add MessageBox test hooks for pool state inspection - Update jaction skill documentation with new API and patterns Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: JasonXuDeveloper - 傑 <jason@xgamedev.net>
Address CodeQL warnings about potential missed Dispose() calls by wrapping test code in try-finally blocks. Signed-off-by: JasonXuDeveloper - 傑 <jason@xgamedev.net>
- Fix Task.CompletedTask -> UniTask.CompletedTask - Fix AsUniTask to delegate to awaiter (prevent double pool return) - Fix Reset to return execution contexts to pool before clearing - Fix MessageBox test hooks to use direct index access (avoid foreach) - Add comment explaining PlayerLoop single-threading in parallel test Signed-off-by: JasonXuDeveloper - 傑 <jason@xgamedev.net>
Convert try-finally patterns to 'using' declarations for cleaner resource management as suggested by CodeQL static analysis. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: JasonXuDeveloper - 傑 <jason@xgamedev.net>
HashSet does not support array indexing. Use LINQ First() method to get the first element from the collection. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: JasonXuDeveloper - 傑 <jason@xgamedev.net>
Add 22 new test files covering all UI Editor components: - Button: JButton, JButtonGroup, JIconButton, JToggleButton - Form: JDropdown, JFormField, JObjectField, JTextField, JToggle - Layout: JCard, JRow, JSection, JStack - Feedback: JLogView, JProgressBar, JStatusBar - Navigation: JBreadcrumb - Theming: JTheme, Tokens - Utilities: EnumHelpers, StyleSheetManager - Base: JComponent Coverage improved from 14% to 61.7% for UI.Editor package. Uses reflection for button click testing with graceful fallback. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
github-actions
bot
added
documentation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| Assert.AreEqual(11, counter); | ||
|
|
||
| action.Dispose(); |
Check warning
Code scanning / CodeQL
Dispose may not be called if an exception is thrown during execution Warning
Unity Test Results✅ EditMode: All tests passed Unity Version: 2022.3.55f1 ✅ All tests passed! The PR is ready for review. View workflow run |
There was a problem hiding this comment.
Pull request overview
Adds parallel/concurrent execution support to JAction (with per-execution handles/results) and substantially expands UI Editor test coverage, alongside code coverage configuration updates.
Changes:
- Introduces per-execution context/handle/result types to support concurrent
JAction.ExecuteAsync()runs and per-execution cancellation. - Adds extensive EditMode unit tests for UI Editor components/utilities and updates test assemblies.
- Updates Unity Code Coverage settings to include key JEngine assemblies.
Reviewed changes
Copilot reviewed 72 out of 72 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| UnityProject/ProjectSettings/Packages/com.unity.testtools.codecoverage/Settings.json | Enables/configures code coverage and scopes it to JEngine assemblies. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.util/Runtime/JEngine.Util.asmdef | Adds UniTask dependency to the Util runtime assembly. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.util/Runtime/JAction.cs | Core JAction changes to support parallel execution and per-execution handles/contexts. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.util/Runtime/JActionAwaitable.cs | Refactors awaitable to be context-based (and updates awaiter behavior). |
| UnityProject/Packages/com.jasonxudeveloper.jengine.util/Runtime/JActionExecutionHandle.cs | Adds per-execution handle API (awaitable + cancellable). |
| UnityProject/Packages/com.jasonxudeveloper.jengine.util/Runtime/JActionExecutionHandle.cs.meta | Unity metadata for the new runtime file. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.util/Runtime/JActionExecution.cs | Adds per-execution result object to capture cancellation state. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.util/Runtime/JActionExecution.cs.meta | Unity metadata for the new runtime file. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.util/Runtime/Internal/JActionRunner.cs | Updates runner to drive per-execution contexts instead of per-action execution. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.util/Runtime/Internal/JActionExecutionContext.cs | Introduces pooled per-execution state machine enabling parallel execution. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.util/Runtime/Internal/JActionExecutionContext.cs.meta | Unity metadata for the new internal runtime file. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.util/Tests/Runtime/JEngine.Util.Tests.asmdef | Updates runtime test assembly references (now references JEngine.Util and UniTask). |
| UnityProject/Packages/com.jasonxudeveloper.jengine.util/Tests/Editor/JEngine.Util.Editor.Tests.asmdef | Updates editor test assembly references (now references JEngine.Util and UniTask). |
| UnityProject/Packages/com.jasonxudeveloper.jengine.util/Tests/Runtime/JActionRuntimeTests.cs | Adds/extends PlayMode tests for async/parallel execution and cancellation behaviors. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Runtime/MessageBox.cs | Adds test hooks for inspecting/simulating MessageBox behavior under UNITY_INCLUDE_TESTS. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/MessageBoxTests.cs | Expands EditMode tests around MessageBox pool state, null/empty input handling, and concurrency. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/JEngine.UI.Editor.Tests.asmdef | Adds JEngine.UI.Editor reference to the UI Editor test assembly. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Utilities/StyleSheetManagerTests.cs | New tests for stylesheet caching/application. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Utilities/StyleSheetManagerTests.cs.meta | Unity metadata for new test file. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Utilities/EnumHelpersTests.cs | New tests for enum helper utilities. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Utilities/EnumHelpersTests.cs.meta | Unity metadata for new test file. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Utilities.meta | Unity folder metadata for Utilities tests. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Theming/TokensTests.cs.meta | Unity metadata for new theming tests. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Theming/JThemeTests.cs.meta | Unity metadata for new theming tests. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Theming.meta | Unity folder metadata for Theming tests. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Navigation/JBreadcrumbTests.cs | New tests for breadcrumb navigation component. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Navigation/JBreadcrumbTests.cs.meta | Unity metadata for new test file. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Navigation.meta | Unity folder metadata for Navigation tests. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Layout/JStackTests.cs | New tests for stack layout component. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Layout/JStackTests.cs.meta | Unity metadata for new test file. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Layout/JSectionTests.cs | New tests for section layout component. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Layout/JSectionTests.cs.meta | Unity metadata for new test file. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Layout/JRowTests.cs | New tests for row layout component. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Layout/JRowTests.cs.meta | Unity metadata for new test file. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Layout/JCardTests.cs | New tests for card layout component. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Layout/JCardTests.cs.meta | Unity metadata for new test file. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Layout.meta | Unity folder metadata for Layout tests. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Form/JToggleTests.cs | New tests for toggle form component. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Form/JToggleTests.cs.meta | Unity metadata for new test file. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Form/JTextFieldTests.cs | New tests for text field form component. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Form/JTextFieldTests.cs.meta | Unity metadata for new test file. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Form/JObjectFieldTests.cs | New tests for generic Unity object field wrapper. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Form/JObjectFieldTests.cs.meta | Unity metadata for new test file. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Form/JFormFieldTests.cs | New tests for form field layout wrapper. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Form/JFormFieldTests.cs.meta | Unity metadata for new test file. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Form/JDropdownTests.cs | New tests for dropdown (including generic/enum helpers). |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Form/JDropdownTests.cs.meta | Unity metadata for new test file. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Form.meta | Unity folder metadata for Form tests. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Feedback/JStatusBarTests.cs | New tests for status bar component. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Feedback/JStatusBarTests.cs.meta | Unity metadata for new test file. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Feedback/JProgressBarTests.cs | New tests for progress bar component. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Feedback/JProgressBarTests.cs.meta | Unity metadata for new test file. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Feedback/JLogViewTests.cs | New tests for log view component. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Feedback/JLogViewTests.cs.meta | Unity metadata for new test file. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Feedback.meta | Unity folder metadata for Feedback tests. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Button/JToggleButtonTests.cs | New tests for toggle button component. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Button/JToggleButtonTests.cs.meta | Unity metadata for new test file. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Button/JIconButtonTests.cs | New tests for icon button component (incl. reflection-based click simulation). |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Button/JIconButtonTests.cs.meta | Unity metadata for new test file. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Button/JButtonTests.cs | New tests for button component (variants, styling, click simulation). |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Button/JButtonTests.cs.meta | Unity metadata for new test file. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Button/JButtonGroupTests.cs | New tests for button group component. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Button/JButtonGroupTests.cs.meta | Unity metadata for new test file. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Button.meta | Unity folder metadata for Button tests. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Base/JComponentTests.cs | New tests for base component fluent API behavior. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Base/JComponentTests.cs.meta | Unity metadata for new test file. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components/Base.meta | Unity folder metadata for Base tests. |
| UnityProject/Packages/com.jasonxudeveloper.jengine.ui/Tests/Editor/Components.meta | Unity folder metadata for Components tests. |
| .claude-plugin/skills/jaction/SKILL.md | Updates internal documentation to include parallel execution and new handle/result types. |
Comments suppressed due to low confidence (1)
UnityProject/Packages/com.jasonxudeveloper.jengine.util/Runtime/JAction.cs:949
Dispose()callsCancel()and then immediately returns theJActionto the pool. With parallel executions, this can return task state storages/contexts to pools while handles are still awaiting completion, and it can also cancel other in-flight executions unexpectedly. Consider makingDispose()defer pooling until all active contexts have completed (e.g., set a pending-dispose flag and onlyPool.Return(this)when_activeContextsis empty and_syncContextis not executing).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+56
to
+63
| /// <summary> | ||
| /// Disposes the underlying JAction, returning it to the pool. | ||
| /// </summary> | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public void Dispose() | ||
| { | ||
| Action?.Dispose(); | ||
| } |
There was a problem hiding this comment.
JActionExecution.Dispose() disposes the underlying shared JAction. In parallel mode, disposing the result of one execution can cancel other concurrent executions of the same JAction (since JAction.Dispose() calls Cancel() when executing). Consider either removing IDisposable from JActionExecution, making Dispose() a no-op (or warning) when Action.Executing is true, or implementing reference-counting/pending-dispose so the action is only pooled once all executions have finished.
Comment on lines
+95
to
+101
| /// <summary> | ||
| /// Disposes the underlying JAction, returning it to the pool. | ||
| /// </summary> | ||
| public void Dispose() | ||
| { | ||
| _action?.Dispose(); | ||
| } |
There was a problem hiding this comment.
JActionExecutionHandle.Dispose() disposes the underlying JAction even if this handle's execution is still running. In parallel mode (or when other handles exist), this can cancel unrelated executions and/or return the action to the pool while other handles still reference it. Safer options are: (1) have handle disposal cancel only this execution context and not dispose the action, or (2) only dispose the action when it is no longer executing (or via a pending-dispose mechanism on JAction).
Comment on lines
24
to
25
| /// </remarks> | ||
| public readonly struct JActionAwaitable |
There was a problem hiding this comment.
The XML remarks just above this struct still state that JAction.ExecuteAsync() returns ValueTask<JAction>, but it now returns JActionExecutionHandle. Please update the documentation to match the current API and recommended usage patterns, otherwise consumers will be misled.
Comment on lines
+139
to
+143
| /// Test hook: Simulates clicking a button on the most recently shown message box. | ||
| /// </summary> | ||
| /// <param name="clickOk">If true, simulates clicking OK; otherwise simulates clicking Cancel.</param> | ||
| /// <returns>True if a message box was found and the click was simulated.</returns> | ||
| internal static bool TestSimulateButtonClick(bool clickOk) |
There was a problem hiding this comment.
The comment/doc says this simulates clicking a button on the "most recently shown" message box, but ActiveMessageBoxes is a HashSet, so there is no defined ordering. Either adjust the documentation to say "an arbitrary active message box", or track the last shown instance explicitly if ordering matters for tests.
Comment on lines
4
to
+8
| "references": [ | ||
| "GUID:5c8e1f4d7a3b9e2c6f0d8a4b7e3c1f9d", | ||
| "JEngine.Util", | ||
| "UnityEngine.TestRunner", | ||
| "UnityEditor.TestRunner" | ||
| "UnityEditor.TestRunner", | ||
| "UniTask" |
There was a problem hiding this comment.
JEngine.Util.Tests.asmdef (Runtime tests) references UnityEditor.TestRunner, which can make the test assembly Editor-only and potentially break running PlayMode tests in a player build. If these are PlayMode/runtime tests, consider removing the UnityEditor.TestRunner reference (keep UnityEngine.TestRunner + nunit.framework.dll).
Comment on lines
+298
to
+302
| if (task.FloatParam1 > 0 && _repeatLastTime > 0) | ||
| { | ||
| if (currentTime - _repeatLastTime < task.FloatParam1) | ||
| return false; | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
Comment on lines
+332
to
+336
| if (task.FloatParam1 > 0 && _repeatLastTime > 0) | ||
| { | ||
| if (currentTime - _repeatLastTime < task.FloatParam1) | ||
| return false; | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
Comment on lines
+380
to
+384
| if (task.FloatParam1 > 0 && _repeatLastTime > 0) | ||
| { | ||
| if (currentTime - _repeatLastTime < task.FloatParam1) | ||
| return false; | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
Comment on lines
+416
to
+420
| if (task.FloatParam1 > 0 && _repeatLastTime > 0) | ||
| { | ||
| if (currentTime - _repeatLastTime < task.FloatParam1) | ||
| return false; | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
Comment on lines
+445
to
+449
| if (task.FloatParam1 > 0 && _repeatCounter > 0) | ||
| { | ||
| if (currentTime - _repeatLastTime < task.FloatParam1) | ||
| return false; | ||
| } |
There was a problem hiding this comment.
These 'if' statements can be combined.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
JAction.Parallel()andJAction.ParallelAsync()Changes
JEngine.Util
JAction.Parallel()andJAction.ParallelAsync()methods for concurrent executionJActionExecutionHandlefor tracking parallel execution resultsusingstatements instead of try-finally for cleaner codeJEngine.UI.Editor Tests (NEW)
JButtonTests.cs- 40+ tests for button variants, styling, chainingJIconButtonTests.cs- Tests for icon button sizing and tooltipsJToggleButtonTests.cs- Tests for toggle state and callbacksJButtonGroupTests.cs- Tests for button groupingJDropdownTests.cs- Tests for dropdown with generics and enumsJTextFieldTests.cs- Tests for text field validationJToggleTests.cs- Tests for toggle componentJObjectFieldTests.cs- Tests for Unity object fieldsJFormFieldTests.cs- Tests for form field layoutJStackTests.cs- Tests for stack layoutJCardTests.cs- Tests for card componentJRowTests.cs- Tests for row layoutJSectionTests.cs- Tests for section componentJProgressBarTests.cs- Tests for progress barJStatusBarTests.cs- Tests for status barJLogViewTests.cs- Tests for log viewJBreadcrumbTests.cs- Tests for breadcrumb navigationJThemeTests.cs- Tests for theming systemTokensTests.cs- Tests for design tokensEnumHelpersTests.cs- Tests for enum utilitiesStyleSheetManagerTests.cs- Tests for stylesheet managementJComponentTests.cs- Tests for base componentTest Coverage
Test plan
🤖 Generated with Claude Code