8000 Adjust the way that test observation suspension works. by briancroom · Pull Request #486 · Quick/Quick · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Adjust the way that test observation suspension works. #486

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 2 commits into from
Mar 2, 2016

Conversation

briancroom
Copy link
Member

When I updated the Nimble submodule to fix compilation on Xcode 7.3, I discovered that the changes made in Quick/Nimble#244 break the Quick tests because of the way that we use +[XCTestObservationCenter _suspendObservationForBlock:] to enable running XCTestSuite instances during the middle of an existing run. The problem is that Nimble now relies on a test observer of its own in order to be able to report failures to XCTest (see the CI failure here).

This PR changes the QCKSpecRunner behavior so that, instead of using private API on XCTestObservationCenter, it swizzles public methods to keep track of XCTest's own observers, then temporarily removes them from the center to implement the suspension behavior. I'm not convinced that this is necessarily any more future-proof than the old mechanism, because it relies on being able to add and remove observers at arbitrary times during an ongoing test run, but it does work with current XCTest versions, at least.

Any concerns, @Quick/contributors?

@briancroom briancroom force-pushed the rework_suspendObservation branch from 52ecf67 to 0223ec3 Compare February 16, 2016 11:54
@briancroom
Copy link
Member Author

Well, I take back that part about avoiding private APIs...

Unfortunately it seems that xctool does swizzling on XCTestLog which is a "legacy test observer" and uses a different registration mechanism, with the result that it threw exceptions about getting into an invalid state when the nested test suite started running. I've now pushed an alternative implementation which manipulates the XCTestObservationCenter's observers property to remove any observers (legacy or standard) which come from XCTest, while leaving custom ones (like Nimble's).

I'm not thrilled about needing to do this sort of hackery, but I don't have any other workarounds in mind at this point.

Modify the internal set of observers managed by XCTestObservationCenter
to not include XCTest's own observers while performing the suspension.
This allows user-provided observers to remain in place, which is needed
for Nimble to work properly now.
@briancroom briancroom force-pushed the rework_suspendObservation branch from 0223ec3 to 6105748 Compare February 16, 2016 12:01
@briancroom briancroom mentioned this pull request Mar 2, 2016
modocache added a commit that referenced this pull request Mar 2, 2016
Adjust the way that test observation suspension works.
@modocache modocache merged commit 8be4a82 into Quick:master Mar 2, 2016
@modocache
Copy link
Member

Sorry to keep you waiting, @briancroom! This looks great. I'm also not thrilled about all the hoops we jump through to support xctool... but c'est la vie, I guess 😅

@implementation XCTestObservationCenter (QCKSuspendObservation)

static BOOL (^isFromApple)(id, BOOL *) = ^BOOL(id observer, BOOL *stop){
return [[NSBundle bundleForClass:[observer class]].bundleIdentifier containsString:@"com.apple.dt.XCTest"];
Copy link
Member

Choose a reason for hiding this comment

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

nit-pick: As a maintainer I would love an inline comment here describing why this isApple predicate is necessary. I understand I can git blame and find this pull request, but hand waves code locality something something.

@briancroom briancroom deleted the rework_suspendObservation branch March 2, 2016 11:38
@briancroom
Copy link
Member Author

Thanks for the review and good points @modocache and @sharplet! I've pushed a commit addressing these things to master.

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.

3 participants
0