Skip to content

Fixed XCTest load order issues exposed in Xcode 7.3 #819

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

Merged
merged 1 commit into from
Mar 24, 2016

Conversation

benasher44
Copy link
Contributor

It turns out that if you don't ensure that XCTestObservationCenter has already loaded, then attempting to use it and add an observer subverts some sort of internal setup required to get XCTest to spit out log lines during your tests like this:

Test Case -[MyTestTarget.testOne testMethodOne] started.

This is a really big problem if you rely on something like xcpretty parsing this output from XCTest to later generate a test report. To fix this, I moved any loading that KIF needed from a +(void)load method, into +[KIFTestCase setUp]. This fixed the issue, and now all of our important XCTest logging is back.

Things I tried that didn't work:

  1. Doing the setup in +[XCTestObservationCenter initialize] (not a good idea for other reasons, but I tried anyway)
  2. Doing the setup in a function with __attribute__(constructor)
  3. Moving the setup into +(void)load on XCTestObservationCenter. This appeared to work initially, but this didn't work when switching from using KIF as a static library to using it as a dynamic framework. So, I added a dispatch_async, and this appeared to work in every case. But, I found with further testing that enabling accessibility here wasn't nearly as reliable as enabling it in the XCTestObservation method (as it was previously).

Here are the docs on +(void)load for reference:

Invoked whenever a class or category is added to the Objective-C runtime; implement this method to perform class-specific behavior upon loading.

The load message is sent to classes and categories that are both dynamically loaded and statically linked, but only if the newly loaded class or category implements a method that can respond.
The order of initialization is as follows:

  1. All initializers in any framework you link to.
  2. All +load methods in your image.
  3. All C++ static initializers and C/C++ __attribute__(constructor) functions in your image.
  4. All initializers in frameworks that link to you.

In addition:

  • A class’s +load method is called after all of its superclasses’ +load methods.
  • A category +load method is called after the class’s own +load method.
    In a custom implementation of load you can therefore safely message other unrelated classes from the same image, but any load methods implemented by those classes may not have run yet.

I also went ahead and applied similar treatment I initially applied to the KIFAccessibilityEnabler setup, wherein I moved it into the + (void)load method on XCTestObservationCenter because that's the class it needed to use (i.e. attempt 3 above), to the UIApplication swizzling as well. In this case, we'd also want to also be sure that the UIApplication class is loaded in the runtime before interacting with it.

@benasher44
Copy link
Contributor Author

@schluete here's the fix

@phatmann
Copy link
Contributor

Thanks for the experimentation and the fix. If someone in the future finds a way to fix this without the dispatch_async hack, then we can use that. But at least we have a fix for now. Will merge after Travis passes.

@benasher44
Copy link
Contributor Author

@phatmann may want to hold off on merging this just yet, as we're still running this against our internal suite to see if other issues crop up. Will update soon once that's done, but I thought I'd just go ahead and get this out there

@phatmann
Copy link
Contributor

Good, coz it makes me nervous to have race condition hacks :-)

@benasher44
Copy link
Contributor Author

@phatmann okay so on our end things look good, but I wanna talk about this some more (because the hack and travis is failing). Is there any reason why we need to do it this way (using XCTestObservationCenter)? It seems like we could just go ahead and call _enableAccessibility once in + (void)load (like how it was done previously in the case that XCTestObservationCenter isn't available). This seems sufficient to me, since with XCTestObservationCenter we're setting up when we're told that the test bundle is starting and resetting when we're told the test bundle is finished. When the test bundle finishes, the program exits, so it seems like we could just set this up as soon as we know KIFAccessibilityEnabler and not worry about teardown. Then, we avoid this issue altogether.

Is there something I'm missing?

@phatmann
Copy link
Contributor

The teardown is done to restore the simulator to its initial state. See 927050d.

@benasher44
Copy link
Contributor Author

@phatmann Ah okay. How about we just do the setup in + (void)load, and then I'll add a destructor function like so:

__attribute__((destructor))
void ResetAccessibilityInpector() {
  [[KIFAccessibilityEnabler sharedAccessibilityEnabler] _resetAccessibilityInspector];
}

I've confirmed that this does get called right before the test bundle quits.

@benasher44
Copy link
Contributor Author

Okay, I looked back at #596, which appears to be the discussion that led to this change, and after that and some experimentation, it's pretty clear that all of this behaves far more reliably if we rely on XCTestObservationCenter. So, I'm back in the camp of keeping this hack.

@benasher44
Copy link
Contributor Author

Actually, doing this in +setUp for KIFTestCase seems to work pretty well. Going to see what travis thinks.

@benasher44
Copy link
Contributor Author

Okay, see the comment about why I now can't use XCTestObservation for cleanup.

On the plus side, this does make KIFAccessibilityEnabler more flexible: even if you're not using XCTestCase (is this true for some people? because this class is written like that's true), you have an API to use KIFAccessibilityEnabler yourself, and KIF will guarantee the cleanup for you regardless.

@benasher44
Copy link
Contributor Author

Alrighty this looks good on our end, and now it's passing travis! :)

This was referenced Mar 24, 2016
@phatmann
Copy link
Contributor

Looks like a good set of changes. Can you please squash the commits? Then I can merge it.

- Moved KIFAccessibilityEnabler setup into +[KIFTestCase setUp]
- Moved KIFAccessibilityEnabler teardown into a destructor function
- Moved UIApplication swizzling into +[UIApplication load] category (ensures
  UIApplication is loaded in the runtime)
@benasher44 benasher44 force-pushed the basher_fix_xctest_loading branch from 66a195a to 9418095 Compare March 24, 2016 23:01
@benasher44
Copy link
Contributor Author

Done!

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.

2 participants