[swift-build-dev] [swift-evolution] [Review] SE-0019 Swift Testing (Package Manager)

Fri Jan 8 01:00:17 CST 2016

Thank you, Paul. Your detailed feedback is both appreciated and just the sort of feedback we need to make this process useful.

The concerns that you raise here are the sorts of things that I think we'll be able to address outside this proposal process. The purpose of this process is to validate our high-level direction, not to vet every last technical detail, so I think we can do things like change the default behavior of `swift build` with respect to test code without going through this process. As such, I'd encourage you to follow up on the swift-build-dev mailing list about your specific reservations and help us get the right specifics in place here as we implement this.

Cheers,

	- Rick

> On Jan 7, 2016, at 9:26 PM, Paul Cantrell <cantrell at pobox.com> wrote:
> 
> What is your evaluation of the proposal?
> 
> I recommend accepting it as a vision and first draft, though not as a releasable product (yet).
> 
> This is a difficult proposal to review because of the way it mixes the general and the specific. On the one hand, it feels like this document’s primary intent is to lay out a large-scale vision, a philosophy of how the Package Manager should treat tests — and more importantly, a vision of the habits package authors should have around writing tests. On the other hand, the proposal is full of very specific details (such as directory structure) — too many such details to evaluate it as a large-scale vision only, yet not enough to evaluate it as a completely specified solution. I’ll therefore take the vision and the details separately.
> 
> I like the proposal’s broad vision very much. It advocates three important cultural norms about testing in Swift, all somewhat contrary to other package management systems, but all ones I approve of:
> 
> The package manager not only allows but proactively encourages package authors to write and maintain tests.
> Tests are a uniform feature of Swift packages, with a standard invocation mechanism and standard result reporting (as opposed to being an ad hoc build task that differs from project to project).
> Tests should run reproducibly, for everyone, on checkout with no manual setup. It’s just “run tests;” there is no step 2.
> 
> While the proposal doesn’t state all of this quite so specifically, it’s there in the thinking — and I approve. Thinking about this proposal in terms of Chris Lattner’s notion of the “programmer model,” I see this proposal tending toward a world where a package’s tests are run regularly not only but its contributors, but also its clients. That has cultural implications: users will hold package owners accountable if tests are missing, don’t work, or fail to catch problems. “Tests pass for everyone everywhere” is a significantly higher bar than “tests passed for one person somewhere,” and I like a Swift culture built around that higher standard of reproducibility.
> 
> The specific details of the proposal are a mixed bag. These seem good to me:
> 
> the proposed directory structure,
> the notion of multiple test modules,
> automatic dependency determination between test modules and library targets,
> the long-term goal of “swift test” being the command line invocation mechanism,
> the ability to target individual tests from the command line,
> starting with XCTest as the initial mechanism with the goal of shifting to a more abstract protocol-based approach in the future, and
> providing machine-readable test results.
> 
> These details I have questions or reservations about:
> 
> lack of clarity about how to run individual test modules from the command line,
> lack of clarity about which test modules are run by default with “swift test” (“all of them” is probably the wrong answer),
> lack of clarity about how test modules interact with Package.swift, and related to that, how a project should specify build configuration and dependencies specific to a particular test module,
> the “swift build --test” syntax (although I realize that this initial approach is due to larger constraints), and
> a seeming assumption that extra compilation is essentially free.
> 
> This last one deserves special comment. The notion that the build command will also build tests but will not run them seems peculiar, and poorly justified in the document. Why build if you’re not going to use the build output? Developers spend hours shaving fractions of seconds off of their build cycles, and for good reason: development is highly iterative. And not all those iterations involve running tests!
> 
> The proposal’s authors’ answer to this seems to run something along the lines of “incremental builds are fast, and slow test builds are a code smell anyway.” That’s a weak justification. Incremental builds are not fast when iterating on changes to a core type that forces recompilation of large swaths of the tests, and even in ideal cases, Swift’s incremental builds are not always as fast in practice as they ought to be in theory. If users’ tests are building slowly, then either (1) that smell is there for a good reason, and you’re punishing users already in pain, or (2) the additional disincentive of slow builds is unlikely to alter behavior for which there are already the much larger disincentives of high maintenance cost and difficulty bringing in new contributors.
> 
> The “--without-tests” option does little to allay this concern. Because it is the right default, it will become another hoop to jump through — and Swift developers will forever be swapping shell aliases that make it the default. (Check the number of upvotes on this question: http://stackoverflow.com/questions/1381725/how-to-make-no-ri-no-rdoc-the-default-for-gem-install <http://stackoverflow.com/questions/1381725/how-to-make-no-ri-no-rdoc-the-default-for-gem-install>)
> 
> Carthage made similar mistakes about unnecessary builds being approximately free, and has paid for it in a larger stream of user complaints (some written by this author, as you can probably tell).
> 
> These concerns aside, this proposal is the right direction. My recommendation is thus to accept it as a vision, direction for development, and first draft, with the expectation that subsequent proposals will address the concerns above.
> 
> Is the problem being addressed significant enough to warrant a change to Swift … er, the Swift PM?
> 
> Yes. As noted above, making testing a first-class citizen of the Swift ecosystem has large cultural benefits. The potential to be able to build dependency tests against a particular set of resolved version is highly appealing. Setting testing standards at the package manager’s outset will help prevent test approach fragmentation, and thus help realize these benefits.
> 
> Does this proposal fit well with the feel and direction of the Swift PM?
> 
> It does fit with the Swift PM’s general direction of being prescriptive about top-level project structure and standardized build process, while leaving intra-module structure fairly open.
> 
> If you have you used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
> 
> I’ve used bundler extensively, which does not address testing at all. Though Ruby has a strong testing culture, individual gem tests are of highly varying quality, and are sometimes difficult to run. Environment-related gem bugs — new Ruby version, incompatible dependency version, etc. — are sometimes a problem, and would presumably be easier to catch if it were easy to run gem tests en masse.
> 
> How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
> 
> I read the proposal carefully and asked some questions, but have not followed the whole discussion thread.
> 
> Cheers,
> 
> Paul
> 
> 
>> On Jan 5, 2016, at 1:06 PM, Rick Ballard via swift-evolution <swift-evolution at swift.org <mailto:swift-evolution at swift.org>> wrote:
>> 
>> Hello Swift community,
>> 
>> The review of “Swift Testing” for the Package Manager begins now and runs through Thursday, January 7th. The proposal is available here:
>> 
>> 	https://github.com/apple/swift-evolution/blob/master/proposals/0019-package-manager-testing.md <https://github.com/apple/swift-evolution/blob/master/proposals/0019-package-manager-testing.md>
>> 
>> For this particular review, please note that a significant amount of discussion history is available in the original pull request for the proposal:
>> 
>> 	https://github.com/apple/swift-evolution/pull/51
>> 
>> Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-evolution mailing list at
>> 
>> 	https://lists.swift.org/mailman/listinfo/swift-evolution
>> 
>> or, if you would like to keep your feedback private, directly to the review manager.
>> 
>> 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 you 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
>> 
>> 	https://github.com/apple/swift-evolution/blob/master/process.md
>> 
>> Thank you,
>> 
>> 	- Rick
>> 	  Review Manager
>> _______________________________________________
>> swift-evolution mailing list
>> swift-evolution at swift.org <mailto:swift-evolution at swift.org>
>> https://lists.swift.org/mailman/listinfo/swift-evolution <https://lists.swift.org/mailman/listinfo/swift-evolution>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-build-dev/attachments/20160107/f7b94269/attachment.html>