Add onStubMissing to report missing stubs; detects "stale" stubs which can lead to intermittent failures

[c1ira]

Checklist

• I've checked that all new and existing tests pass
• I've updated the documentation if necessary
• I've added an entry in the CHANGELOG to credit myself

Description

Add onStubMissing to report missing stubs; detects "stale" stubs which can lead to intermittent failures.

Motivation and Context

We've been using a fork with this change at Capital One for several months. It's been a big help in solving previously mysterious test failures, where if a stub was missing a request would go to a live server with inconsistent results. It has also helped us visualize our API usage in a different way, for example noticing when a request is made only intermittently or under specific conditions.

Tested by... a) Using it at Capital One for several months, getting hit hundreds, if not thousands, of times per day. b) Ran existing unit tests, and they all passed. c) Did not add a unit test for this feature, since didn't see unit tests for the other debug blocks.

[AliSoftware]

Thanks for the PR, I'll look into it this weekend.

But were you aware that when you setup multiple stubs, they are always tested in the reverse order they were added, which means that you can start all you tests by first adding a "fallback stub" which always succeeds (condition always true) and fatalErrors or XCTFails, preventing such risk of requests leaking to the real network?

This is described in the wiki here:

This may also be used for a fallback stub, as the first stub being installed will only be called if every other stub fails.

Basically you just have to:

func setUp() {
  super.setUp()
  // Being the very first stub to be installed, this will be the last condition to be checked
  // So it will only be triggered if all other stubs added later failed to match their respective condition
  // And no other stub matched.
  stub(condition: { _ in return true }) { req in
    fatalError("You forgot to stub request \(req) and it almost hit the network. Please add a stub for it")
    // I used fatalError but maybe you prefer to just XCTFail, it's up to you
  }
  // And then install all your other stubs
}

If that is sufficient for your changes, then maybe this onStubMissing addition is in fact not needed and you can use that trick instead. But even if it's the case I'd still then love a PR on the documentation, as I think a lot of people don't know this trick or realize they can use it, so maybe we could expose it better in the documentation.

[c1ira]

Thanks, I'll try that.

[AliSoftware]

I actually forgot that we had that many debugging blocks already (I remembered about onStubActivation but I now realize we also added onStubRedirectBlock and afterStubFinishBlock which I forgot about).
So even if this feature of catching un-stubbed requests is already achievable today (by make the very first stub be a fallback with condition true), adding an explicit onStubMissing block like you did probably still makes sense (if not only for better discoverability) 👍

[c1ira]

Oh, okay, great.

[AliSoftware]

Hey @c1ira could you add a period + 2 spaces at the end of this line? It's a little trick in markdown so that once rendered, it inserts a new line inside the same paragraph (see other entries)

Also don't hesitate to add a link to that PR too (under the link to your name) while you're at it

[AliSoftware]

Changelog nitpicking + double-check if canInitRequest isn't called too many times for same request by the system

[AliSoftware]

Did you check that this is only called once? I remember when I worked on canInitWithRequest a while ago that iOS called that method more than once to test a single request.
I have no idea why it would call it multiple times while calling it only one should be enough for the system, but in practice, at least some iOS versions ago when I worked on this part of the code, putting a breakpoint there made it be triggered multiple times even for only one request, so better check in case it's still the case (especially with old iOS version, as maybe they've improved that since recent updates hopefully)

[mikelupo]

This may not hold water, but I have a vague memory that this was fixed in iOS 8.4??

-----Original Message----- From: AliSoftware <notifications@github.com> To: AliSoftware/OHHTTPStubs <OHHTTPStubs@noreply.github.com> Cc: Subscribed <subscribed@noreply.github.com> Sent: Mon, Oct 16, 2017 3:34 pm Subject: Re: [AliSoftware/OHHTTPStubs] Add onStubMissing to report missing stubs; detects "stale" stubs which can lead to intermittent failures (#264) @AliSoftware requested changes on this pull request. Changelog nitpicking + double-check if canInitRequest isn't called too many times for same request by the system In OHHTTPStubs/Sources/OHHTTPStubs.m:

@@ -337,7 +343,11 @@ @implementation OHHTTPStubsProtocol

+ (BOOL)canInitWithRequest:(NSURLRequest *)request { - return ([OHHTTPStubs.sharedInstance firstStubPassingTestForRequest:request] != nil); + BOOL found = ([OHHTTPStubs.sharedInstance firstStubPassingTestForRequest:request] != nil); + if (!found && OHHTTPStubs.sharedInstance.onStubMissingBlock) { + OHHTTPStubs.sharedInstance.onStubMissingBlock(request); Did you check that this is only called once? I remember when I worked on canInitWithRequest a while ago that iOS called that method more than once to test a single request. I have no idea why it would call it multiple times while calling it only one should be enough for the system, but in practice, at least some iOS versions ago when I worked on this part of the code, putting a breakpoint there made it be triggered multiple times even for only one request, so better check in case it's still the case (especially with old iOS version, as maybe they've improved that since recent updates hopefully) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[AliSoftware]

This may not hold water, but I have a vague memory that this was fixed in iOS 8.4??

🤔 Interesting, didn't remember they might have solved it. (Still worth testing again I guess?)

[c1ira]

I haven't noticed canInitRequest called repeatedly, and like I wrote, we've been using this for months. Also I worry it might do more harm than good to try to suppress that.

[AliSoftware]

Just waiting for CI queues before merging