Moya: Provide sampleData in Provider instead of TargetType

[tbaranes]

I'd like to bring back an old subject: #425, I know it has been closed but since there was a solution suggested, I think it could be worth a second though!

In the current state of Moya, everyone has to implement sampleData in the TargetType. It's great to make testing APIs easy, but I don't think it's right to have to embed the stubbed response directly in the app instead of having them only in the tests target.

I know there was a few potential workaround for this in #425, but they all comes with downsides: won't work anymore on simulators, breaks the target types basic flow... In the end, I completely agree with Hirad's solution: adding a sampleData response to StubClosure, and removing sampleData from TargetType.

One downside that Ash brought up: it won't encourage people to add tests, but I do think, if people doesn't want to test, they will just return Data(). So, in the end, I see more advantages of this implementation than the current one (except that it's introducing a breaking change).

Could it become a reality soon? 🙃

[BasThomas]

When doing a search, it does seem like a lot of people just return Data(): https://github.com/search?q=%22var+sampledata%22+language:Swift+extension:.swift&type=Code&utf8=%E2%9C%93.

On it being a breaking change: we're releasing 9.0 soon, and with Swift 4 around the corner, there will probably be room to include this in 10.0 in the near future if we want to go this way.

[SD10]

I completely agree with @tbaranes. I find the inclusion of testing code directly in the app unpalatable. As someone who is passionate about testing, I would like to have this separation of concerns.

I don't really believe testing is a practice that can be encouraged through an API. If one does not see the benefit of or appreciate writing tests, they are simply are not going to do so.

Furthermore, from the perspective of responsibility, I think it makes far more sense for sampleData to be the concern of the Provider as opposed to the TargetType.

[BasThomas]

What do other @Moya/contributors think?

[colinta]

I was, for a long time, of the "let's encourage testing" mindset, but these days I'm leaning more towards having a good separation of concerns, and hoping people choose to take the time to have stubbed responses (but it's up to them).

I think including it in the provider makes a ton of sense, I'm on board with this change.

[ashfurrow]

Yup, makes sense to me.

[Jeehut]

I'm always trying to test my apps, and I usually do. But I have never used the sampleData for that. If I want to test parsing JSON data into my Swift objects, I write specific tests for that – it's part of the model then, though. If I want to test that the API answers the way I expect it to, sampleData doesn't help with that either.

I've never found good usage for the sampleData anyways, so nowadays I'm actually defaulting to return Data(), too. Maybe I missed something about stubbing, but from my point of view other tests are much more useful, so I'd welcome this change!

[pedrovereza]

Sounds good to me.

If we decide to make the change, I think we should make it part of 9.0 (even if it delays the release a bit), since we already included breaking changes in TargetType.

[Jeehut]

By the way, why are we not using the GitHub Projects feature to manage what goes into our next releases? That would give us a good overview and prevent using a PR like https://github.com/Moya/Moya/pull/1124 for that. Or are there any concerns?

[pedrovereza]

@Dschee We are using milestones to keep track, but I'm not sure of the benefits GitHub Projects would give us (feels more like a kanban thing).

We are using branch 9.0.0-dev for breaking changes going out in 9.0, and we basically rebase the changes on top of master from time to time, which will make the merge from 9.0.0-dev to master easier when we decide to release it. PR #1124 was opened just to let more people double check that the rebase was good before pushing it to 9.0.0-dev.

We should probably continue this discussion in a separate issue (if needed) so we don't hijack this one 😅

[Jeehut]

Ahh, I overlooked the milestones. That's a good alternative, of course.

To keep the discussion to this topic: Could we add this issue to the milestone 9.0.0 board until it proves to be more complicated than it currently looks? Cause this change doesn't look overly complex to me. 😄

[BasThomas]

Anyone up for a PR?

[SD10]

I tried taking a crack at it. If we use the approach of adding a response to StubClosure:

-typealias StubClosure = TargetType -> Moya.StubBehavior
+typealias StubClosure = TargetType -> (Moya.StubBehavior, Response)

This requires that StubClosure is also passed to the method defaultEndpointMapping(target:).

This is because defaultEndpointMapping(target:) is a class method so I won't have access to the stubClosure instance property to get the data for the sampleResponseClosure. It is currently just receiving the sampleData from the TargetType.

I think this is the only thing a little awkward about this approach.

Another thing to point out, what I didn't like is how it broke a lot of the tests that were written without the use of a MoyaProvider. If a test is written using just an Endpoint then we have no way to retrieve sampleData because the current implementation gets it from the TargetType.

I would have to create some sort of namespace in my TestTarget to store all the sampleData in order to remove the resulting boilerplate code.

Any insight, suggestions, and corrections are appreciated 😅

[SD10]

What about providing a default implementation for sampleData in TargetType:

extension TargetType {
   var sampleData: Data {
      return Data()
   }
}

I think this is the implementation that most people who are not using testing are doing already:
Why not make it default?

Then in the Test Target people can provide a new implementation for sampleData:

// Example TargetType used in App
enum MyAppTargets: TargetType {
    case zen
}

// Test Target Implementation
extension MyAppTargets {
    var sampleData: Data {
        // return sample data for testing
    }
}

I think this is a far easier way to remove test code from the Main Target and less damaging to the API. However, if we're concerned about moving sampleData to the provider from a responsibility perspective then this is not a valid solution.

[pedrovereza]

@SD10 We've had conversations about providing default implementations for common things, but in the end we agreed that, as a rule of thumb, we'd rather have the user explicity define (and understand) the behavior instead of relying on default implementations provided by us.

More in https://github.com/Moya/Moya/pull/1067#issuecomment-298128123 and #861 😉

[SD10]

@pedrovereza I don't know if this is a hard rule but if Moya is no longer going to force people to write networking tests then this seems like a good case for a default implementation.

I'd also like to point out the contrast that the MoyaProvider has the default behavior of StubBehavior.never -- it makes sense to default sampleData to Data() to match this.

From #861 this is an excerpt by @scottrhoyt:

As for sampleData, even as an ardent unit tester, I think there is a difference between first-class support of unit testing and first-class forcing people to write testing code. We all know very few projects get started with full test suites in place. If tests do eventually get added, it is often done at a later date (i.e. not much TDD going on). The amount of Moya code that starts off with

var sampleData: Data {
  return Data()
}

considerably outnumbers the amount of code that is using sampleData in it's intended fashion. This search backs that up at least for public repos. By adding even a little bit of friction to selecting Moya as your networking layer because of it's verbosity and forcing of unit testing code into your app, we just encourage people to go a different route that will be harder for them to add unit tests once they get to the point that they realize they need it. While it is admirable to say that we can change testing etiquette by forcing people to implement sampleData, the reality is that it's still just a--confusing for anyone that isn't familiar with Moya--Data() away from being ignored and that is what most people are doing. Changing testing behavior will require a bigger impetus than a single networking library, but every single networking library should still support it in some way. Frankly I think Moya should go further to support testing by providing an easy way for test data to be stored in fixtures because if you want to use real returns from even a moderate-complexity API, the TargetType code quickly gets bloated back to unreadable status.

It's a large read but my takes from it are:

• Forcing people to implement sampleData makes it harder to get Moya up and running
• sampleData is required prematurely (long before unit tests are written)
• Having sampleData in the TargetType bloats the file, especially with a complex API

[SD10]

I've also been considering the possibility of a TestTargetType protocol:

protocol TestTargetType: TargetType {
    var sampleData: Data { get }
    var stubBehavior: Moya.StubBehavior { get }
}

Moya users can then conform to this protocol inside of their testing target. This adds the benefits of:

• moving sampleData to the Test Target
• allows more fine grain control over stubBehavior eg.) stubBehavior based on specific target case

Moya provider could then route a request based on whether a Target is a TestTargetType:

extension MoyaProvider where Target: TestTargetType {

    func requestNormal(_ target: Target, queue: DispatchQueue?, progress: Moya.ProgressBlock?, completion: @escaping Moya.Completion) -> Cancellable {
        // Return stubbed response
    }
}

This could even improve the MoyaProvider because if the target is not a TestTargetType then the provider knows it doesn't have to handle anything regarding stubbing. You could remove something like the StubClosure from the initializer of the MoyaProvider as well.

[pedrovereza]

@SD10

It's a large read but my takes from it are:
Forcing people to implement sampleData makes it harder to get Moya up and running
sampleData is required prematurely (long before unit tests are written)
Having sampleData in the TargetType bloats the file, especially with a complex API

I agree with all these points, but I don't think that defaulting sampleData is the best solution. I think that it should be removed from TargetType completely :wink:

[minuscorp]

@SD10 I completely agree with your proposal of adding a separate protocol that we can extend from our TargetType to provide the sampleData and other testing behavior. In production (mainly with heavy data exchange applications), provide those, for example, json files into the application and adding MB of useless code is a heavy drawback... For now I have to provide a Data() default to sampleData to avoid this, but I want to test it as well...