Skip to content

[test] Disable test on Linux that's ICU version-specific #20780

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 27, 2018
Merged

[test] Disable test on Linux that's ICU version-specific #20780

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions test/stdlib/CharacterProperties.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,14 @@ CharacterPropertiesTests.test("Casing") {
expectTrue(Character("π").isLowercase)

expectEqual("SS", Character("ß").uppercased())

// Some versions of ICU on Linux (62.1) have a bug producing the wrong value
// when lowercasing "ẞ". Darwin platforms never shipped this version, so
// conditionally test based on platform.
#if _runtime(_ObjC)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linux could in theory have the ObjC runtime. Can we conditionalize this on "Darwin"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The convention for asking "is this a Darwin platform that has Apple's implementation of the objc runtime and Foundation" is spelled #if _runtime(_ObjC). This is an internal conditional check (not exposed to users) and should return false on any Linux, where interop would otherwise be broken.

This predates me and I don't really know why it's done this way, but it's followed consistently throughout the standard library in hundreds of places. CC @jrose-apple and @jckarter who might have opinions about this convention and whether it should change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It well predates canImport, which is at least part of the reason.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does #if _runtime(_ObjC) check whether the runtime is available or merely ObjC interoperability is enabled? I ask because there are a few tests that are run twice, once with interop and once without, and regardless of platform.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's asking if interop is enabled, i.e. "will the Swift runtime be one that interoperates with Objective-C".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@milseman – Can we please change this to "#ifdef darwin"? I'd really like to see the conflation of the ObjC runtime with the Darwin platform kept to a minimum.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have -enable-objc-interop which controls whether ObjC interop is present or not. Darwin and ObjC are not synonymous.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For all practical purposes, they are. I would bet that any theoretical interop with any other ObjC runtime and Foundation implementation is going to require a lot more than a binary flag here, since the existing code paths for generating metadata and integrating with ObjC runtime and Foundation facilities are extremely specific to Apple's implementation. Maybe we can change the name to _runtime(_AppleObjC) and change the flag to -enable-objc-interop=apple if it makes you all feel better, but I think this is the right condition to use in the standard library, regardless of what we name it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed https://bugs.swift.org/browse/SR-9358 for that.

expectEqual("ß", Character("ẞ").lowercased())
#endif

expectEqual("и", Character("И").lowercased())
expectEqual("И", Character("и").uppercased())
expectEqual("π", Character("Π").lowercased())
Expand Down