Skip to content
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

Only call +before/afterEach on CDRHooks conformers #387

Merged
merged 3 commits into from
Apr 15, 2016

Conversation

idoru
Copy link
Contributor

@idoru idoru commented Apr 12, 2016

Rebased from #365, some small refactoring and addition of tests for +afterEach hook.

Tests for +afterEach are performed via asserts that are set up using atexit(3) since Cedar specs don't have visibility outside of their own context. Would like to know if anyone has better ideas for how to test this.

@tjarratt and I debated the inclusion of the test classes in CDRHooksSpec.mm but we ultimately felt it made the distinguishing characteristics clear. Curious to know how others feel about it.

This change also stops special casing for UIAccessibilitySafeCategory__NSObject and SCRCException since we shouldn't expect them to conform to CDRHooks.


describe(@"global afterEach", ^{
it(@"should run after all specs", ^{
atexit(verifyAfterEachCalledAsExpected);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is awesome! I'll have to remember this trick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This probably further warrants that the two class examples are kept here and only have effect on static variables local to the test implementation - that way no other machinery (including other tests) can unintentionally mess with this state between the time these tests run and the time the process exits and triggers the NSCAssert validations.

@tjarratt
Copy link
Contributor

Interesting point about not having to special case those classes now -- because that if statement should bail out early when those classes don't conform to our protocol, it should be safe to call that function with any Class. That's a great improvement to this bit of code.

Rebased from and added more tests from #365
@idoru idoru force-pushed the hooks-protocol-before-after-each branch from e3a01d3 to 82debd2 Compare April 12, 2016 21:12
@idoru
Copy link
Contributor Author

idoru commented Apr 13, 2016

Having slept on this, I'm not happy that these tests only really can see if the methods were called, but not when.

In fact, as soon as a single example runs (quite likely before this test), then +beforeEach and +afterEach on the conformant class will already have been called.

@idoru idoru self-assigned this Apr 13, 2016
@idoru
Copy link
Contributor Author

idoru commented Apr 13, 2016

2db594b is closer to what I'm thinking but I only realized when I finally ran rake that this doesn't work on suites because the TestObservationHelper relies on XCTest.

@idoru idoru force-pushed the hooks-protocol-before-after-each branch from 2db594b to 873eabf Compare April 14, 2016 22:02
@idoru idoru force-pushed the hooks-protocol-before-after-each branch from 873eabf to 14c129a Compare April 15, 2016 14:01
@@ -205,7 +205,6 @@ void expectFailure(CDRSpecBlock block) {
globalValue__ = @"something";
});

expect(globalValue__).to(be_nil);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@briancroom I think this was added in error 2 years ago... What's odd is that the example further up on line 174 seems to imply this should have raised an exception in the main spec run, but it only occurred when the spec gets redefined via construction of a CDRSpecRun in CDRHooksSpec.

@idoru idoru merged commit a323321 into master Apr 15, 2016
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