Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to me as the review manager by DM. When contacting the review manager directly, please put "SE-0455" in the subject line.
What goes into a review?
The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:
What is your evaluation of the proposal?
Is the problem being addressed significant enough to warrant a change to Swift?
Does this proposal fit well with the feel and direction of Swift?
If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
More information about the Swift evolution process is available at:
It can also improve debug build performance as fewer symbols exported from a binary can result in faster linking.
Ahh… good news! Do we have any open source Benchmarks with examples of the potential perf wins?
It will be changed to default to "unspecified" (respecting any target settings), and explicitly passing --enable-testable-imports or --disable-testable-imports will force all targets to enable or disable testing, respectively.
So I'm clear… we are implying that passing disable-testable-imports on a target should cascade down through the graph of dependencies? So that building the Foo target (that depends on Bar) with disable-testable-imports will also build Bar with disable-testable-imports (unless the engineer that built Bar explicitly passed enable-testable-imports?
This seems fine, my biggest issue is that @testable is big blunt instrument that's barely sufficient for testing. And if you're trying to be hygienic, it's just wrong. It should be fully replaced or greatly enhanced in the future. But giving packages control of the current feature is good. My only question is whether this still allows for package consumers to build them with testability?
The existing --enable-testable-imports / --disable-testable-imports command line flag to swift-test currently defaults to --enable-testable-imports . It will be changed to default to "unspecified" (respecting any target settings)
Will having it off by default result in many broken builds when this rolls out? Symbols previously available to tests will disappear until enableTestableImport is used.
Thanks for the feedback so far! I'll try to address all the outstanding questions:
No, this is a theoretical performance win that I mentioned only because it's a nice potential side benefit, I haven't measured it and don't expect to as it's not the main goal of the feature.
These command line options are passed to swift build and apply to all targets in the build graph; you can't pass different values for individual targets.
This paragraph is meant to make clear that the command line options will override any use of the enableTestableImport package manifest API.
I actually don't think we should have these command line options in the first place, but they're mentioned in the proposal because they exist today and their behavior is necessarily impacted by this feature, and any proposal to remove them would be something to discuss separately.
Testability is controlled by the producer, not the consumer, so no. That's a property of how the feature works, and not something I'd want to work around at the build system level due to the level of complexity that would be required.
Please note that this proposal does NOT change the default behavior, even when adopting the new manifest version. @testable will continue to be enabled by default in debug mode until/if a potential future proposal switches the default.
The command line flags only override what the package manifest specifies and generally shouldn't be used.
Attempting to enable @testable import in release builds will result in a build warning.
This should probably not happen. We actually want to encourage teams to test their release code (yes, even if it means explicitly enabling testability.) Otherwise how can you say your release code has anything other than 0% code coverage in testing?
Separately, I believe we had discussed the --build-tests flag and decided that when it was passed, testability should be implicitly enabled even in release mode. I don't see that called out here anywhere, although maybe I missed it.
We cannot allow enabling @testable import in normal release builds via Package.swift because that would risk permanently over-exporting symbols to all users of the package for normal release builds. And this is something we already don't allow today.
That's what the command line overrides are for, because they are inherently temporarily applied to the build.
We also don't want that, because the purpose of this proposal is to allow disabling use of @testable import at the package level for codebases which don't want it. If you want to force-enable @testable in release mode for testing purposes because some of the packages in the graph are using @testable, that's what the command line options are for.
