[UUM-138957] Remove runtime test dependency on UnityEditor APIs#664
[UUM-138957] Remove runtime test dependency on UnityEditor APIs#664lopezt-unity wants to merge 11 commits intomasterfrom
Conversation
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR introduces significant improvements for runtime testing, but there are a few critical issues that need to be addressed:
- Logic Error: A logic error in submesh index comparison in
RuntimeUtilitywill lead to incorrect test results. - Build Failure Risk: Including
UnityEditor.TestRunnerin a runtime assembly definition will cause build failures on standalone platforms. - Resource Loading: The resource path resolution relies on
StackTrace, which is unreliable in non-development runtime builds. - Memory Management: Several tests destroy components instead of GameObjects, leading to object leaks in the test runner.
🤖 Helpful? 👍/👎
| int vertexCount = expected.vertexCount; | ||
| int subMeshCount = expected.subMeshCount; | ||
|
|
||
| Assert.AreEqual(vertexCount, result.vertexCount, expected.name + " != " + result.name + " (submesh count)"); |
There was a problem hiding this comment.
In AssertAreEqual, the assertion message for the vertexCount check incorrectly identifies it as a "submesh count" failure. This will make it difficult to debug test failures.
| Assert.AreEqual(vertexCount, result.vertexCount, expected.name + " != " + result.name + " (submesh count)"); | |
| Assert.AreEqual(vertexCount, result.vertexCount, expected.name + " != " + result.name + " (vertex count)"); |
🤖 Helpful? 👍/👎
| for (int i = 0; i < subMeshCount; i++) | ||
| { | ||
| uint indexCount = expected.GetIndexCount(i); | ||
|
|
||
| Assert.AreEqual(expected.GetTopology(i), result.GetTopology(i)); | ||
| Assert.AreEqual(indexCount, result.GetIndexCount(i)); | ||
|
|
||
| expected.GetIndices(leftIndices, i); | ||
| result.GetIndices(rightIndices, i); | ||
|
|
||
| for (int n = 0; n < indexCount; n++) | ||
| Assert.AreEqual(leftIndices[n], rightIndices[n], message); | ||
| } |
There was a problem hiding this comment.
There is a logic error in how submesh indices are compared. The leftIndices and rightIndices lists are declared outside the subMeshCount loop (lines 189-190) and mesh.GetIndices(list, submesh) in Unity appends to the list rather than clearing it.
Consequently, for any submesh after the first (i > 0), the lists will contain indices from all previous submeshes, and the inner loop (line 202) will re-validate the indices of the first submesh rather than the current one. The lists should be cleared at the start of each iteration.
| for (int i = 0; i < subMeshCount; i++) | |
| { | |
| uint indexCount = expected.GetIndexCount(i); | |
| Assert.AreEqual(expected.GetTopology(i), result.GetTopology(i)); | |
| Assert.AreEqual(indexCount, result.GetIndexCount(i)); | |
| expected.GetIndices(leftIndices, i); | |
| result.GetIndices(rightIndices, i); | |
| for (int n = 0; n < indexCount; n++) | |
| Assert.AreEqual(leftIndices[n], rightIndices[n], message); | |
| } | |
| for (int i = 0; i < subMeshCount; i++) | |
| { | |
| uint indexCount = expected.GetIndexCount(i); | |
| Assert.AreEqual(expected.GetTopology(i), result.GetTopology(i)); | |
| Assert.AreEqual(indexCount, result.GetIndexCount(i)); | |
| leftIndices.Clear(); | |
| rightIndices.Clear(); | |
| expected.GetIndices(leftIndices, i); | |
| result.GetIndices(rightIndices, i); | |
| for (int n = 0; n < (int)indexCount; n++) | |
| Assert.AreEqual(leftIndices[n], rightIndices[n], message); | |
| } |
🤖 Helpful? 👍/👎
| } | ||
|
|
||
| // Get the calling file path relative to the `Tests/` directory | ||
| string fullFilePath = Path.GetFullPath(filePath).Replace("\\", "/"); |
There was a problem hiding this comment.
The variable fullFilePath is assigned but never used. Additionally, there are several lines of commented-out code (lines 225-227) that should likely be removed to keep the utility class maintainable.
🤖 Helpful? 👍/👎
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## master #664 +/- ##
==========================================
+ Coverage 36.05% 37.02% +0.96%
==========================================
Files 277 277
Lines 34909 34970 +61
==========================================
+ Hits 12588 12948 +360
+ Misses 22321 22022 -299
Flags with carried forward coverage won't be shown. Click here to find out more. |
2 participants
Purpose of this PR
The PR changes the way Framework and Runtime tests are working in ProBuilder.
Previously these supposedly runtime tests where using UnityEditor APIs, which was failing on some trunk tests.
For more context:
Two probuilder test assembly definitions (Unity.ProBuilder.Tests.Framework.asmdef and Unity.ProBuilder.Tests.asmdef) had "includePlatforms": [] (all platforms) despite using UnityEditor.* APIs (AssetDatabase, EditorSceneManager, PackageManager.PackageInfo). These editor-only APIs are unavailable to runtime assemblies, causing CS0234/CS0103 compilation errors, that's why I wanted to merge those changes in my PR. By changing both "includePlatforms": ["Editor"] to match their actual dependencies, consistent with the existing Unity.ProBuilder.Editor.Tests.asmdef , the jobs no longer fail.
However, runtime tests should not be relying on Editor APIs.
This PR update the runtime tests to get rid of the different Editor APIs used in it.
Most changes are:
Tests.FrameworkanymoreTestUtility.cs(inTests.Framework) have been moved toRuntimeUtility.csTemplatefolder to aResourcesone so that they can be loaded directly usingResources.Load()RuntimeUtility.cshave been updated accordinglyLinks
Jira: https://jira.unity3d.com/browse/UUM-138957
Comments to Reviewers
All the tests have be re-run in the Editor (Editor and Playmode) and are all 🟢