Release 1.0.0 #28
Release 1.0.0 #28
Conversation
- Added *AGENTS.md* document to help guide AI coding assistants to understand and work with this package library - Added an entire test suit of unit/integration/performance/smoke tests to cover all the code for all services in this package **Changed**: - Changed *VersionServices* namespace from *GameLovers* to *GameLovers.Services* to maintain consistency with other services in the package. - Made *CallOnSpawned*, *CallOnSpawned\<TData\>*, and *CallOnDespawned* methods virtual in *ObjectPoolBase\<T\>* to allow derived pool classes to customize lifecycle callback behavior. **Fixed**: - Fixed the *README.md* file to now follow best practices in OSS standards for Unity's package library projects - Fixed linter warnings in *VersionServices.cs* (redundant field initialization, unused lambda parameter, member shadowing) - Fixed *GameObjectPool* not invoking *IPoolEntitySpawn.OnSpawn()* and *IPoolEntityDespawn.OnDespawn()* on components attached to spawned GameObjects.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
WalkthroughThe changes include enhancements across various files such as introducing new documents, optimizing methods for better customization and performance, adding unit and performance tests, and refining variable initialization and event handling. Changes
Uplevel your code reviews with CodeRabbit ProCodeRabbit ProIf you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. Moreover, CodeRabbit Pro is free for open source projects. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| protected virtual void CallOnSpawned(T entity) | ||
| { | ||
| var poolEntity = entity as IPoolEntitySpawn; | ||
|
|
||
| poolEntity?.OnSpawn(); | ||
| } | ||
|
|
||
| protected void CallOnSpawned<TData>(T entity, TData data) | ||
| protected virtual void CallOnSpawned<TData>(T entity, TData data) | ||
| { | ||
| var poolEntity = entity as IPoolEntitySpawn<TData>; | ||
|
|
||
| poolEntity?.OnSpawn(data); | ||
| } | ||
|
|
||
| protected void CallOnDespawned(T entity) | ||
| protected virtual void CallOnDespawned(T entity) |
There was a problem hiding this comment.
- The methods
CallOnSpawned,CallOnSpawned<TData>, andCallOnDespawnedhave been changed to beprotected virtualinstead of justprotected. This change allows derived pool classes to customize the lifecycle callback behavior, which is a good design decision for extensibility.
| /// <inheritdoc /> | ||
| protected override void CallOnSpawned(GameObject entity) | ||
| { | ||
| var poolEntity = entity.GetComponent<IPoolEntitySpawn>(); | ||
|
|
||
| poolEntity?.OnSpawn(); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| protected override void CallOnSpawned<TData>(GameObject entity, TData data) | ||
| { | ||
| var poolEntity = entity.GetComponent<IPoolEntitySpawn<TData>>(); | ||
|
|
||
| poolEntity?.OnSpawn(data); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| protected override void CallOnDespawned(GameObject entity) | ||
| { | ||
| var poolEntity = entity.GetComponent<IPoolEntityDespawn>(); | ||
|
|
||
| poolEntity?.OnDespawn(); | ||
| } |
There was a problem hiding this comment.
- Similar changes have been made for methods related to
GameObjectentities. TheCallOnSpawned,CallOnSpawned<TData>, andCallOnDespawnedmethods are nowprotected override, enabling customization of lifecycle callbacks forGameObjectentities. - Additionally, a new method
PostDespawnEntityhas been added to handle post-despawn actions like deactivating the entity, which is a good practice for managing object states efficiently.
| /// <inheritdoc /> | ||
| protected override void CallOnSpawned(T entity) | ||
| { | ||
| var poolEntity = entity.GetComponent<IPoolEntitySpawn>(); | ||
|
|
||
| poolEntity?.OnSpawn(); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| protected override void CallOnSpawned<TData>(T entity, TData data) | ||
| { | ||
| var poolEntity = entity.GetComponent<IPoolEntitySpawn<TData>>(); | ||
|
|
||
| poolEntity?.OnSpawn(data); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| protected override void CallOnDespawned(T entity) | ||
| { | ||
| var poolEntity = entity.GetComponent<IPoolEntityDespawn>(); | ||
|
|
||
| poolEntity?.OnDespawn(); | ||
| } | ||
|
|
There was a problem hiding this comment.
- The same changes as above have been applied to generic type
Tentities, ensuring consistency in handling lifecycle callbacks for different types of entities. - The addition of the
PostDespawnEntitymethod for generic entities follows the same pattern as forGameObjectentities, promoting code consistency.
Suggestions
No issues found in the provided code snippets.
| ### Key Features | ||
|
|
There was a problem hiding this comment.
Consider optimizing the TickService class for safe iteration by using a foreach loop instead of directly iterating over the list. This can improve readability and potentially avoid issues related to modifying the collection during iteration.
- foreach (var tickable in _tickables)
+ foreach (var tickable in _tickables.ToList())GameObjectPool.cs
|
|
||
| | Problem | Solution | | ||
| |---------|----------| |
There was a problem hiding this comment.
When adding virtual methods for customization, ensure that the base class provides a default implementation to avoid forcing derived classes to implement unnecessary behavior.
+ protected virtual void CallOnSpawned(T obj) { }
+ protected virtual void CallOnDespawned(T obj) { }README.md
| **Made with ❤️ for the Unity community** | ||
|
|
||
| *If this package helps your project, consider giving it a star ⭐ on GitHub!* | ||
| *If this package helps your project, please consider giving it a ⭐ on GitHub!* |
There was a problem hiding this comment.
Consider updating the message to encourage users to star the repository on GitHub for support and recognition.
*If this package helps your project, please consider giving it a ⭐ on GitHub!*Overall
The code changes look mostly good with minor suggestions for optimization and consistency. Ensure to address the mentioned points to enhance performance, maintainability, and consistency across the codebase.
| public struct TestMessage : IMessage {} | ||
|
|
||
| [Test, Performance] | ||
| public void Publish_100Subscribers_MeasureTime() |
There was a problem hiding this comment.
Consider adding comments to explain the purpose of the loop.
+ // Loop to subscribe mock subscribers for testing purposes
| { | ||
| var sub = new MockSubscriber(); | ||
| broker.Subscribe<TestMessage>(sub.OnMessage); | ||
| } |
There was a problem hiding this comment.
Ensure proper cleanup after each test to avoid side effects on subsequent tests.
+ // Consider unsubscribing mock subscribers after each test to maintain test independence
| public void PublishSafe_MeasureAllocations() | ||
| { | ||
| var broker = new MessageBrokerService(); | ||
| for (var i = 0; i < 100; i++) | ||
| { | ||
| var sub = new MockSubscriber(); | ||
| broker.Subscribe<TestMessage>(sub.OnMessage); | ||
| } | ||
|
|
||
| Measure.Method(() => broker.PublishSafe(new TestMessage())) | ||
| .GC() | ||
| .WarmupCount(10) | ||
| .MeasurementCount(100) | ||
| .Run(); | ||
| } |
There was a problem hiding this comment.
It's crucial to ensure that the performance tests are isolated and do not have side effects on memory allocations. Consider resetting or cleaning up resources between measurements.
+ // Add resource cleanup or reset mechanism between measurements to prevent interference
| /// Call <see cref="Dispose"/> to clear the Tick Service correctly. | ||
| /// </summary> | ||
| public interface ITickService | ||
| public interface ITickService : IDisposable |
There was a problem hiding this comment.
The ITickService interface should inherit from IDisposable as mentioned in the description.
- public interface ITickService
+ public interface ITickService : IDisposableIssue in new hunk:
|
|
||
| /// <inheritdoc cref="ITickService"/> | ||
| public class TickService : ITickService, IDisposable | ||
| public class TickService : ITickService |
There was a problem hiding this comment.
The TickService class should implement IDisposable as per the description.
- public class TickService : ITickService
+ public class TickService : ITickService, IDisposableIssue in new hunk:
| // Iterate backwards to allow safe mutation during iteration | ||
| for (int i = _onFixedUpdateList.Count - 1; i >= 0; i--) | ||
| { | ||
| arrayCopy[i].Action(Time.fixedTime); | ||
| // Skip if the item was removed by a previous action | ||
| if (i >= _onFixedUpdateList.Count) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| _onFixedUpdateList[i].Action(Time.fixedTime); |
There was a problem hiding this comment.
The iteration logic has changed significantly. The new implementation iterates backwards to allow safe mutation during iteration. However, there is a potential issue with the condition if (i >= _onFixedUpdateList.Count) which might lead to an out-of-bounds access. This condition needs to be revised to ensure it doesn't exceed the list size.
- var arrayCopy = _onFixedUpdateList.ToArray();
- for (int i = 0; i < arrayCopy.Length; i++)
+ for (int i = _onFixedUpdateList.Count - 1; i >= 0; i--)Issue in new hunk:
| return; | ||
| } | ||
|
|
||
| var arrayCopy = list.ToArray(); | ||
|
|
||
| for (int i = 0; i < arrayCopy.Length; i++) | ||
| // Iterate backwards to allow safe mutation during iteration | ||
| for (int i = list.Count - 1; i >= 0; i--) | ||
| { | ||
| var tickData = arrayCopy[i]; | ||
| // Skip if the item was removed by a previous action | ||
| if (i >= list.Count) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| var tickData = list[i]; | ||
| var time = tickData.RealTime ? Time.realtimeSinceStartup : Time.time; | ||
|
|
||
| if (time < tickData.LastTickTime + tickData.DeltaTime) |
There was a problem hiding this comment.
Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [334-360]
Similar to the previous iteration logic, the new implementation also iterates backwards for safe mutation. However, the condition if (i >= list.Count) needs to be adjusted to prevent out-of-bounds access.
- var arrayCopy = list.ToArray();
- for (int i = 0; i < arrayCopy.Length; i++)
+ for (int i = list.Count - 1; i >= 0; i--)Issue in new hunk:
|
|
||
| tickData.Action(deltaTime); | ||
|
|
||
| // Check if the update was not unsubscribed in the call | ||
| var index = i - (arrayCopy.Length - countBefore); | ||
| if (list.Count > index && tickData == list[index]) | ||
| // Check if the item still exists and wasn't unsubscribed | ||
| if (i < list.Count && list[i] == tickData) | ||
| { | ||
| var overFlow = tickData.DeltaTime == 0 ? 0 : deltaTime % tickData.DeltaTime; | ||
|
|
||
| tickData.LastTickTime = tickData.TimeOverflowToNextTick ? time - overFlow : time; | ||
|
|
||
| list[index] = tickData; | ||
| list[i] = tickData; |
There was a problem hiding this comment.
The handling of the loop and updating the tickData.LastTickTime seems to have changed. Ensure that the logic for updating LastTickTime and checking if the item still exists is correctly implemented to avoid any issues related to data consistency.
No specific fix provided due to the complexity of changes.
---end_change_section---
| [Test, Performance] | ||
| public void ObjectPool_SpawnDespawn_1000Cycles() | ||
| { | ||
| var pool = new ObjectPool<MockEntity>(1000, () => new MockEntity()); | ||
| var entities = new MockEntity[1000]; | ||
|
|
||
| Measure.Method(() => | ||
| { | ||
| for (var i = 0; i < 1000; i++) entities[i] = pool.Spawn(); | ||
| for (var i = 0; i < 1000; i++) pool.Despawn(entities[i]); | ||
| }) |
There was a problem hiding this comment.
The performance test for spawning and despawning entities in the ObjectPool class looks good. However, to further enhance the performance testing, consider adding assertions to validate the behavior of the pool after the cycles. This can help ensure that the pool is functioning correctly under load.
+ // Add assertions to validate pool state after spawn and despawn cycles
+ var activeCount = pool.ActiveCount;
+ var inactiveCount = pool.InactiveCount;
+ Assert.AreEqual(0, activeCount, "Active count should be 0 after despawning all entities");
+ Assert.AreEqual(1000, inactiveCount, "Inactive count should be 1000 after despawning all entities");
| public interface IInterface {} | ||
| public class Implementation : IInterface {} | ||
| public interface IDisposableInterface : IDisposable {} | ||
| public class DisposableImplementation : IDisposableInterface | ||
| { | ||
| public void Dispose() {} | ||
| } |
There was a problem hiding this comment.
Consider extracting the DisposableImplementation class into a separate file for better modularity and organization.
| { | ||
| MainInstaller.Clean(); | ||
| } | ||
|
|
There was a problem hiding this comment.
The Cleanup method should be marked as static since it does not access instance members.
| using System; | ||
| using GameLovers.Services; |
There was a problem hiding this comment.
The using directive for System seems unnecessary and can be removed.
- using System;
| namespace GameLovers.Services | ||
| { | ||
| /// <summary> |
There was a problem hiding this comment.
The namespace change from GameLovers to GameLovers.Services seems inconsistent with the provided context. Ensure that this change aligns with the overall project structure and organization.
- namespace GameLovers
+ namespace GameLovers.ServicesInconsistent Field Name in VersionData Struct
| public string CommitHash; | ||
| public string BranchName; | ||
| public string BuildType; |
There was a problem hiding this comment.
There is a discrepancy in the field name CommitHash in the VersionData struct compared to the old hunk where it was named Commit. Make sure the field names are consistent throughout the codebase for clarity and maintainability.
- public string Commit;
+ public string CommitHash;Initialization of _loaded Field
55:
The _loaded field initialization seems to be missing in the new hunk. It's essential to ensure proper initialization of variables to avoid unexpected behavior.
+ private static bool _loaded;Event Handler Assignment
69:
The event handler assignment for the request.completed event seems to have changed syntax. Verify that the event handling logic is correctly updated to prevent any unintended side effects.
- request.completed += operation => source.SetResult(request.asset as TextAsset);
+ request.completed += _ => source.SetResult(request.asset as TextAsset);Version Formatting Logic
| /// </summary> | ||
| public static string FormatInternalVersion(VersionData data) | ||
| { | ||
| string version = $"{Application.version}-{data.BuildNumber}.{data.BranchName}.{data.Commit}"; | ||
| var version = $"{Application.version}-{data.BuildNumber}.{data.BranchName}.{data.CommitHash}"; | ||
|
|
||
| if (!string.IsNullOrEmpty(data.BuildType)) | ||
| { |
There was a problem hiding this comment.
Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [127-131]
The version formatting logic has been modified. Ensure that the version string is constructed accurately based on the provided data fields to maintain consistency and correctness.
- string version = $"{Application.version}-{data.BuildNumber}.{data.BranchName}.{data.Commit}";
+ var version = $"{Application.version}-{data.BuildNumber}.{data.BranchName}.{data.CommitHash}";
| // Create and save run info to PlayerPrefs (required by Performance Testing Package in EditMode) | ||
| // Note: RunSettings is internal to the package, but only Run metadata is required to prevent | ||
| // the NullReferenceException in Metadata.SetRuntimeSettings() |
There was a problem hiding this comment.
Consider adding error handling for potential exceptions during PlayerPrefs saving.
+ try
+ {
SaveToPrefs(run, PlayerPrefKeyRunJSON);
+ }
+ catch (Exception e)
+ {
+ Debug.LogError($"Error saving performance test metadata to PlayerPrefs: {e.Message}");
+ }
| private static Unity.PerformanceTesting.Data.Editor GetEditorInfo() | ||
| { | ||
| var fullVersion = UnityEditorInternal.InternalEditorUtility.GetFullUnityVersion(); | ||
| const string pattern = @"(.+\.+.+\.\w+)|((?<=\().+(?=\)))"; | ||
| var matches = Regex.Matches(fullVersion, pattern); | ||
|
|
||
| return new Unity.PerformanceTesting.Data.Editor | ||
| { | ||
| Branch = GetEditorBranch(), | ||
| Version = matches.Count > 0 ? matches[0].Value : "unknown", | ||
| Changeset = matches.Count > 1 ? matches[1].Value : "unknown", | ||
| Date = UnityEditorInternal.InternalEditorUtility.GetUnityVersionDate(), | ||
| }; | ||
| } | ||
|
|
||
| private static string GetEditorBranch() | ||
| { | ||
| foreach (var method in typeof(UnityEditorInternal.InternalEditorUtility).GetMethods()) | ||
| { | ||
| if (method.Name.Contains("GetUnityBuildBranch")) | ||
| { | ||
| return (string)method.Invoke(null, null); | ||
| } | ||
| } | ||
| return "null"; | ||
| } |
There was a problem hiding this comment.
The method GetEditorBranch() could be simplified by directly returning the result of the method invocation.
- foreach (var method in typeof(UnityEditorInternal.InternalEditorUtility).GetMethods())
- {
- if (method.Name.Contains("GetUnityBuildBranch"))
- {
- return (string)method.Invoke(null, null);
- }
- }
- return "null";
+ var method = typeof(UnityEditorInternal.InternalEditorUtility).GetMethod("GetUnityBuildBranch");
+ return method?.Invoke(null, null)?.ToString() ?? "null";
| private static List<string> GetPackageDependencies() | ||
| { | ||
| var packages = UnityEditor.PackageManager.PackageInfo.GetAllRegisteredPackages(); | ||
| return packages.Select(p => $"{p.name}@{p.version}").ToList(); | ||
| } | ||
|
|
||
| private static void SetBuildSettings(Run run) | ||
| { | ||
| run.Player.GpuSkinning = PlayerSettings.gpuSkinning; | ||
| run.Player.ScriptingBackend = PlayerSettings | ||
| .GetScriptingBackend(UnityEditor.Build.NamedBuildTarget.FromBuildTargetGroup(EditorUserBuildSettings.selectedBuildTargetGroup)) | ||
| .ToString(); | ||
| run.Player.RenderThreadingMode = PlayerSettings.graphicsJobs | ||
| ? PlayerSettings.graphicsJobMode.ToString() | ||
| : PlayerSettings.MTRendering ? "MultiThreaded" : "SingleThreaded"; | ||
| run.Player.AndroidTargetSdkVersion = PlayerSettings.Android.targetSdkVersion.ToString(); | ||
| run.Player.AndroidBuildSystem = EditorUserBuildSettings.androidBuildSystem.ToString(); | ||
| run.Player.BuildTarget = EditorUserBuildSettings.activeBuildTarget.ToString(); | ||
| run.Player.StereoRenderingPath = PlayerSettings.stereoRenderingPath.ToString(); |
There was a problem hiding this comment.
The method SetBuildSettings() can be optimized by extracting repetitive code into separate methods for better readability and maintainability.
+ private static void SetPlayerSettings(Run run)
+ {
+ run.Player.GpuSkinning = PlayerSettings.gpuSkinning;
+ run.Player.AndroidTargetSdkVersion = PlayerSettings.Android.targetSdkVersion.ToString();
+ run.Player.AndroidBuildSystem = EditorUserBuildSettings.androidBuildSystem.ToString();
+ run.Player.BuildTarget = EditorUserBuildSettings.activeBuildTarget.ToString();
+ run.Player.StereoRenderingPath = PlayerSettings.stereoRenderingPath.ToString();
+ }
+
+ private static void SetScriptingBackend(Run run)
+ {
+ run.Player.ScriptingBackend = PlayerSettings.GetScriptingBackend(UnityEditor.Build.NamedBuildTarget.FromBuildTargetGroup(EditorUserBuildSettings.selectedBuildTargetGroup)).ToString();
+ }
+
+ private static void SetRenderThreadingMode(Run run)
+ {
+ run.Player.RenderThreadingMode = PlayerSettings.graphicsJobs ? PlayerSettings.graphicsJobMode.ToString() : PlayerSettings.MTRendering ? "MultiThreaded" : "SingleThreaded";
+ }
+
+ private static void SetBuildSettings(Run run)
+ {
+ SetPlayerSettings(run);
+ SetScriptingBackend(run);
+ SetRenderThreadingMode(run);
+ }
| private static long ConvertToUnixTimestamp(DateTime date) | ||
| { | ||
| var origin = new DateTime(1970, 1, 1, 0, 0, 0, 0, DateTimeKind.Utc); | ||
| var diff = date.ToUniversalTime() - origin; | ||
| return (long)Math.Floor(diff.TotalSeconds); |
There was a problem hiding this comment.
Consider using TimeSpan.TotalSeconds instead of Math.Floor(diff.TotalSeconds) for converting to Unix timestamp.
- return (long)Math.Floor(diff.TotalSeconds);
+ return (long)diff.TotalSeconds;End of Review
| public void AddData_Successfully() | ||
| { | ||
| var data = Substitute.For<IDataMockup>(); | ||
|
|
||
| _dataService.AddOrReplaceData(data); | ||
|
|
||
| Assert.AreSame(data, _dataService.GetData<IDataMockup>()); |
There was a problem hiding this comment.
The test AddData_Successfully seems to be testing the behavior correctly. However, consider adding a teardown method to clean up after each test to ensure independence between tests.
[Test]
public void AddData_Successfully()
{
var data = Substitute.For<IDataMockup>();
_dataService.AddOrReplaceData(data);
Assert.AreSame(data, _dataService.GetData<IDataMockup>());
}
[TearDown]
public void TearDown()
{
PlayerPrefs.DeleteAll();
}
| public void DateTime_Convertions_Successfully() | ||
| { | ||
| Assert.GreaterOrEqual(ErrorValue, (_timeService.DateTimeUtcFromUnityTime(_timeService.UnityTimeNow) - _timeService.DateTimeUtcNow).TotalMilliseconds); | ||
| Assert.GreaterOrEqual(ErrorValue, (_timeService.DateTimeUtcFromUnixTime(_timeService.UnixTimeNow) - _timeService.DateTimeUtcNow).TotalMilliseconds); | ||
| } |
There was a problem hiding this comment.
In the DateTime_Convertions_Successfully test, consider breaking down the assertions into separate tests for better test granularity and readability.
| { | ||
| Assert.GreaterOrEqual(ErrorValue, _timeService.UnityTimeFromDateTimeUtc(_timeService.DateTimeUtcNow) - _timeService.UnityTimeNow); | ||
| Assert.GreaterOrEqual(ErrorValue, _timeService.UnityTimeFromUnixTime(_timeService.UnixTimeNow) - _timeService.UnityTimeNow); | ||
| } |
There was a problem hiding this comment.
Similarly, in the UnityTime_Convertions_Successfully test, split the assertions into individual tests for clarity and easier debugging.
| public void UnixTime_Convertions_Successfully() | ||
| { | ||
| Assert.GreaterOrEqual(ErrorValue, _timeService.UnixTimeFromDateTimeUtc(_timeService.DateTimeUtcNow) - _timeService.UnixTimeNow); | ||
| Assert.GreaterOrEqual(ErrorValue, _timeService.UnixTimeFromUnityTime(_timeService.UnityTimeNow) - _timeService.UnixTimeNow); | ||
| } |
There was a problem hiding this comment.
For the UnixTime_Convertions_Successfully test, it's advisable to separate each assertion into its own test method to isolate failures.
| public void AddTime_AllTimeTypes_Successfully() | ||
| { | ||
| var extraTime = 50.5f; | ||
| var extraTimeInMilliseconds = TimeSpan.FromSeconds(extraTime).TotalMilliseconds; | ||
| var dateTime = _timeService.DateTimeUtcNow; | ||
| var unityTime = _timeService.UnityTimeNow; | ||
| var unixTime = _timeService.UnixTimeNow; | ||
|
|
||
| _timeService.AddTime(extraTime); | ||
|
|
||
| Assert.LessOrEqual(0, _timeService.DateTimeUtcNow.CompareTo(dateTime.AddSeconds(extraTime))); | ||
| Assert.GreaterOrEqual(_timeService.UnityTimeNow, unityTime + extraTime); | ||
| Assert.GreaterOrEqual(_timeService.UnixTimeNow, unixTime - extraTimeInMilliseconds); | ||
| } |
There was a problem hiding this comment.
The AddTime_AllTimeTypes_Successfully test is doing multiple assertions within a single test. It's recommended to split these assertions into separate test methods to improve test maintainability and readability.
| public void AddTime_NegativeValue_SubtractsTime() | ||
| { | ||
| var initialUnityTime = _timeService.UnityTimeNow; | ||
| var negativeTime = -10f; | ||
|
|
||
| _timeService.AddTime(negativeTime); | ||
|
|
||
| Assert.Less(_timeService.UnityTimeNow, initialUnityTime); | ||
| Assert.That(_timeService.UnityTimeNow, Is.EqualTo(initialUnityTime + negativeTime).Within(ErrorValue)); | ||
| } |
There was a problem hiding this comment.
In the AddTime_NegativeValue_SubtractsTime test, consider breaking down the assertions into distinct test cases to enhance test isolation and clarity.
| public void SetInitialTime_ResetsTimeBase() | ||
| { | ||
| // SetInitialTime acts as a "reset" by synchronizing the time base | ||
| var customInitialTime = new DateTime(2025, 1, 1, 12, 0, 0, DateTimeKind.Utc); | ||
|
|
||
| _timeService.SetInitialTime(customInitialTime); | ||
|
|
||
| // After setting initial time, DateTimeUtcNow should be close to the custom time | ||
| // (plus any time that has passed since realtimeSinceStartup was captured) | ||
| var now = _timeService.DateTimeUtcNow; | ||
|
|
||
| // The difference should be very small (just the time since SetInitialTime was called) | ||
| Assert.That((now - customInitialTime).TotalSeconds, Is.LessThan(1.0)); | ||
| } |
There was a problem hiding this comment.
The SetInitialTime_ResetsTimeBase test contains comments that explain the purpose of the test. While comments are helpful, consider refactoring them into descriptive test method names to make the intent clearer without relying solely on comments.
Overall Review
The test class TimeServiceTest contains multiple test methods with grouped assertions. To enhance test readability, maintainability, and isolation, it's recommended to refactor the tests by splitting the grouped assertions into separate test methods. Additionally, consider using more descriptive test method names to convey the test scenarios effectively.
| { | ||
| public class MockPoolEntity : MonoBehaviour, IPoolEntitySpawn, IPoolEntityDespawn | ||
| { | ||
| public int SpawnCount; | ||
| public int DespawnCount; | ||
|
|
||
| public void OnSpawn() => SpawnCount++; | ||
| public void OnDespawn() => DespawnCount++; | ||
| } | ||
|
|
||
| private GameObject _sample; | ||
| private GameObjectPool _pool; | ||
|
|
||
| [SetUp] | ||
| public void Init() | ||
| { | ||
| _sample = new GameObject("Sample"); | ||
| _sample.AddComponent<MockPoolEntity>(); | ||
| _sample.SetActive(false); | ||
| _pool = new GameObjectPool(0, _sample); | ||
| } | ||
|
|
||
| [TearDown] | ||
| public void Cleanup() | ||
| { | ||
| _pool.Dispose(true); | ||
| if (_sample != null) Object.Destroy(_sample); | ||
| } |
There was a problem hiding this comment.
The setup and teardown methods in the test class are well-structured. However, there is a potential issue with object destruction timing in the Dispose_DestroysAllInstances test case. As noted in the comment, object destruction is delayed until the end of the frame or the next frame. This can lead to assertions failing due to the object still being present during the test execution.
+ // Consider waiting for one more frame before asserting instance destruction
| [UnityTest] | ||
| public IEnumerator Spawn_InstantiatesPrefab() | ||
| { | ||
| var instance = _pool.Spawn(); | ||
|
|
||
| Assert.IsNotNull(instance); | ||
| Assert.AreNotSame(_sample, instance); | ||
| Assert.IsTrue(instance.activeSelf); | ||
|
|
||
| yield return null; | ||
| } | ||
|
|
||
| [UnityTest] | ||
| public IEnumerator Despawn_DeactivatesGameObject() | ||
| { | ||
| var instance = _pool.Spawn(); | ||
| _pool.Despawn(instance); | ||
|
|
||
| Assert.IsFalse(instance.activeSelf); | ||
|
|
||
| yield return null; | ||
| } | ||
|
|
||
| [UnityTest] | ||
| public IEnumerator Spawn_InvokesIPoolEntitySpawn() | ||
| { | ||
| var instance = _pool.Spawn(); | ||
| var mock = instance.GetComponent<MockPoolEntity>(); | ||
|
|
||
| Assert.AreEqual(1, mock.SpawnCount); | ||
|
|
||
| _pool.Despawn(instance); | ||
| Assert.AreEqual(1, mock.DespawnCount); | ||
|
|
||
| yield return null; | ||
| } | ||
|
|
||
| [UnityTest] | ||
| public IEnumerator Dispose_DestroysAllInstances() | ||
| { | ||
| var instance = _pool.Spawn(); | ||
| _pool.Dispose(); | ||
|
|
||
| // Note: Object destruction is delayed until end of frame or next frame | ||
| yield return null; | ||
|
|
||
| Assert.IsTrue(instance == null); | ||
| } | ||
|
|
||
| [UnityTest] | ||
| public IEnumerator Dispose_WithSampleDestroy_DestroysSample() | ||
| { | ||
| _pool.Dispose(true); | ||
|
|
||
| yield return null; | ||
|
|
||
| Assert.IsTrue(_sample == null); | ||
| } |
There was a problem hiding this comment.
The test cases cover various aspects of the GameObjectPool functionality effectively. However, it's crucial to ensure that the tests are deterministic and do not rely on timing or external factors that may introduce flakiness.
+ // Ensure tests are deterministic and not affected by timing issuesEnd of Review
| var entity = _poolService.Spawn<IMockPoolableEntity>(); | ||
|
|
||
| Assert.AreEqual(entity,_poolService.Spawn<IMockPoolableEntity>()); | ||
| _pool.Received().Spawn(); | ||
| Assert.IsNotNull(entity); | ||
| Assert.IsInstanceOf<MockPoolableEntity>(entity); |
There was a problem hiding this comment.
The new implementation directly spawns an entity from _poolService, which is a more straightforward approach compared to using a substitute. This change simplifies the test setup and assertion.
- var entity = Substitute.For<IMockPoolableEntity>();
- _pool.Spawn().Returns(entity);
-
- Assert.AreEqual(entity, _poolService.Spawn<IMockPoolableEntity>());
- _pool.Received().Spawn();
+ var entity = _poolService.Spawn<IMockPoolableEntity>();
+
+ Assert.IsNotNull(entity);
+ Assert.IsInstanceOf<MockPoolableEntity>(entity);
| var entity = _poolService.Spawn<IMockPoolableEntity>(); | ||
|
|
||
| _poolService.Despawn(entity); | ||
| _pool.Received().Despawn(entity); | ||
| Assert.DoesNotThrow(() => _poolService.Despawn(entity)); |
There was a problem hiding this comment.
In the new implementation, the test now directly spawns an entity from _poolService and asserts that despawning the entity does not throw an exception. This change simplifies the test logic.
- var entity = Substitute.For<IMockPoolableEntity>();
-
- _poolService.Despawn(entity);
-
- _pool.Received().Despawn(entity);
+ var entity = _poolService.Spawn<IMockPoolableEntity>();
+
+ Assert.DoesNotThrow(() => _poolService.Despawn(entity));
| public void Despawn_NotAddedPool_ThrowsException() | ||
| { | ||
| var entity = Substitute.For<IMockPoolableEntity>(); | ||
| var entity = new MockPoolableEntity(); |
There was a problem hiding this comment.
The new test creates a new MockPoolableEntity directly instead of using a substitute. This change simplifies the test setup and aligns with the direct usage of entities in the pool service.
- var entity = Substitute.For<IMockPoolableEntity>();
-
- _poolService = new PoolService();
+ var entity = new MockPoolableEntity();
+
+ _poolService = new PoolService();
| _poolService.Spawn<IMockPoolableEntity>(); | ||
| _poolService.DespawnAll<IMockPoolableEntity>(); | ||
| _pool.Received().DespawnAll(); | ||
| Assert.DoesNotThrow(() => _poolService.DespawnAll<IMockPoolableEntity>()); |
There was a problem hiding this comment.
The updated test now directly spawns an entity and tests the DespawnAll method on _poolService. This change simplifies the test by removing unnecessary interactions with the pool object.
- _poolService.DespawnAll<IMockPoolableEntity>();
-
- _pool.Received().DespawnAll();
+ _poolService.Spawn<IMockPoolableEntity>();
+ _poolService.DespawnAll<IMockPoolableEntity>();
+
+ Assert.DoesNotThrow(() => _poolService.DespawnAll<IMockPoolableEntity>());
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Threading; | ||
| using GameLovers.Services; | ||
| using NSubstitute; | ||
| using NUnit.Framework; |
There was a problem hiding this comment.
Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [106-109]
The new test ensures that calling RemovePool on _poolService does not throw an exception. This test verifies the removal functionality directly without involving the pool object, which is a good practice for unit testing.
+ Assert.DoesNotThrow(() => _poolService.RemovePool<IMockPoolableEntity>());
| public IEnumerator Update_1000Subscribers_MeasureFrameTime() | ||
| { | ||
| var tickService = new TickService(); | ||
| for (var i = 0; i < 1000; i++) | ||
| { | ||
| tickService.SubscribeOnUpdate(dt => {}); | ||
| } | ||
|
|
||
| yield return Measure.Frames() | ||
| .WarmupCount(10) | ||
| .MeasurementCount(100) | ||
| .Run(); | ||
|
|
||
| tickService.Dispose(); |
There was a problem hiding this comment.
The performance test Update_1000Subscribers_MeasureFrameTime seems to be subscribing 1000 times within a loop. This could potentially impact performance due to the high number of subscriptions. Consider optimizing this logic to avoid unnecessary subscriptions.
| [Test, Performance] | ||
| public void Subscribe_Unsubscribe_Churn_MeasureTime() | ||
| { | ||
| var tickService = new TickService(); | ||
| System.Action<float>[] actions = new System.Action<float>[100]; | ||
| for (int i = 0; i < 100; i++) actions[i] = dt => {}; | ||
|
|
||
| Measure.Method(() => | ||
| { | ||
| for (var i = 0; i < 100; i++) tickService.SubscribeOnUpdate(actions[i]); | ||
| for (var i = 0; i < 100; i++) tickService.UnsubscribeOnUpdate(actions[i]); | ||
| }) | ||
| .WarmupCount(5) | ||
| .MeasurementCount(20) | ||
| .Run(); | ||
|
|
||
| tickService.Dispose(); | ||
| } |
There was a problem hiding this comment.
In the Subscribe_Unsubscribe_Churn_MeasureTime test, there is a loop subscribing and unsubscribing actions. While this churn test is valid, ensure that the actions being subscribed and unsubscribed are unique to simulate real-world scenarios accurately. Also, consider adding assertions to validate the behavior of the TickService.
+ // Consider adding assertions to validate behavior
+ Assert.AreEqual(expectedResult, actualResult);
| public IEnumerator Update_1000Subscribers_MeasureFrameTime() | ||
| { | ||
| var tickService = new TickService(); | ||
| for (var i = 0; i < 1000; i++) | ||
| { | ||
| tickService.SubscribeOnUpdate(dt => {}); | ||
| } | ||
|
|
||
| yield return Measure.Frames() | ||
| .WarmupCount(10) | ||
| .MeasurementCount(100) | ||
| .Run(); | ||
|
|
||
| tickService.Dispose(); |
There was a problem hiding this comment.
The performance test Update_1000Subscribers_MeasureFrameTime seems to be subscribing 1000 times within a loop. This could potentially impact performance due to the high number of subscriptions. Consider optimizing this logic to avoid unnecessary subscriptions.
- for (var i = 0; i < 1000; i++)
+ for (var i = 0; i < 1000; i+=10) // Example: Subscribe in batches of 10
| [Test, Performance] | ||
| public void Subscribe_Unsubscribe_Churn_MeasureTime() | ||
| { | ||
| var tickService = new TickService(); | ||
| System.Action<float>[] actions = new System.Action<float>[100]; | ||
| for (int i = 0; i < 100; i++) actions[i] = dt => {}; | ||
|
|
||
| Measure.Method(() => | ||
| { | ||
| for (var i = 0; i < 100; i++) tickService.SubscribeOnUpdate(actions[i]); | ||
| for (var i = 0; i < 100; i++) tickService.UnsubscribeOnUpdate(actions[i]); | ||
| }) | ||
| .WarmupCount(5) | ||
| .MeasurementCount(20) | ||
| .Run(); | ||
|
|
||
| tickService.Dispose(); | ||
| } |
There was a problem hiding this comment.
In the Subscribe_Unsubscribe_Churn_MeasureTime test, there is a loop subscribing and unsubscribing actions. While this churn test is valid, ensure that the actions being subscribed and unsubscribed are unique to simulate real-world scenarios accurately. Also, consider adding assertions to validate the behavior of the TickService.
+ // Consider adding assertions to validate behavior
+ Assert.AreEqual(expectedResult, actualResult);
| public IEnumerator SubscribeOnUpdate_ReceivesDeltaTime() | ||
| { | ||
| float receivedDelta = -1f; | ||
| _tickService.SubscribeOnUpdate(dt => receivedDelta = dt); | ||
|
|
||
| yield return null; // Wait for next frame | ||
| yield return null; // Wait one more to be sure | ||
|
|
||
| Assert.GreaterOrEqual(receivedDelta, 0f); |
There was a problem hiding this comment.
Consider adding a timeout mechanism to prevent the test from hanging indefinitely in case of unexpected behavior.
| public IEnumerator SubscribeOnUpdate_TimeOverflow_CarriesOverflow() | ||
| { | ||
| float interval = 0.05f; | ||
| int callCount = 0; | ||
| _tickService.SubscribeOnUpdate(dt => callCount++, interval, true); | ||
|
|
||
| yield return new WaitForSeconds(interval * 2.5f); | ||
|
|
||
| // If overflow is carried, it should have triggered at least twice | ||
| Assert.GreaterOrEqual(callCount, 2); | ||
| } |
There was a problem hiding this comment.
Ensure that the time-based tests are resilient to potential variations in execution time, especially on different machines or under varying loads.
| [UnityTest] | ||
| public IEnumerator SubscribeOnUpdate_RealTime_UsesUnscaledTime() | ||
| { | ||
| float initialTimeScale = Time.timeScale; | ||
| Time.timeScale = 0f; | ||
|
|
||
| float receivedDelta = -1f; | ||
| _tickService.SubscribeOnUpdate(dt => receivedDelta = dt, 0f, false, true); | ||
|
|
||
| yield return new WaitForSecondsRealtime(0.1f); | ||
|
|
||
| Time.timeScale = initialTimeScale; | ||
|
|
||
| Assert.Greater(receivedDelta, 0f); | ||
| } |
There was a problem hiding this comment.
The test involving real-time and unscaled time should be carefully reviewed to handle edge cases where time might not progress as expected due to system limitations.
| [UnityTest] | ||
| public IEnumerator UnsubscribeOnUpdate_DuringCallback_SafelyRemoves() | ||
| { | ||
| int callCount = 0; | ||
| System.Action<float> action = null; | ||
| action = dt => | ||
| { | ||
| callCount++; | ||
| _tickService.UnsubscribeOnUpdate(action); | ||
| }; | ||
|
|
||
| _tickService.SubscribeOnUpdate(action); | ||
|
|
||
| yield return null; | ||
| yield return null; | ||
|
|
||
| Assert.AreEqual(1, callCount); | ||
| } |
There was a problem hiding this comment.
The test for unsubscribing during a callback should be expanded to cover scenarios where multiple subscribers are involved to ensure robustness.
| public IEnumerator Dispose_DestroysGameObject() | ||
| { | ||
| var initialCount = Object.FindObjectsByType<TickServiceMonoBehaviour>(FindObjectsSortMode.None).Length; | ||
| var tickService = new TickService(); | ||
|
|
||
| Assert.AreEqual(initialCount + 1, Object.FindObjectsByType<TickServiceMonoBehaviour>(FindObjectsSortMode.None).Length); | ||
|
|
||
| tickService.Dispose(); | ||
| yield return null; // Allow Destroy to complete | ||
|
|
||
| Assert.AreEqual(initialCount, Object.FindObjectsByType<TickServiceMonoBehaviour>(FindObjectsSortMode.None).Length); | ||
| } |
There was a problem hiding this comment.
It's recommended to verify the disposal of GameObjects more thoroughly by checking if the disposed objects are actually destroyed in Unity's scene hierarchy.
| public void MultipleInstances_CreateMultipleGameObjects() | ||
| { | ||
| // Note: The service doesn't enforce singleton, but it throws if _tickObject is already set | ||
| // However, _tickObject is an instance field in the current implementation. | ||
| // Wait, I saw a check in the constructor: | ||
| /* | ||
| public TickService() | ||
| { | ||
| if (_tickObject != null) | ||
| { | ||
| throw new InvalidOperationException("The tick service is being initialized for the second time and that is not valid"); | ||
| } | ||
| ... | ||
| } | ||
| */ | ||
| // But _tickObject is private readonly TickServiceMonoBehaviour _tickObject; | ||
| // So it's always null for a new instance. The check seems to be intended for a static field but isn't. | ||
|
|
||
| var service1 = new TickService(); | ||
| var service2 = new TickService(); | ||
|
|
||
| var objects = Object.FindObjectsByType<TickServiceMonoBehaviour>(FindObjectsSortMode.None); | ||
| Assert.GreaterOrEqual(objects.Length, 2); | ||
|
|
||
| service1.Dispose(); | ||
| service2.Dispose(); | ||
| } |
There was a problem hiding this comment.
The test for creating multiple instances should be revised to accurately reflect the intended behavior regarding singleton enforcement and object creation.
This review provides insights into potential improvements and areas to consider for enhancing the reliability and effectiveness of the test suite.
| public void AllServices_Instantiate_WithoutException() | ||
| { | ||
| Assert.DoesNotThrow(() => new MessageBrokerService()); | ||
| Assert.DoesNotThrow(() => new PoolService()); | ||
| Assert.DoesNotThrow(() => new DataService()); | ||
| Assert.DoesNotThrow(() => new TimeService()); | ||
| Assert.DoesNotThrow(() => new RngService(RngService.CreateRngData(0))); |
There was a problem hiding this comment.
Consider consolidating the instantiation tests for services into a parameterized test to reduce duplication and improve maintainability.
[Test]
public void Services_Instantiate_WithoutException([ValueSource(typeof(ServiceSource), "All")] Type serviceType)
{
Assert.DoesNotThrow(() => Activator.CreateInstance(serviceType));
}
| public void TickService_CreatesGameObject() | ||
| { | ||
| var service = new TickService(); | ||
| var go = GameObject.Find("TickServiceMonoBehaviour"); | ||
| Assert.IsNotNull(go); | ||
| service.Dispose(); |
There was a problem hiding this comment.
Ensure proper cleanup of GameObject instances created during tests to avoid potential memory leaks or clutter in the scene.
[Test]
public void TickService_CreatesGameObject()
{
var service = new TickService();
var go = GameObject.Find("TickServiceMonoBehaviour");
Assert.IsNotNull(go);
service.Dispose();
GameObject.Destroy(go); // Add this line to properly clean up the GameObject.
}
| [Test] | ||
| public void CoroutineService_CreatesGameObject() | ||
| { | ||
| var service = new CoroutineService(); | ||
| var go = GameObject.Find("CoroutineServiceMonoBehaviour"); | ||
| Assert.IsNotNull(go); | ||
| service.Dispose(); |
There was a problem hiding this comment.
Apply the same cleanup pattern as above to the CoroutineService test to ensure proper disposal of resources.
| [Test] | ||
| public void MainInstaller_BindResolve_Works() | ||
| { | ||
| var broker = new MessageBrokerService(); | ||
| MainInstaller.Bind<IMessageBrokerService>(broker); | ||
| Assert.AreSame(broker, MainInstaller.Resolve<IMessageBrokerService>()); | ||
| } |
There was a problem hiding this comment.
Consider refactoring the MainInstaller_BindResolve_Works test to use Arrange-Act-Assert (AAA) pattern for better readability and maintainability.
[Test]
public void MainInstaller_BindResolve_Works()
{
// Arrange
var broker = new MessageBrokerService();
// Act
MainInstaller.Bind<IMessageBrokerService>(broker);
// Assert
Assert.AreSame(broker, MainInstaller.Resolve<IMessageBrokerService>());
}
New:
Changed:
Fixed:
Summary by CodeRabbit