test(setup): look up UpdateDependencyStatus as static too - #1385
Conversation
CoplayDev#1383 made MCPSetupWindow.UpdateDependencyStatus internal static so the new GitDetectionTests could drive it directly. The characterization test still looked it up with BindingFlags.NonPublic | Instance, which no longer matches, so GetMethod returned null and the fixture failed on the Assert.IsNotNull guard rather than on the behaviour it exists to describe. beta has been red on it since 9d93c42. Adding Static alongside Instance keeps the lookup working whichever way the method is declared. The test is a characterization test - it records that the window drives "valid"/"invalid" class lists, which is still exactly what the method does. Not caught before merging because CoplayDev#1383 came from a fork, where the Unity test legs report Skipped, and the pre-merge run was filtered to GitDetectionTests. Measured on 2021.3.45f2: full EditMode 1243 total, 1163 passed, 0 failed, 80 skipped; WindowsCharacterizationTests 31/31.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe characterization test now includes ChangesReflection Test Update
Estimated code review effort: 2 (Simple) | ~5 minutes ✨ 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 |
There was a problem hiding this comment.
🟢 Approval recommended
The change is a narrow, correct test-only fix that restores reflection lookup compatibility with the current static method signature.
Pull request overview
Fixes a Unity EditMode characterization test that broke after MCPSetupWindow.UpdateDependencyStatus was changed to internal static, by updating the reflection lookup to match static methods as well as instance ones. This keeps the characterization test focused on the intended behavior (CSS class-list changes) rather than failing on a null MethodInfo.
Changes:
- Update
GetMethodBindingFlags to includeBindingFlags.StaticalongsideBindingFlags.Instance. - Add an explanatory comment describing why both flags are included and what failure it prevents.
File summaries
| File | Description |
|---|---|
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Windows/Characterization/Windows_Characterization.cs | Adjusts reflection BindingFlags so the characterization test can find UpdateDependencyStatus after it became static. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Description
betais red onWindowsCharacterizationTests.MCPSetupWindow_ModifiesClassListForStatus_ValidInvalidPatternand has been since 9d93c42 (#1383).#1383 made
MCPSetupWindow.UpdateDependencyStatusinternal staticso the newGitDetectionTestscould drive it directly. The characterization test still looks it up withBindingFlags.NonPublic | BindingFlags.Instance, which no longer matches a static method, soGetMethodreturnsnulland the fixture dies on itsAssert.IsNotNullguard rather than on the behaviour it exists to describe.Changes Made
Adds
BindingFlags.StaticalongsideInstance, so the lookup works whichever way the method is declared. The test is a characterization test — it records that the window drives"valid"/"invalid"class lists on the indicators, which is still exactly what the method does, so the assertion it makes is unchanged and correct.Why it was not caught
#1383 came from a fork, where the Unity test legs report Skipped, and the pre-merge EditMode run was filtered to
GitDetectionTests. Nothing ran the characterization fixture.This is the second floor-only miss this cycle. Widening
compile-check.ymlpastdefaultVersionis worth doing on its own, but only a real test run catches this class — worth considering whether the maintainer-side pre-merge check should always run the unfiltered suite.Compatibility / Package Source
TestProjects/UnityMCPTestsis pinned to)file:Testing
WindowsCharacterizationTestson its own: 31/31.Server/changeBefore this commit the same full run was 1 failed.
Documentation Updates
Summary by CodeRabbit