Skip to content

Add Unit Testing Infrastructure #61

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

Closed
wants to merge 3 commits into from

Conversation

briancroom
Copy link
Contributor

Motivation

Corelibs XCTest already has an mature setup for executing end-to-end functional tests against itself which is doing an outstanding job of providing confidence that the library will work as expected when linked into clients' test suites. Sometimes, though, it feels more appropriate to write more granular unit tests against particular APIs provided by the library. This PR aims to satisfy that missing component by adding a suite of unit tests which can be run from Xcode on OS X, or from the build script on other platforms.

Details

The unit test suite is located in the Tests/Unit/ directory, as a sibling to the existing functional test suite. It uses Corelibs XCTest to test itself, building an app whose main.swift invokes XCTMain with the list of XCTestCase subclasses which contain the tests themselves. I implemented a simple pair of tests around XCTAssert behavior to act as a seed and example test for the suite.

Ordinarily when writing unit tests for a library, I would prefer to link the library into the test target and restrict the tests to working against the public API so as to prevent them on relying too much on implementation details. For this suite, however, I came to the conclusion that the public API (at least in its present form) lends itself rather poorly to unit testing because it generally requires calling into XCTMain to set up state, and that function can't be called from a unit test without exiting the process altogether! Because of this, I decided to simply build the library source files together with the unit test sources, allowing for judicious use of internal API in unit tests. (Note that the standard integration of the Corelibs XCTest dynamic library into client executables is thoroughly by the Functional test suite.)

For OS X, the Xcode project now has a new SwiftXCTestUnitTests app target which executes the test suite when run. For Linux, the build script's test action has been extended to also build and run the unit tests, although, notably, it does not link the unit tests against an already-built XCTest dynamic library the way that the functional tests do.

CI

To get CI to run the new test suite, the overall Swift build script will need to be updated (around here) to execute the unit test suite on OS X., On Linux no additional work is required because it is already taken care of by XCTest's own build script.

@modocache
Copy link
Contributor

I'm excited to explore the idea of a unit test suite for corelibs-xctest. Thanks for working on this!

I see how XCTMain() exiting prevents unit testing public API. That being said, I'm not entirely convinced we should be testing internal API. XCTFailureHandler, for example, is a global function that was introduced in a refactoring with no corresponding change in behavior (b414213). I'm worried that if we choose to refactor failure handling further, the unit test introduced in this pull request would end up slowing us down. I envision a situation in which a contributor refactors failure handling and all the functional tests pass (so the behavior is identical), but the unit test fails (because it tests an implementation detail).

That being said, I'm aware I may be close-minded on this subject--I've typically only ever tested internal API when adding tests to a legacy codebase that had no tests at all. So maybe I need to expand my mind a bit here. @ddunbar has commented in the past on how he thought tests should be implemented--is this the sort of thing you had in mind?

One follow-up question for you: is there a public API in corelibs-xctest or Apple XCTest you envision unit testing? In other words, do you anticipate the unit test suite will only be used for internal API?

@modocache
Copy link
Contributor

One follow-up question for you: is there a public API in corelibs-xctest or Apple XCTest you envision unit testing?

After further thought, I'll answer my own question: I wouldn't mind a test for XCTPrint(). Does it flush output as it claims? Does it print things out as it should (that is, could we add a regression test for #45)?

I'm still not convinced of testing internal API, but if we were to choose to do so, I think XCTPrint() would be a good place to start.

@briancroom
Copy link
Contributor Author

I'm not entirely convinced we should be testing internal API

Neither am I! And I'm actually glad that you are trying to talk me out of this madness 😆 But I'm going to stand my ground for now at least, and let's see where this discussion goes!

Because this project is aiming to hold tightly to an existing API and be very conservative about exposing additional public API, I have the impression that it shares some characteristics with legacy codebases in terms of testability. Writing that makes me a bit sad.. I wonder how we can reconcile these goals with the desire of having a public API more amenable to unit testing?

I have no particular attachment to the particular unit tests that I included in this PR to bootstrap the suite, and your point about the test increasing the codebase's rigidity is valid. I must admit to being unsure how to approach the XCTPrint() test you propose though, unless through something like using dup2 to redirect stdout to a pipe, or similar. Messing with global state like that feels a bit smelly for a unit test though :/

The concrete bit of work that motivated me to produce this PR was adding the ability to use a command line argument to specify a particular test case to execute (See here and here.) I felt that the Functional test suite on its own wasn't going to let me apply TDD for the new feature. I'm not anticipating exposing any new public API for this work, though, which pushed me down the path you see in this PR.

With regards to other bits of work, it seems to me that unit tests for e.g. an XCTestObservationCenter implementation would require calling into internal API as well, because that class' public API doesn't include any methods for informing it about events that have occurred during the test run.


That was a bit scatterbrained, but hopefully reveals more of my thoughts about this. Feedback please! 😄

@modocache
Copy link
Contributor

Ah, thanks for the links to the command-line interface work--I hadn't been aware of that! I can see what you mean about the functional test suite: I think it'll be great at verifying the intended behavior, but not conducive to the unit-based testing that TDD would require.

I'm not quite convinced by your thoughts on XCTestObservationCenter, since my inclination would be to write a functional test that included a registered observer, then confirm that it received the messages. Once again my preference for only testing public API is showing here. 😅

Another approach to unit testing would be to split this up into separate modules, then only test the public API of those modules. The logic to parse command-line arguments, for example, seems like it could be extracted into its own framework.

It sounds like we'll need some more opinions here. @mike-ferris-apple? @ddunbar?

@briancroom
Copy link
Contributor Author

I like the concept of breaking this up into modules, but I don't have a clear picture of what that would look like in terms of how the project is built. (Incidentally, that reminds me of this - I hope you can feel my internal conflict about testing internal!)

As another concrete case which to contribute to this discussion, here are the tests I produced while TDDing my way towards #64. I only included the functional test in that PR as I didn't want to conflate things too much there.

@mike-ferris
Copy link

A few scattered comments...

First, a purely technical thing. @briancroom , you said in the original description that you're compiling the sources for XCTest into your test bundle to get access to internal stuff. Did you consider compiling XCTest for testability and using @testable import instead? That would seem the more normal way to do this. On the Linux end, we could consider having debug builds of XCTest be compiled for testability always to facilitate this. (Xcode does this by default for the Debug build configuration as well.)

On to the more philosophical stuff. In a significant code base I don't think it's avoidable to have unit tests need to have access to and use internal API. (That's why the testability feature was added last year to Swift...) Generally speaking when designing a public API, I don't think sufficient testability of the breadth of the implementation should be a forcing factor to making API public. There will be aspects of the internals of the framework that it is important to ensure are functioning properly so that higher level testing can assume those pieces are working, but that doesn't mean those aspects should themselves be public.

So, in general I don't have a problem with unit tests testing internals.

That said, it would be nice, as @modocache points out, not to have to rely on aspects of the internals that we know we'd like to remove (like our global state) as much as possible.

For the test that is being implemented here, I would much rather see that use XCTestObservation (of course that requires that we have that bit of API, but with @briancroom 's recent work on XCTestCase I believe we're ready for a trivial implementation of the observation stuff to be pretty, well, trivial.) But I'd also love to see any implementation of XCTestObservationCenter come along with unit tests that test the basic mechanism of test observation in isolation (just using the observation center, an observer and calls into the internal methods that cause the observers to be notified, no test cases involved), which will require use of internal API in that area. Of course, we'll also want testing that the test cases themselves are integrated with the observation center appropriately. These tests can be more straight-forward and probably won't require non-public API. With all that testing, we can then have confidence that further unit tests that are predicated on the observation system can be relied upon to work as intended and, in turn, test what they're meant to test.

Getting that in place, with the infrastructure that @briancroom has implemented here, will put us in a good place to test all sorts of behavior having to do with what tests run, in what order, and what kinds of failures they report all through public API...

@briancroom
Copy link
Contributor Author

Did you consider compiling XCTest for testability and using @testable import instead?

Yeah I went down that route first, which worked great on OS X in Xcode, but ran into some challenges with the build script on Linux which made me decide to try this approach instead. In particular, my attempts to update the build script to integrate the unit tests this way would have resulted in some troublesome UX for people using the script.

Enabling testability for the library for debug mode makes sense, but then what happens when the script is used in release mode (the default)? Do we just skip the unit tests? That seems potentially misleading, because the fact that the script exited with 0 after a test run still wouldn't mean that the library has been fully tested. It's even a bit tricker when directly running the test action on the script to test a previously-built library because in that case we don't even know whether it was built in release or debug mode.

I considered having the script always build a version of the library with testablity enabled and putting it somewhere else for the unit tests to link against, but that is still misleading in the case of the test action which is supposed to run the tests against an existing library, not build a new one.

That said, I agree that importing the library with @testable would be a more standard approach and I'd be happy to see us do that if we can solve these issues satisfactorily!

@mike-ferris
Copy link

These are issues that we face generally. As, more and more, we have to build differently in order to be able to do (some kinds of) testing, it makes things more complicated.

What it comes down to is that tests that need access to non-public API cannot be run against production-built stuff. I guess this is one more reason to prefer tests using only public API, but I still think that's impractical for the finest grain testing that one would want/need in many cases.

It's certainly not a huge deal to compile the framework code directly into the tests until/unless we come up with good solutions to the issues you raise. That itself has similar drawbacks. If the tests and the release framework are built at the same time, you know they came from the same code, but they were still built differently (the presence of the test code that may be using the internals in different ways than the rest of the code base may cause different amounts optimization of the internal APIs. In an ideal world, that wouldn't matter, and even in the real world it probably matters only rarely, but it's still a difference.

@briancroom
Copy link
Contributor Author

Just wanted to drop a note that I haven't forgotten about this, and hope to get a chance to revisit this soon, especially now that our public API has a larger surface area than when I was first working on it!

@modocache
Copy link
Contributor

Yes, I think this deserves another discussion now that we're mirroring the XCTest API more closely. It may help us verify subtle behavior that would be very unwieldy to put in a functional test. For example, "if I call XCTestRun.totalDuration before a test has finished running on Apple XCTest, does it return 0 or throw an exception?"

@ddunbar
Copy link
Contributor

ddunbar commented Apr 9, 2016

My $.04 here:

  • I agree with Mike that testing internal API is unavoidable in a large project. I would even go so far as to say it is a desirable thing to do, because large frameworks that are comprised of small, effective, public APIs are a thing of beauty -- making them have quick effective tests is going to require testing internal API.
  • I do think that it is important when writing tests against internal API to be very cognizant of testing at the right boundary. I'm not a fan of unit tests that test implementation details and rely heavily on test-specific mocking (as opposed to general purpose protocols part of the design itself). I try to restrict testing to what I consider "strong API boundaries" -- usually places where one component can be treated as black box by another component within the same module. This also places into the "unit tests test your API design" philosophy.
  • I think using @testability is the right thing to do, even though it has pain points. This is the best current "push" towards having better language support for this (and note that testability itself is not tied to debug builds, that is just an Xcode default -- there is nothing preventing users from making @testable release builds). Pain points here are things we need to address in the compiler / tooling.
  • Another approach, which may or may not be relevant here (I'm not familiar enough with the internal complexity to know) is to develop additional "public" interfaces (this could itself either be an internal only tool, or internal only API tested with unit tests) that expose a swiss army knife of tools to test the library. This is the approach taken by Swift tools like swift-ide-test. For XCTest, that might take the form of infrastructure that handles setting up dummy testing contexts, test methods, etc and then easily validating the results.

@shahmishal
Copy link
Member

The Swift project moved the default branch to main and deleted master branch, so GitHub automatically closed the PR. Please rebase on top of the main and we can reopen.

More detail about the branch update - https://forums.swift.org/t/updating-branch-names/40412

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants