[WIP] Refactoring Moya's stubbing API #1502

SD10 · 2017-12-12T00:10:00Z

Summary

This is purely experimental but I was dabbling with the idea of defining the stubbing API in terms of TestTargetType as opposed to configuring a MoyaProvider.

@Moya/contributors I would appreciate if any of you could review this and provide feedback 🙏

How?

We add a protocol that allows Moya users to provide the sample response and stub behavior:

protocol TestTargetType {
    var sampleResponse: EndpointSampleResponse { get }
    var stubBehavior: Moya.StubBehavior { get }
}

Then internally, we use a where clause on MoyaProvider to determine if the Target we are handling is capable of stubbing requests.

extension MoyaProvider where Target: TestTargetType {
/// ... stubbing methods here
}

It all comes down to this method:

func performRequest(_ target: Target, request: URLRequest, callbackQueue: DispatchQueue?, progress: Moya.ProgressBlock?, completion: @escaping Moya.Completion, endpoint: Endpoint<Target>) -> Cancellable

When Target != TestTargetType this version is called:

extension MoyaProvider {
    private func performRequest(_ target: Target, request: URLRequest, callbackQueue: DispatchQueue?, progress: Moya.ProgressBlock?, completion: @escaping Moya.Completion, endpoint: Endpoint<Target>) -> Cancellable {
        return self.performNormalRequest(target, request: request, callbackQueue: callbackQueue, progress: progress, completion: completion, endpoint: endpoint)
    }
}

When Target == TestTargetType this version is called:

extension MoyaProvider where Target: TestTargetType {
    private func performRequest(_ target: Target, request: URLRequest, callbackQueue: DispatchQueue?, progress: Moya.ProgressBlock?, completion: @escaping Moya.Completion, endpoint: Endpoint<Target>) -> Cancellable {
        switch target.stubBehavior {
        case .never:
            return self.performNormalRequest(target, request: request, callbackQueue: callbackQueue, progress: progress, completion: completion, endpoint: endpoint)
        default:
            return self.stubRequest(target, request: request, callbackQueue: callbackQueue, completion: completion, endpoint: endpoint, stubBehavior: target.stubBehavior)
        }
    }
}

Pros:

Removes stubClosure from MoyaProvider because you can control behavior in TestTargetType
Moves all stubbing behavior to MoyaProvider+Stubbing.swift
Providers not using a TestTargetType need not be concerned with anything related to stubbing and perform no checks regarding stubbing at runtime

Cons:

That's what I want to hear from you all 😅 I'm biased and blind to my own system

Final thoughts

This breaks a lot of tests, I have not fixed them because it will take some work and I didn't want to start if people think this may not be a good change for the project. However, I think that fixing those tests would help me better understand any drawbacks to this approach.

SD10 · 2017-12-12T05:15:46Z

Sources/Moya/MultiTarget.swift

+// TODO: - Not sure how this would affect sample data
+//    public var sampleData: Data {
+//        return target.sampleData
+//    }


This is also the only thing I haven't been able to get around with this PR. I don't use the MultiTarget so I don't really understand it's purpose.

Yeah, this is tricky. I will think about this one, but two ideas that come to my mind right now are:

Create two MultiTargets, one that stubs and one that does not (like with target types)

If target is not a Testing one, use empty data/crash, but this is not the best solution I think.

This PR is very low priority IMO. It won't make it in Moya 12.0 for sure and I'd like a few Moya members to discuss if they even like this change.

Revisited this today... I have a feeling it "just works" now that I understand what MultiTarget is doing.

If the multi-target type is a TestTargetType the user will have to specify where to get sampleData from.

Following the pattern of the existing properties, they would have to extract the associated value of the target case. Then they would have to cast to TestTargetType to access the underlying sampleData.

If casting fails they still have to return something because sampleData is non-optional. Really hard to go unnoticed unless you're already writing bad code to begin with.

So as the other properties are doing, they would unwrap the associated value of the enum and return the data from the nested target.

@SD10 Sorry, I'm not sure I follow... Are you saying that we won't provide any support for MultiTarget with TestTargetType so the users would have to do it themselves?

@sunshinejr I wouldn't say "we won't provide any support" but I was going to suggest that they extend MultiTarget to conform to TestTargetType inside their test target. We could add a MultiTestTarget if you believe this is expecting too much.

I guess the latter also has some added type safety, because we can ensure that the associated value in the enum case is a TestTargetType as well

@SD10 Oh, sorry if it sounded like that, I was just trying to understand if that's what you would do. I don't think there is a 100% good solution for this, MultiTestTarget is okay, but still doesn't cover all cases (like mixed targets & test targets). I wonder if making few examples instead would be enough... What do you think?

MultiTestTarget would cover all cases because the enum associated value would have to be TestTargetType as well.

An edge case would be a MultiTarget with an inner TestTargetType. Stubbing would be determined by the type of the root target, MultiTarget, so it would never stub.

We could document this. Even provide an assertion/error if the inner target is a TestTargetType but MultiTarget is not? The only way to fully prevent this is to check every target at runtime by trying to cast it to TestTargetType.

Honestly, the edge case allows you to opt in or out of testing by wrapping your TargetType or TestTargetType. It's valuable behavior in a way 😂

Let's sit on this a bit more for me to explore casting. To be honest, I really liked the code separation that the where clause provides, but if I can protect people from their own design decisions then that should be explored.

Realized last night in a dream that nothing can be done with casting. The MultiTarget abstracts away any sense of an underlying TargetType.

BasThomas · 2017-12-18T21:21:16Z

This is super cool - thanks for sharing your thoughts and code @SD10; much appreciated. I’ll give this a closer inspection soon 👍

_{Sent with GitHawk}

freak4pc

I like the idea in general. I think it provides a less involved approach.

I couldn't exactly understand where you define the headers - since I noticed you removed those everywhere.

Also have a few styling nits / thoughts I'd love some comments on :)

Great work!

freak4pc · 2017-12-18T21:24:00Z

Sources/Moya/MoyaProvider+Stubbing.swift

+        notifyPluginsOfImpendingStub(for: request, target: target)
+        let plugins = self.plugins
+        let stub: () -> Void = createStubFunction(cancellableToken, forTarget: target, withCompletion: completion, endpoint: endpoint, plugins: plugins, request: request)
+        switch stubBehavior {


Mind breaking this up to logical portions with some newlines? (before the switch, before and after the notifyPlugins, etc)

It's a bit hard to chew down this way

freak4pc · 2017-12-18T21:24:53Z

Sources/Moya/MoyaProvider+Stubbing.swift

+        case .delayed(let delay):
+            let killTimeOffset = Int64(CDouble(delay) * CDouble(NSEC_PER_SEC))
+            let killTime = DispatchTime.now() + Double(killTimeOffset) / Double(NSEC_PER_SEC)
+            (callbackQueue ?? DispatchQueue.main).asyncAfter(deadline: killTime) {


You should be able to do do .asyncAfter(deadline: .now() + delay) { IIRC

I believe this should work as well. Good clean up 👍

freak4pc · 2017-12-18T21:25:56Z

Sources/Moya/MoyaProvider+Stubbing.swift

+    /// Creates a function which, when called, executes the appropriate stubbing behavior for the given parameters.
+    public final func createStubFunction(_ token: CancellableToken, forTarget target: Target, withCompletion completion: @escaping Moya.Completion, endpoint: Endpoint<Target>, plugins: [PluginType], request: URLRequest) -> (() -> Void) { // swiftlint:disable:this function_parameter_count
+        return {
+            if token.isCancelled {


This would read better as

guard !token.isCancelled else { self.cancelCompletion(completion, target: target) return }

IMO

freak4pc · 2017-12-18T21:32:26Z

Tests/MultiTargetSpec.swift

-                let expectedData = "sample data".data(using: .utf8)!
-                expect(target.sampleData).to(equal(expectedData))
-            }
+//            it("uses correct sample data") {


If it's commented out, just remove it :)

Ah, I commented it out temporarily. I should've added a TODO. I don't think I should be reducing test coverage, but rather fix whatever tests I break 😅

Adding a TODO is always good :) But if your PR is WIP and you remember to fix this later than its fine :)

freak4pc · 2017-12-18T21:33:18Z

Sources/Moya/MoyaProvider+Stubbing.swift

+
+    /// Return a response after a delay.
+    case delayed(seconds: TimeInterval)
+}


Really great ideas here ! Super nice work @SD10 .

Thanks for the review!

MoyaBot · 2017-12-19T23:11:53Z

	4 Warnings
⚠️	PR is classed as Work in Progress
⚠️	Big PR, try to keep changes smaller if you can
⚠️	Any changes to library code should be reflected in the Changelog. Please consider adding a note there and adhere to the Changelog Guidelines.
⚠️	Consider adding supporting documentation to this change. Documentation can be found in the `docs` directory.

SwiftLint found issues

Warnings

File	Line	Reason
MultiTarget.swift	24	TODOs should be avoided (- Not sure how this would affe...).
EndpointSpec.swift	274	Colons should be next to the identifier when specifying a type and next to the key in dictionary literals.
EndpointSpec.swift	275	Colons should be next to the identifier when specifying a type and next to the key in dictionary literals.
MoyaProviderSpec.swift	978	Files should have a single trailing newline.

Generated by 🚫 Danger

SD10 · 2018-01-09T07:05:05Z

I'm tagging #1539 because I think it's an issue that will help me better analyze potential cons with this approach.

SD10 · 2018-03-08T19:45:18Z

Revisited this today with an even better understanding of Moya's internals.

Currently, to stub a request in Moya you have to configure the stubbed request in 3 different places:

You need to provide sampleData in the TargetType protocol
You need to provide a StubBehavior through the stubClosure property of MoyaProvider
You need to provide a new endpointClosure to a MoyaProvider if you want to manipulate the mapping of a TargetType to EndpointSampleResponse

I think the drawback of this approach is it requires so much configuration in many different places throughtout the library. This also results in intertwining testing logic with the business logic of your codebase.

Furthermore, stubClosure and endpointClosure both take in a TargetType as their only parameters.

I think this alludes to the behavior that people really want: to customize behavior per TargetType.

So, this PR aims to address this issue by making all these things configurable in the TargetType directly. Looking for any feedback from @Moya/contributors again

sunshinejr · 2018-04-10T08:39:11Z

@SD10 I know that we are still trying to pass this one, so please rebase it when you find time so we can review it again.

gligorkot · 2018-05-24T21:38:24Z

Hey @SD10, just wanted to chime in as an outsider as I've been using Moya since v7.0 and love it. I think this would really be a worthwhile change. I've been reading through the issues and I believe this would address #1126 so I thought I'd link it here as well, but correct me if I'm wrong?

One thing I wanted to add, I am big on unit testing my frameworks and have been writing tests for the network layer for a while, so having the stubs really makes this easy; however if it was possible to only include the stubs and TestTargetType in the UnitTest package this would be awesome! Wasn't sure if this was your intention so I thought I'd add it.

Happy to provide help on this matter and jump more into the open source waters.

SD10 · 2018-05-24T22:30:00Z

@gligorkot Thanks for the feedback 💯 This does in fact address #1126 and I agree it's a worthwhile change.

It allows greater flexibility, a better separation of concerns, and would allow us to remove MoyaProvider's closure based customization in favor of plugins (should we choose to do so) in the future.

I do plan on fixing this up, however, I just started a new job so my time for open source has significantly decreased. Over the next month, I will be moving our apps networking layer to Moya, so after that, I will look at this again.

If the Moya community accepts this proposal, I don't see this change being made until Moya 13.

In the meantime, if you want to move your tests to the test target this work around could work?

// In your app target
extension TargetType {
   var data: Data { return Data() }
}

// In your test target
extension YourObjectThatConformsToTargetType {
    var data: Data { 
          // return your real data here
    }
}

This should work because the extension on your object will override the implementation provided on theTargetType protocol.

gligorkot · 2018-07-12T21:47:42Z

@SD10 thanks for getting back to me and providing the workaround, however I just tried it and the extension implementation didn't override the TargetType implementation in this case, so I'm back to including my sampleData in the main target instead of only the test target. Hopefully this makes it in the goals for one of the next versions of Moya.

SD10 · 2018-07-12T22:43:28Z

@gligorkot This will definitely be in Moya 13.0 if the community agrees it's a good approach.

Regarding your work around, I tried the exact same thing yesterday and figured out you can't change the dynamic dispatch for the Target argument of the defaultEndpointMapping closure in the test target. I got around this by providing my own endpointClosure to the MoyaProvider in the test target. The Endpoint closure may also need it's target argument typed as your specialized TargetType.

// In your test target
let endpointClosure: (MyTargetType) -> Endpoint = { target in
   return Endpoint(...
}

amaurydavid · 2018-11-06T09:22:18Z

I like the idea of stripping down the stub configuration to only a specific target type and its 2 properties.
If the intention is great, I don't think it is easy enough to use for every stubbing use cases.

As I see it, there are 2 main usages of request stubbing: creating unit tests, and having a placeholder content for a soon-to-be-live route of our favorite API.

For the first case, this solution is functionnal: just replace TargetType by TestTargetType in the mock target, implement the 2 new properties, and if needed, also replace MultiTarget by MultiTestTarget. Nothing more to be done to support stubbing.

As of the second case, it's more a boilerplate. Let's say you start with a target without stub, and you want to support a new route that will be released soon:

replace TargetType by TestTargetType
implement the 2 new properties
replace MultiTarget by MultiTestTarget if needed
And then, when the API is live, you'll have to remove the stubbing code:
replace TestTargetType by TargetType
remove the 2 old properties
replace MultiTestTarget by MultiTarget if needed

Finally, repeat again when a new route comes up.
It's ok to do all that for one route, but after having to change types twice we'll consider just using TestTargetType and MultiTestTarget permanently to avoid changing types everytime. Or you will juste forget to change the types.
Moreover, if you are a new dev on the project, you will try to make as few changes as possibles, and changing types can be too much a worry.

So great solution for unit tests, not so great for temporary stubbing.

Speaking of temporary stubbing, could you consider making the sampleResponse being Optional? It would prevent having to return Data() for a route using StubBehavior.never. Or even better to me: merging those 2 properties by having the Data as associated value of the StubBehavior (as proposed on #1754 ).

SD10 · 2018-11-15T07:27:00Z

Hey @amaurydavid, thanks so much for taking the time to review this ❤️ Unfortunately I can only give a brief response.

As I see it, there are 2 main usages of request stubbing: creating unit tests, and having a placeholder content for a soon-to-be-live route of our favorite API.

I think this is a really good way of summarizing things 👍

For the first case, this solution is functionnal: just replace TargetType by TestTargetType in the mock target, implement the 2 new properties, and if needed, also replace MultiTarget by MultiTestTarget.

It is a bit concerning that you would need to replace MultiTarget with MultiTestTarget. Unfortunately, that downside is related to the abstraction of MultiTarget itself

As of the second case, it's more a boilerplate. Let's say you start with a target without stub, and you want to support a new route that will be released soon:

replace TargetType by TestTargetType

implement the 2 new properties

replace MultiTarget by MultiTestTarget if needed

And then, when the API is live, you'll have to remove the stubbing code:

Good point, but I think there's always some overhead around mocking an API. I would probably leverage some compiler directives so that the TestTargetType conformance is only implemented when using something like a development/staging/qa target

Speaking of temporary stubbing, could you consider making the sampleResponse being Optional? It would prevent having to return Data() for a route using StubBehavior.never. Or even better to me: merging those 2 properties by having the Data as associated value of the StubBehavior (as proposed on #1754 ).

The idea of combining the two is interesting! I briefly reviewed that issue but would have to explore the implications of this further.

Thanks for giving me a bunch of things to think about! 👍

amaurydavid · 2019-01-09T13:05:12Z

Hello :) Is there any update on this matter? The subject is opened since more than 1 year now :|

SD10 added 2 commits December 11, 2017 17:46

Create TestTargetType protocol

a2d8878

Temporarily comment out some failing tests

44305d5

SD10 commented Dec 12, 2017

View reviewed changes

freak4pc reviewed Dec 18, 2017

View reviewed changes

Add changes from @freak4pc code review

ffe9d2b

SD10 mentioned this pull request Jan 21, 2018

Undefined behavior when using both endpointClosure and plugins #1549

Closed

gobetti mentioned this pull request Apr 23, 2018

How to use delayed stub with RxTest.TestScheduler? #1650

Closed

SD10 mentioned this pull request May 23, 2018

Allow more granular URLRequest mapping #1654

Closed

SD10 mentioned this pull request Aug 1, 2018

Has anyone thought about an AsyncPluginType for Moya? #1706

Open

pedrovereza mentioned this pull request Aug 3, 2018

Release first beta of version 12.0 #1709

Closed

SD10 mentioned this pull request Aug 15, 2018

Another look at TargetType's sampleData property and stubbing requests #1285

Open

SD10 mentioned this pull request Nov 5, 2018

An easier way to use stub requests and sampleData #1754

Closed

gligorkot mentioned this pull request Nov 14, 2018

[Release] Moya 12.0 #1753

Merged

sunshinejr mentioned this pull request Mar 24, 2020

sampleData as an optional property. #2015

Merged

amaurydavid mentioned this pull request Mar 27, 2020

Feature/stubbing from plugins #2020

Closed

Base automatically changed from development to master September 4, 2021 12:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Refactoring Moya's stubbing API #1502

[WIP] Refactoring Moya's stubbing API #1502

SD10 commented Dec 12, 2017

SD10 Dec 12, 2017

sunshinejr Dec 18, 2017

SD10 Dec 18, 2017

SD10 Mar 8, 2018 •

edited

sunshinejr Mar 11, 2018 •

edited

SD10 Mar 11, 2018 •

edited

sunshinejr Mar 11, 2018

SD10 Mar 11, 2018 •

edited

SD10 Mar 11, 2018

SD10 Mar 17, 2018

BasThomas commented Dec 18, 2017

freak4pc left a comment

freak4pc Dec 18, 2017

freak4pc Dec 18, 2017

SD10 Dec 18, 2017

freak4pc Dec 18, 2017

freak4pc Dec 18, 2017

SD10 Dec 18, 2017

freak4pc Dec 18, 2017

freak4pc Dec 18, 2017

SD10 Dec 18, 2017

MoyaBot commented Dec 19, 2017

SD10 commented Jan 9, 2018

SD10 commented Mar 8, 2018 •

edited

sunshinejr commented Apr 10, 2018

gligorkot commented May 24, 2018

SD10 commented May 24, 2018 •

edited

gligorkot commented Jul 12, 2018

SD10 commented Jul 12, 2018

amaurydavid commented Nov 6, 2018 •

edited

SD10 commented Nov 15, 2018 •

edited

amaurydavid commented Jan 9, 2019

[WIP] Refactoring Moya's stubbing API #1502

Are you sure you want to change the base?

[WIP] Refactoring Moya's stubbing API #1502

Conversation

SD10 commented Dec 12, 2017

Summary

How?

Final thoughts

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SD10 Mar 8, 2018 • edited

Choose a reason for hiding this comment

sunshinejr Mar 11, 2018 • edited

Choose a reason for hiding this comment

SD10 Mar 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SD10 Mar 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BasThomas commented Dec 18, 2017

freak4pc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoyaBot commented Dec 19, 2017

SwiftLint found issues

Warnings

SD10 commented Jan 9, 2018

SD10 commented Mar 8, 2018 • edited

sunshinejr commented Apr 10, 2018

gligorkot commented May 24, 2018

SD10 commented May 24, 2018 • edited

gligorkot commented Jul 12, 2018

SD10 commented Jul 12, 2018

amaurydavid commented Nov 6, 2018 • edited

SD10 commented Nov 15, 2018 • edited

amaurydavid commented Jan 9, 2019

SD10 Mar 8, 2018 •

edited

sunshinejr Mar 11, 2018 •

edited

SD10 Mar 11, 2018 •

edited

SD10 Mar 11, 2018 •

edited

SD10 commented Mar 8, 2018 •

edited

SD10 commented May 24, 2018 •

edited

amaurydavid commented Nov 6, 2018 •

edited

SD10 commented Nov 15, 2018 •

edited