-
Notifications
You must be signed in to change notification settings - Fork 516
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[tools/tests] Fix bug in 'link all' test and the resulting regression that showed up in code. #18016
Conversation
… that showed up in code. There's a 'link all' test that's verifying that the IntroducedAttribute is linked away. It does so by verifying that the linked app doesn't have a 'IntroducedAttribute' type - but the test was constructing the fully qualified type name to look for incorrectly: ObjCRuntime.IntroducedAttribute, , Microsoft.iOS Note the double comma: that meant we wouldn't find the type, even if it wasn't linked away. The fix is easy (use a single comma), with one caveat (don't use a constant string, because the linker sees the reference to "ObjCRuntime.IntroducedAttribute" and _helpfully_ preserves it, exactly what we don't want), but it revealed that the tested behavior regressed: a fully linked app wouldn't link away the IntroducedAttribute. So a fix is also needed: properly remove TVAttribute, WatchAttribute and MacCatalystAttribute, which are subclasses of IntroducedAttribute (and what would make the linker keep IntroducedAttribute). Interestingly this showed up because of a bug in the runtime, where parsing the invalid assembly name would now throw an exception (dotnet/runtime#84118).
✅ API diff for current PR / commitLegacy Xamarin (No breaking changes)
NET (empty diffs)
✅ API diff vs stableLegacy Xamarin (No breaking changes).NET (No breaking changes)✅ Generator diffGenerator diff is empty Pipeline on Agent |
💻 [CI Build] Windows Integration Tests passed 💻✅ All Windows Integration Tests passed. Pipeline on Agent |
💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻✅ All tests on macOS M1 - Mac Big Sur (11.5) passed. Pipeline on Agent |
🚀 [CI Build] Test results 🚀Test results✅ All tests passed on VSTS: simulator tests. 🎉 All 225 tests passed 🎉 Tests counts✅ bcl: All 69 tests passed. Html Report (VSDrops) Download Pipeline on Agent |
💻 [PR Build] Tests on macOS M1 - Mac Ventura (13.0) passed 💻✅ All tests on macOS M1 - Mac Ventura (13.0) passed. Pipeline on Agent |
There's a 'link all' test that's verifying that the IntroducedAttribute is
linked away. It does so by verifying that the linked app doesn't have a
'IntroducedAttribute' type - but the test was constructing the fully qualified
type name to look for incorrectly:
Note the double comma: that meant we wouldn't find the type, even if it wasn't linked away.
The fix is easy (use a single comma), with one caveat (don't use a constant
string, because the linker sees the reference to
"ObjCRuntime.IntroducedAttribute" and helpfully preserves it, exactly what
we don't want), but it revealed that the tested behavior regressed: a fully
linked app wouldn't link away the IntroducedAttribute.
So a fix is also needed: properly remove TVAttribute, WatchAttribute and
MacCatalystAttribute, which are subclasses of IntroducedAttribute (and what
would make the linker keep IntroducedAttribute).
Interestingly this showed up because of a bug in the runtime, where parsing
the invalid assembly name would now throw an exception
(dotnet/runtime#84118).