-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Fix OperatingSystemTest.TestIsOSPlatform_MacCatalyst #84842
Conversation
It got broken by 3d160bc because the assert wasn't updated. Changed the code to verify the iOS/Catalyst conditions explicitly so this doesn't happen again.
/azp run runtime-maccatalyst |
Azure Pipelines successfully started running 1 pipeline(s). |
Assert.False(allResults[10]); // IsWasi() | ||
for (int i = 0; i < allResults.Length; i++) | ||
{ | ||
bool expected = (i == 4 || i == 5) ? true : false; // true for IsIOS() and IsMacCatalyst() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bool expected = (i == 4 || i == 5) ? true : false; // true for IsIOS() and IsMacCatalyst() | |
bool expected = i is 4 or 5; // true for IsIOS() and IsMacCatalyst() |
?
Might be cleaner to change allResults to instead be a Dictionary<string,bool>
, where the string is the name of the Is method. The else case could still assert that only one value was true, and this case could assert that both "IsIOS" and "IsMacCatalyst" are true and everything else is false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated with your suggestion
It got broken by 3d160bc because the length assert wasn't updated. Changed the code to verify the iOS/Catalyst conditions explicitly so this doesn't happen again.