@alloy => Initial work on turning on OHHTTP stubs only #575

Merged
merged 10 commits into from Jul 24, 2015

Conversation

Projects
None yet
3 participants
@orta
Member

orta commented Jul 3, 2015

Alright, so.

The aim of this PR was to initially build out AROHHTTPNoStubAssertionBotand to turn on forced exceptions around networking.I started this, but once I had got deep enough into the PR I began to realise that this is still a flawed approach,

The problem lay in that it was difficult to loop back from the Exception raised by the networking API to the bit of code that caused it. I concluded that this wasn't going to be fun, and wouldn't make life easier in the future, which was the entire point.

So I started wondering what it would take to use some of the techniques I requested in Moya within Eigen. The notable one being stubs as first class citizens. We now have synchronous networking in tests.

This is the stack trace for a simple networking call:
screen shot 2015-07-06 at 15 17 57

And here's a complicated one:
screen shot 2015-07-06 at 16 55 39

You can now determine exactly what test is making what networking request. Note this only affects things going through the API.

The API client will log out example OHHTTP code if you don't have a stub for some networking that happens inside the app. I opted against making it an exception, it's really obvious when you see it.

@@ -6,7 +6,7 @@ @implementation ARTestHelper
+ (void)load;
{
NSOperatingSystemVersion version = [NSProcessInfo processInfo].operatingSystemVersion;
- NSAssert(version.majorVersion == 8 && version.minorVersion == 3,
+ NSAssert(version.majorVersion == 9 && version.minorVersion == 0,

This comment has been minimized.

@orta

orta Jul 3, 2015

Member

oh :-/

@orta

orta Jul 3, 2015

Member

oh :-/

This comment has been minimized.

@alloy

alloy Jul 3, 2015

Member

¿Que es la problema?

@alloy

alloy Jul 3, 2015

Member

¿Que es la problema?

@@ -1,3 +1,5 @@
+// MARK: Formatter Exempt

This comment has been minimized.

@orta

orta Jul 7, 2015

Member

This means that space-commander won't break our dictionaries, and this file is only a thousand line dict ;)

@orta

orta Jul 7, 2015

Member

This means that space-commander won't break our dictionaries, and this file is only a thousand line dict ;)

@@ -104,7 +104,7 @@ - (BOOL)application:(UIApplication *)application willFinishLaunchingWithOptions:
- (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(NSDictionary *)launchOptions
{
- _landingURLRepresentation = self.landingURLRepresentation ?: @"http://artsy.net";
+ _landingURLRepresentation = self.landingURLRepresentation ?: @"https://artsy.net";

This comment has been minimized.

@orta

orta Jul 7, 2015

Member

iOS9 complains now.

@orta

orta Jul 7, 2015

Member

iOS9 complains now.

This comment has been minimized.

@alloy

alloy Jul 8, 2015

Member

iOS 9 don’t like security?

@alloy

alloy Jul 8, 2015

Member

iOS 9 don’t like security?

This comment has been minimized.

@orta

orta Jul 8, 2015

Member

iOS9 more or less complains when you make non-HTTPS requests.
See number 1 on here: https://github.com/ChenYilong/iOS9AdaptationTips

@orta

orta Jul 8, 2015

Member

iOS9 more or less complains when you make non-HTTPS requests.
See number 1 on here: https://github.com/ChenYilong/iOS9AdaptationTips

@@ -8,36 +9,165 @@ @implementation ArtsyAPI
+ (AFJSONRequestOperation *)performRequest:(NSURLRequest *)request success:(void (^)(id))success failure:(void (^)(NSURLRequest *request, NSHTTPURLResponse *response, NSError *error))failure

This comment has been minimized.

@orta

orta Jul 7, 2015

Member

previously theses were all going to class methods to do their work, now they go to a shared instance which could either be an ArtsyAPI or ArtsyOHHTTPAPI

@orta

orta Jul 7, 2015

Member

previously theses were all going to class methods to do their work, now they go to a shared instance which could either be an ArtsyAPI or ArtsyOHHTTPAPI

This comment has been minimized.

@orta

orta Jul 7, 2015

Member

previously theses were all going to class methods to do their work, now they go to a shared instance which could either be an ArtsyAPI or ArtsyOHHTTPAPI

@orta

orta Jul 7, 2015

Member

previously theses were all going to class methods to do their work, now they go to a shared instance which could either be an ArtsyAPI or ArtsyOHHTTPAPI

@@ -49,7 +179,7 @@ + (AFJSONRequestOperation *)getRequest:(NSURLRequest *)request parseIntoAClass:(
object = [klass modelWithJSON:jsonDictionary error:nil];
}
if (success) {
- dispatch_async(dispatch_get_main_queue(), ^{
+ ar_dispatch_main_queue(^{

This comment has been minimized.

@orta

orta Jul 7, 2015

Member

these are the only real changes to the file, aside from moving to instance methods

@orta

orta Jul 7, 2015

Member

these are the only real changes to the file, aside from moving to instance methods

@@ -1,5 +1,6 @@
+

This comment has been minimized.

@orta

orta Jul 7, 2015

Member

I must a missed a bunch of these on my first run

@orta

orta Jul 7, 2015

Member

I must a missed a bunch of these on my first run

-@interface ARArtistNetworkModel : NSObject

This comment has been minimized.

@orta

orta Jul 7, 2015

Member

from a class, to a protocol with two objects corresponding to it

@orta

orta Jul 7, 2015

Member

from a class, to a protocol with two objects corresponding to it

[self setStatusWithTitle:@"Sending…" body:@""];
+ [self sendInquiry];

This comment has been minimized.

@orta

orta Jul 7, 2015

Member

assumed async-ness.
It would do the entire inquiry stuff, then set it to sending afterwards when sync.

@orta

orta Jul 7, 2015

Member

assumed async-ness.
It would do the entire inquiry stuff, then set it to sending afterwards when sync.

@@ -571,7 +571,7 @@ - (void)getRelatedArtists
- (void)getRelatedPosts
{
@weakify(self);
- [self.artist getRelatedPosts:^(NSArray *posts) {
+ [self.networkModel getRelatedPosts:^(NSArray *posts) {

This comment has been minimized.

@orta

orta Jul 7, 2015

Member

just moving responsibility for networking away from the artist within this context

@orta

orta Jul 7, 2015

Member

just moving responsibility for networking away from the artist within this context

@@ -59,9 +63,6 @@ - (void)viewDidLoad
} failure:^(NSError *error) {
ARErrorLog(@"Error getting Featured Link Categories for genes");
}];
-
- [self ar_presentIndeterminateLoadingIndicatorAnimated:self.shouldAnimate];
- [super viewDidLoad];

This comment has been minimized.

@orta

orta Jul 7, 2015

Member

assumed async-ness, would always show a loader cause it was the last thing.

@orta

orta Jul 7, 2015

Member

assumed async-ness, would always show a loader cause it was the last thing.

@@ -290,7 +290,7 @@ - (void)getNextItemSet
return;
};
@weakify(self);
- dispatch_async(self.artworkPageQueue, ^{
+ ar_dispatch_on_queue(self.artworkPageQueue, ^{

This comment has been minimized.

@orta

orta Jul 7, 2015

Member

makes it sync in tests

@orta

orta Jul 7, 2015

Member

makes it sync in tests

@@ -1,63 +0,0 @@
-#import "AROHHTTPNoStubAssertionBot.h"

This comment has been minimized.

@orta

orta Jul 7, 2015

Member

Byeeeeeeee

@orta

orta Jul 7, 2015

Member

Byeeeeeeee

Artsy_Tests/Extensions/ArtsyOHHTTPAPI.m
+ printf("Add a breakpoint in ArtsyOHHTTPAPI.m or look above for more info. \n\n\n");
+ }
+
+ return [super requestOperation:request success:success failure:failureCallback];

This comment has been minimized.

@orta

orta Jul 7, 2015

Member

just performs it async, as though nothing was different

@orta

orta Jul 7, 2015

Member

just performs it async, as though nothing was different

+
+ NSHTTPURLResponse *URLresponse = [[NSHTTPURLResponse alloc] initWithURL:request.URL statusCode:response.statusCode HTTPVersion:@"1.0" headerFields:response.httpHeaders];
+
+ if (response.statusCode >= 200 && response.statusCode < 205) {

This comment has been minimized.

@orta

orta Jul 7, 2015

Member

I couldn't find anything other than 200s for positives within the codebase, but figured it was better to do it right

@orta

orta Jul 7, 2015

Member

I couldn't find anything other than 200s for positives within the codebase, but figured it was better to do it right

+@end
+
+
+@interface ARFakeAFJSONOperation : NSBlockOperation

This comment has been minimized.

@orta

orta Jul 7, 2015

Member

We want to pretend that this block operation is a AFJSONOperation which means replicating the bits of it's API we use.

@orta

orta Jul 7, 2015

Member

We want to pretend that this block operation is a AFJSONOperation which means replicating the bits of it's API we use.

@@ -1,6 +1,6 @@
#import "Sale+Extensions.h"
#import "Bid+Extensions.h"
#import "SaleArtwork+Extensions.h"
-#import "OHHTTPStubs+JSON.h"
+#import "OHHTTPStubs+Artsy.h"

This comment has been minimized.

@orta

orta Jul 7, 2015

Member

scope changed to include images, I use this in Folio, but don't think I'm using it in here.

@orta

orta Jul 7, 2015

Member

scope changed to include images, I use this in Folio, but don't think I'm using it in here.

@@ -16,7 +16,7 @@ @interface ARMessageItemProvider (Testing)
__block ARMessageItemProvider *provider;
__block NSString *placeHolderMessage = @"So And So";
__block NSString *path = @"artist/so-and-so";
- __block UIActivityViewController *activityVC = [[UIActivityViewController alloc] init];
+ __block UIActivityViewController *activityVC = [[UIActivityViewController alloc] initWithActivityItems:@[] applicationActivities:@[]];

This comment has been minimized.

@orta

orta Jul 7, 2015

Member

iOS9 API

@orta

orta Jul 7, 2015

Member

iOS9 API

it(@"does not add a section with other works by the same artist if the artwork has no associated artist", ^{
relatedView.artwork.artist = nil;
[relatedView addSectionsForShow:show];
- expect([relatedView viewWithTag:ARRelatedArtworksArtistArtworks]).will.beNil();
+ expect([relatedView viewWithTag:ARRelatedArtworksArtistArtworks]).to.beNil();
});

This comment has been minimized.

@orta

orta Jul 7, 2015

Member

I'm kinda surprised this test worked before...

The before block below would run it with addSectionsForShow when there was an artist, then eventually it would get rid of it. I've made that before block not get run now.

@orta

orta Jul 7, 2015

Member

I'm kinda surprised this test worked before...

The before block below would run it with addSectionsForShow when there was an artist, then eventually it would get rid of it. I've made that before block not get run now.

This comment has been minimized.

@alloy

alloy Jul 8, 2015

Member

Who says it worked? 😝

I think this was one of the flakier ones.

@alloy

alloy Jul 8, 2015

Member

Who says it worked? 😝

I think this was one of the flakier ones.

+@end
+
+
+@interface OHHTTPStubs (PrivateButIKnowTheCreatorAndItsAllOkay)

This comment has been minimized.

@AliSoftware

AliSoftware Jul 7, 2015

😄 love that category name 👋

@AliSoftware

AliSoftware Jul 7, 2015

😄 love that category name 👋

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jul 8, 2015

Member

The API client will log out example OHHTTP code if you don't have a stub for some networking that happens inside the app.

Oh yeah, epic win 💯 👍

I opted against making it an exception, it's really obvious when you see it.

One thing you could do is in the printed message include a function that people can break on, like is done with unsatisfiable AutoLayout constraints.

Member

alloy commented Jul 8, 2015

The API client will log out example OHHTTP code if you don't have a stub for some networking that happens inside the app.

Oh yeah, epic win 💯 👍

I opted against making it an exception, it's really obvious when you see it.

One thing you could do is in the printed message include a function that people can break on, like is done with unsatisfiable AutoLayout constraints.

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Jul 8, 2015

Member

Right now it tells you to do this: printf("Add a breakpoint in ArtsyOHHTTPAPI.m or look above for more info. \n\n\n");

I think only you add them via magic

Member

orta commented Jul 8, 2015

Right now it tells you to do this: printf("Add a breakpoint in ArtsyOHHTTPAPI.m or look above for more info. \n\n\n");

I think only you add them via magic

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jul 8, 2015

Member

I’m luvin’ this, very well done, sir 👍

I get that you’re not raising an exception atm, but why not have the printed message include a filtered stack trace with the util I made for you? (I always dislike having to run something again unless really really needed.)

Member

alloy commented Jul 8, 2015

I’m luvin’ this, very well done, sir 👍

I get that you’re not raising an exception atm, but why not have the printed message include a filtered stack trace with the util I made for you? (I always dislike having to run something again unless really really needed.)

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jul 8, 2015

Member

I’m gonna hold off with merging this until all normal work is done, just to be sure that I don’t loose any time anymore this week.

Member

alloy commented Jul 8, 2015

I’m gonna hold off with merging this until all normal work is done, just to be sure that I don’t loose any time anymore this week.

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Jul 8, 2015

Member

Oh, nice, stack trace idea = 👍

Member

orta commented Jul 8, 2015

Oh, nice, stack trace idea = 👍

Artsy/Networking/ArtsyAPI.m
+ static dispatch_once_t oncePredicate;
+ dispatch_once(&oncePredicate, ^{
+ if ([[ARDispatchManager sharedManager] useSyncronousDispatches]) {
+ Class klass = NSClassFromString(@"ArtsyOHHTTPAPI");

This comment has been minimized.

@alloy

alloy Jul 15, 2015

Member

Why not just feature check for the class and then just use it? It should only be available during a test run, right?

@alloy

alloy Jul 15, 2015

Member

Why not just feature check for the class and then just use it? It should only be available during a test run, right?

This comment has been minimized.

@orta

orta Jul 15, 2015

Member

Yeah, it will be 💯

@orta

orta Jul 15, 2015

Member

Yeah, it will be 💯

@orta orta referenced this pull request Jul 20, 2015

Merged

@alloy => Add Adjust to Eigen #666

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jul 23, 2015

Member

I rebased this branch and am only seeing the 6 failures that are currently on master as well. Will merge this after some last-minute adhoc fixing on the App Store release.

Member

alloy commented Jul 23, 2015

I rebased this branch and am only seeing the 6 failures that are currently on master as well. Will merge this after some last-minute adhoc fixing on the App Store release.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jul 24, 2015

Member

The only failures are the ones that @1aurabrown is still working on, so MERGE TIME!

Member

alloy commented Jul 24, 2015

The only failures are the ones that @1aurabrown is still working on, so MERGE TIME!

alloy added a commit that referenced this pull request Jul 24, 2015

Merge pull request #575 from artsy/orta-http
@alloy => Initial work on turning on OHHTTP stubs only

@alloy alloy merged commit 9ccfc05 into master Jul 24, 2015

1 check failed

ci/circleci Your tests failed
Details

@alloy alloy deleted the orta-http branch Jul 24, 2015

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Jul 24, 2015

Member

nice!

Member

orta commented on 3af9bcb Jul 24, 2015

nice!

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jul 24, 2015

Member

Yeah it’s something we definitely need to look at at some point, but for now the test output is really suffering from this.

Member

alloy replied Jul 24, 2015

Yeah it’s something we definitely need to look at at some point, but for now the test output is really suffering from this.

This comment has been minimized.

Show comment
Hide comment
@orta

orta Jul 24, 2015

Member

I had wondered about a similar attack of making tests fail when this happens

Member

orta replied Jul 24, 2015

I had wondered about a similar attack of making tests fail when this happens

This comment has been minimized.

Show comment
Hide comment
@orta

orta Jul 24, 2015

Member

Its hard to apply now, but new apps would want this in from day 1

Member

orta replied Jul 24, 2015

Its hard to apply now, but new apps would want this in from day 1

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jul 24, 2015

Member

I think that’s a good idea to have for when we get to tackling it.

Member

alloy replied Jul 24, 2015

I think that’s a good idea to have for when we get to tackling it.

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Jul 24, 2015

Member

Aye

Member

alloy replied Jul 24, 2015

Aye

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment