FIX: Change type aliasing to use the type implementing static interface instead of using derived type. #2265
Conversation
…ce instead of using derived type.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent |
![u-pr-agent[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
|
|
||
| #pragma warning disable CS0649 | ||
| [SuppressMessage("ReSharper", "AccessToStaticMemberViaDerivedType")] | ||
| using Is = NUnit.Framework.Is; |
There was a problem hiding this comment.
Suggestion: Changing the alias for Is from UnityEngine.TestTools.Constraints.Is to the base NUnit.Framework.Is may cause build errors. The derived type contains Unity-specific constraints like Is.AllocatingGCMemory. Please verify that no tests in this file use such constraints, as they will no longer be accessible through the Is alias. [possible issue, importance: 7]
| using Is = NUnit.Framework.Is; | |
| // If Unity-specific constraints are used, they must be fully qualified, e.g.: | |
| // Assert.That(myCode, UnityEngine.TestTools.Constraints.Is.AllocatingGCMemory()); | |
| // Or, for better readability, introduce a separate alias: | |
| using Is = NUnit.Framework.Is; | |
| using UnityIs = UnityEngine.TestTools.Constraints.Is; |
🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr-agent
There was a problem hiding this comment.
Yes, but that is a very specific use case that is still supported since the Unity extension is based on the same class being aliased here. Not using the base is considered bad practise by e.g. JetBrains Rider.
There was a problem hiding this comment.
UnityEngine.TestTools.Constraints just need a using statement to make it accessible.
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop #2265 +/- ##
===========================================
+ Coverage 76.70% 76.81% +0.10%
===========================================
Files 465 476 +11
Lines 87919 88645 +726
===========================================
+ Hits 67442 68093 +651
- Misses 20477 20552 +75 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 24 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| using UnityEngine; | ||
| using UnityEngine.InputSystem; | ||
| using UnityEngine.TestTools; | ||
| using Is = UnityEngine.TestTools.Constraints.Is; |
There was a problem hiding this comment.
was this not being used?
There was a problem hiding this comment.
I think when UnityEngine.TestTools.Constraints isn't included (with using) there is no ambiguous behavior that needs explicit aliasing to be resolved, but I probably need to double check with a version that enables this code.
There was a problem hiding this comment.
There was no GC memory allocation check so UnityEngine.TestTools.Constraints isn't needed at least
There was a problem hiding this comment.
using NUnit.Framework; should be enough
Description
The https://docs.unity3d.com/Packages/com.unity.test-framework@1.1/api/UnityEngine.TestTools.Constraints.AllocatingGCMemoryConstraint.html example is misleading since it guides to alias the extended Is from this package (adding a single extension).
This leads to warnings in certain compilers/IDEs, e.g. https://www.jetbrains.com/help/resharper/AccessToStaticMemberViaDerivedType.html
However, the extension is extending NUnit.Framework.Is and hence the aliasing of Is can be made to this type instead to still support the extensions while also not generating warnings in aforementioned IDE.
This PR has no functional impact but allows noticing warnings in test code in Rider instead of getting hundreds of warnings about the above problem.
Skipping CHANGELOG entry since this is mainly internal.
Testing status & QA
Just checked it compiles and no warnings in Rider 2025.2.3 with Unity 6000.2.7f2.
Overall Product Risks
Minimal.
Comments to reviewers
No functional impact at all is expected.
Checklist
Before review:
Changed,Fixed,Addedsections.Area_CanDoX,Area_CanDoX_EvenIfYIsTheCase,Area_WhenIDoX_AndYHappens_ThisIsTheResult.During merge:
NEW: ___.FIX: ___.DOCS: ___.CHANGE: ___.RELEASE: 1.1.0-preview.3.After merge: