Permalink
Browse files

[ios-sdk] Fix unit tests, two other bugs

Summary:
Apparently either the Lincoln Memorial moved, our place data changed, or our distance algorithm
changed, since our queries that used to work with a 100m radius now require a 200m radius to
return the expected results.

FBRequestConnection was not properly nil-ing an NSError pointer prior to passing it by reference
down the stack. This caused inconsistent behavior in error handling depending on what happened with
previous requests.

Always upper-case HTTPMethod names before setting them on NSURLRequest, as HTTP standard says they are
case-sensitive and FB servers will reject requests with, e.g., lower-case methods.

Made FBTestSession shouldExtendAccessToken faker be one-shot rather than always saying "yes", to avoid
infinite (until the test session ends) loop if token extension failed.

Removed two helpers from FBTests that caused more harm than good because they made it too easy to write tests
that didn't wait for blockers, giving nondeterministic results as whether a test succeeded or failed depended
on timing patterns.

Test Plan:
- Ran unit tests, all passed under 4.3, 5.0, 5.1
- Ran Scrumptious, worked correctly

Revert Plan:

Reviewers: jacl, mmarucheck, gregschechte, ayden

Reviewed By: jacl

CC: platform-diffs@lists

Differential Revision: https://phabricator.fb.com/D495517

Task ID: 1125562, 1125563
  • Loading branch information...
1 parent 3d0742e commit 88cfcd8511b83b3090e6aa5439f0c5d0798c7a57 @clang13 clang13 committed Jun 13, 2012
@@ -473,7 +473,8 @@ - (NSMutableURLRequest *)requestWithBatch:(NSArray *)requests
cachePolicy:NSURLRequestReloadIgnoringLocalCacheData
timeoutInterval:timeout];
- NSString *httpMethod = metadata.request.HTTPMethod;
+ // HTTP methods are case-sensitive; be helpful in case someone provided a mixed case one.
+ NSString *httpMethod = [metadata.request.HTTPMethod uppercaseString];
[request setHTTPMethod:httpMethod];
[self appendAttachments:metadata.request.parameters
toBody:body
@@ -1005,7 +1006,7 @@ - (id)parseJSONOrOtherwise:(NSString *)utf8
utf8, FBNonJSONResponseProperty,
nil];
NSString *jsonrep = [parser stringWithObject:original];
- NSError *reparseError;
+ NSError *reparseError = nil;
parsed = [parser objectWithString:jsonrep error:&reparseError];
if (!reparseError) {
*error = nil;
@@ -18,7 +18,8 @@
@interface FBTestSession (Internal)
-// Can be used during testing to force a request for an access token refresh.
+// Can be used during testing to force a request for an access token refresh. This affects only the next
+// connection, when this flag is reset.
@property (readwrite) BOOL forceAccessTokenRefresh;
@end
View
@@ -459,7 +459,10 @@ - (void)authorizeWithPermissions:(NSArray*)permissions
}
- (BOOL)shouldExtendAccessToken {
- return self.forceAccessTokenRefresh || [super shouldExtendAccessToken];
+ // Note: we reset the flag each time we are queried. Tests should set it as needed for more complicated logic.
+ BOOL extend = self.forceAccessTokenRefresh || [super shouldExtendAccessToken];
+ self.forceAccessTokenRefresh = NO;
+ return extend;
}
#pragma mark -
@@ -44,9 +44,9 @@ - (void)testBatchingTwoSearches
session:self.defaultTestSession];
FBRequestConnection *connection = [[FBRequestConnection alloc] init];
- __block FBTestBlocker *blocker = [[FBTestBlocker alloc] init];
+ __block FBTestBlocker *blocker = [[FBTestBlocker alloc] initWithExpectedSignalCount:2];
- [connection addRequest:request1 completionHandler:[self handlerExpectingSuccess]];
+ [connection addRequest:request1 completionHandler:[self handlerExpectingSuccessSignaling:blocker]];
[connection addRequest:request2 completionHandler:[self handlerExpectingSuccessSignaling:blocker]];
[connection start];
@@ -207,7 +207,7 @@ - (void)testMixedSuccessAndFailure
for (int i = 0; i < kNumRequests; ++i) {
BOOL success = (i % 2) == 1;
- FBRequest *request = [FBRequest requestForGraphPath:success ? @"4" : @"-1"
+ FBRequest *request = [FBRequest requestForGraphPath:success ? @"me" : @"-1"
session:self.defaultTestSession];
[connection addRequest:request
completionHandler:success ?
@@ -87,7 +87,7 @@ - (void)testRequestPlaceSearchWithNoSearchText
{
CLLocationCoordinate2D coordinate = CLLocationCoordinate2DMake(38.889468, -77.03524);
FBRequest *searchRequest = [FBRequest requestForPlacesSearchAtCoordinate:coordinate
- radiusInMeters:100
+ radiusInMeters:200
resultsLimit:5
searchText:nil
session:self.defaultTestSession];
@@ -117,7 +117,7 @@ - (void)testRequestPlaceSearchWithSearchText
{
CLLocationCoordinate2D coordinate = CLLocationCoordinate2DMake(38.889468, -77.03524);
FBRequest *searchRequest = [FBRequest requestForPlacesSearchAtCoordinate:coordinate
- radiusInMeters:100
+ radiusInMeters:200
resultsLimit:5
searchText:@"Lincoln Memorial"
session:self.defaultTestSession];
@@ -234,6 +234,30 @@ - (void)traverseGraphObject:(id)graphObject
}
}
+- (void)testSimpleGraphGet
+{
+ __block FBTestBlocker *blocker = [[FBTestBlocker alloc] init];
+ [FBRequest startWithSession:self.defaultTestSession
+ graphPath:@"TourEiffel"
+ completionHandler:[self handlerExpectingSuccessSignaling:blocker]];
+
+ [blocker wait];
+ [blocker release];
+
+}
+
+- (void)testSimpleGraphGetWithExpectedFailure
+{
+ __block FBTestBlocker *blocker = [[FBTestBlocker alloc] init];
+ [FBRequest startWithSession:self.defaultTestSession
+ graphPath:@"-1"
+ completionHandler:[self handlerExpectingFailureSignaling:blocker]];
+
+ [blocker wait];
+ [blocker release];
+
+}
+
- (void)testFastEnumeration
{
id graphObject = [self graphObjectWithUnwrappedData];
@@ -84,7 +84,7 @@ - (void)testWillNotPiggybackIfWouldExceedBatchSize
// Minimize traffic by just getting our id.
[request.parameters setObject:@"id" forKey:@"fields"];
- [connection addRequest:request completionHandler:self.handlerExpectingSuccess];
+ [connection addRequest:request completionHandler:[self handlerExpectingSuccessSignaling:nil]];
}
[connection start];
// We don't need to wait for the requests to complete to determine success.
@@ -106,7 +106,6 @@ - (void)testCachedRequests
FBTestBlocker *blocker = [[FBTestBlocker alloc] init];
FBTestSession *session = [self getSessionWithSharedUserWithPermissions:nil];
- session.forceAccessTokenRefresh = YES;
// here we just want to seed the cache, by identifying the cache, and by choosing not to consult the cache
FBRequestConnection *connection = [[FBRequestConnection alloc] init];
@@ -152,7 +151,6 @@ - (void)testDelete
FBTestBlocker *blocker = [[[FBTestBlocker alloc] initWithExpectedSignalCount:3] autorelease];
FBTestSession *session = [self getSessionWithSharedUserWithPermissions:nil];
- session.forceAccessTokenRefresh = YES;
FBRequest *request = [FBRequest requestForGraphPath:@"me/feed"
session:session];
@@ -168,8 +166,10 @@ - (void)testDelete
NSMutableArray *fbids = [NSMutableArray array];
FBRequestHandler handler = ^(FBRequestConnection *connection, id<FBGraphObject> result, NSError *error) {
- STAssertNotNil(result, @"should have a result here");
- STAssertNil(error, @"should not have an error here");
+ // There's a lot going on in this test. To make failures easier to understand, giving each
+ // of the handlers a number so we can tell what failed.
+ STAssertNotNil(result, @"should have a result here: Handler 1");
+ STAssertNil(error, @"should not have an error here: Handler 1");
[fbids addObject: [[result objectForKey:@"id"] description]];
[blocker signal];
};
@@ -183,6 +183,12 @@ - (void)testDelete
[blocker wait];
+ if (fbids.count != 3) {
+ STAssertTrue(fbids.count == 3, @"wrong number of fbids, aborting test");
+ // Things are bad. Continuing isn't going to make them better, and might throw exceptions.
+ return;
+ }
+
blocker = [[FBTestBlocker alloc] initWithExpectedSignalCount:3];
connection = [[[FBRequestConnection alloc] init] autorelease];
@@ -192,8 +198,9 @@ - (void)testDelete
HTTPMethod:@"delete"];
[connection addRequest:deleteRequest
completionHandler:^(FBRequestConnection *connection, id result, NSError *error) {
- STAssertNotNil(result, @"should have a result here");
- STAssertNil(error, @"should not have an error here");
+ STAssertNotNil(result, @"should have a result here: Handler 2");
+ STAssertNil(error, @"should not have an error here: Handler 2");
+ STAssertTrue(0 != fbids.count, @"not enough fbids: Handler 2");
[fbids removeObjectAtIndex:fbids.count-1];
[blocker signal];
}];
@@ -206,23 +213,31 @@ - (void)testDelete
HTTPMethod:nil];
[connection addRequest:deleteRequest
completionHandler:^(FBRequestConnection *connection, id result, NSError *error) {
- STAssertNotNil(result, @"should have a result here");
- STAssertNil(error, @"should not have an error here");
+ STAssertNotNil(result, @"should have a result here: Handler 3");
+ STAssertNil(error, @"should not have an error here: Handler 3");
+ STAssertTrue(0 != fbids.count, @"not enough fbids: Handler 3");
[fbids removeObjectAtIndex:fbids.count-1];
[blocker signal];
}];
[connection addRequest:request completionHandler:^(FBRequestConnection *connection, id result, NSError *error) {
- STAssertNotNil(result, @"should have a result here");
- STAssertNil(error, @"should not have an error here");
- [fbids addObject: [[result objectForKey:@"id"] description]];
+ STAssertNotNil(result, @"should have a result here: Handler 4");
+ STAssertNil(error, @"should not have an error here: Handler 4");
+ if (result) {
+ [fbids addObject: [[result objectForKey:@"id"] description]];
+ }
[blocker signal];
}];
// these deletes two and adds one
[connection start];
[blocker wait];
+ if (fbids.count != 2) {
+ STAssertTrue(fbids.count == 2, @"wrong number of fbids, aborting test");
+ // Things are bad. Continuing isn't going to make them better, and might throw exceptions.
+ return;
+ }
blocker = [[[FBTestBlocker alloc] initWithExpectedSignalCount:2] autorelease];
@@ -234,8 +249,9 @@ - (void)testDelete
nil]
HTTPMethod:nil
completionHandler:^(FBRequestConnection *connection, id result, NSError *error) {
- STAssertNotNil(result, @"should have a result here");
- STAssertNil(error, @"should not have an error here");
+ STAssertNotNil(result, @"should have a result here: Handler 5");
+ STAssertNil(error, @"should not have an error here: Handler 5");
+ STAssertTrue(0 != fbids.count, @"not enough fbids: Handler 5");
[fbids removeObjectAtIndex:fbids.count-1];
[blocker signal];
}];
@@ -246,8 +262,9 @@ - (void)testDelete
parameters:nil
HTTPMethod:@"delete"
completionHandler:^(FBRequestConnection *connection, id result, NSError *error) {
- STAssertNotNil(result, @"should have a result here");
- STAssertNil(error, @"should not have an error here");
+ STAssertNotNil(result, @"should have a result here: Handler 6");
+ STAssertNil(error, @"should not have an error here: Handler 6");
+ STAssertTrue(0 != fbids.count, @"not enough fbids: Handler 6");
[fbids removeObjectAtIndex:fbids.count-1];
[blocker signal];
}];
View
@@ -46,10 +46,7 @@
@property (readonly, retain) FBTestSession *defaultTestSession;
- (FBRequestHandler)handlerExpectingSuccessSignaling:(FBTestBlocker*)blocker;
-- (FBRequestHandler)handlerExpectingSuccess;
-
- (FBRequestHandler)handlerExpectingFailureSignaling:(FBTestBlocker*)blocker;
-- (FBRequestHandler)handlerExpectingFailure;
- (FBTestSession *)getSessionWithSharedUserWithPermissions:(NSArray*)permissions;
- (FBTestSession *)getSessionWithSharedUserWithPermissions:(NSArray*)permissions
View
@@ -265,10 +265,6 @@ - (id)batchedPostAndGetWithSession:(FBSession*)session
#pragma mark -
#pragma mark Handlers
-- (FBRequestHandler)handlerExpectingSuccess {
- return [self handlerExpectingSuccessSignaling:nil];
-}
-
- (FBRequestHandler)handlerExpectingSuccessSignaling:(FBTestBlocker*)blocker {
FBRequestHandler handler =
^(FBRequestConnection *connection, id result, NSError *error) {
@@ -279,10 +275,6 @@ - (FBRequestHandler)handlerExpectingSuccessSignaling:(FBTestBlocker*)blocker {
return [[handler copy] autorelease];
}
-- (FBRequestHandler)handlerExpectingFailure {
- return [self handlerExpectingFailureSignaling:nil];
-}
-
- (FBRequestHandler)handlerExpectingFailureSignaling:(FBTestBlocker*)blocker {
FBRequestHandler handler =
^(FBRequestConnection *connection, id result, NSError *error) {

0 comments on commit 88cfcd8

Please sign in to comment.