search

fiskaly / fiskaly-sdk-swift

fiskaly Cloud-TSE SDK for Swift/iOS
MIT License
5 stars 5 forks source link

Allow injecting a client instance when initializing a FiskalyHttpClient #16

Closed marcelvoss closed 4 years ago

marcelvoss commented 4 years ago

Hi there,

As we're currently implementing this SDK into the SumUp merchant app, we want to test its integration without having to rely on Fiskaly's backend being available during e.g. a CI run. Currently, this is incredibly hard and requires some dirty tricks on our side. For simplifying testability in these scenarios, injecting a mock based on a protocol is the easiest way.

This would for example also allow the SDK's tests to move away from using the actual backend and only use mock responses (I didn't convert the tests that are in place, as I wasn't sure that is what you want). Therefore, this PR focusses on adding the infrastructure for making the FiskalyHttpClient testable.

Please let us know what you think, a change like this would massively improve the testability for us.

prempador commented 4 years ago

Hi @marcelvoss ,

The requested changes seem reasonable and the integration looks very clean.

Regarding the mock of the tests. We are working on a test-catalog with mocked responses and these changes would come in handy.

However I am currently working on refactoring the SDK and I would like to merge my changes before adding yours. The coding parts of this branch are done, I just need to adapt the documentation, so merging the branch into yours should work and would not require any further changes.

Would you be able to adapt your PR, or do you want me to do it?

marcelvoss commented 4 years ago

Hi @prempador,

thanks for the swift (spot the pun!) review. Glad it will be helpful for y'all as well. I did another small adjustment that also reduces the amount of code a bit. I think there's some more stuff that could make it nicer but for simplicity, I'll leave it as it is for now until I have some more time coming back at it.

Would you be able to adapt your PR, or do you want me to do it?

I'm not sure what you mean by that. Do you want me to change the base branch to yours, so this PR will be merged into your branch?

prempador commented 4 years ago

I'm not sure what you mean by that. Do you want me to change the base branch to yours, so this PR will be merged into your branch?

Yes. I would suggest to rebase to my refactor branch to resolve potential conflicts, so, as soon as my branch is merged into master (this will probably happen tomorrow or friday) we don't have any conflicts anymore. If you don't have time however I can do it.

I will take a closer look at this PR as soon as I finished my changes.

marcelvoss commented 4 years ago

@prempador Rebased upon your refactoring branch.

prempador commented 4 years ago

Sorry, muscle memory made me delete the branch

prempador commented 4 years ago

Well, I think I just overcomplicated this whole process by not merging your branch before addressing your comments, I am sorry. Will try to fix this. @marcelvoss

prempador commented 4 years ago

That didn't take too long, done. Looks good, will be merged now.