Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Global mode to choose only to mock registered routes #84

Merged
merged 6 commits into from
Feb 24, 2021

Conversation

letatas
Copy link
Contributor

@letatas letatas commented Feb 16, 2021

Hello,
This is my first time contribution ever.
Yesterday I wrote a question / feature request for our needs: #83

After checking out the source, I found an easy way to integrate this feature without changing the current way it works. So I propose a Pull request to integrate it.

This feature adds a new property to the Mocker struct called mode. By default the current mode is .optout meaning you have to manually define all the routes you want the system to ignore and thus to process as if the mocker does not exist. This is the way it works today. The other mode is .optin used to let all the routes being processed as if there were no mocking system, except for the mocked routes.

The use case leading to this feature is when you have an existing API with many routes, and a team has to develop a new route, for your development phase, you can mock only this route, to keep using you app and all the other routes in a complete transparent way.

I hope you'll find this useful.

…d routes, and let every other url to be processed as if the Mocker is not present.
Copy link
Contributor

@AvdLee AvdLee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your contribution. For a first timer it looks really great!

I've added some comments. Can you fix those?
Also, can you write tests for both modes to make sure they work as expected?

Thanks!

Sources/Mocker.swift Outdated Show resolved Hide resolved
Sources/Mocker.swift Outdated Show resolved Hide resolved
@letatas
Copy link
Contributor Author

letatas commented Feb 17, 2021

I've fixed what you told me to, and I added unit tests.

Hope it's ok!

@wetransferplatform
Copy link
Collaborator

wetransferplatform commented Feb 17, 2021

Messages
📖

View more details on Bitrise

📖 Mocker: Executed 25 tests, with 3 failures (0 unexpected) in 0.000 (0.000) seconds

Mocker.framework: Coverage: 0.0

File Coverage
Mocker.swift 0.0% ⚠️

MockerTests.xctest: Coverage: 0.0

File Coverage
MockerTests.swift 0.0% ⚠️

Generated by 🚫 Danger Swift against 0a7b306

Copy link
Contributor

@AvdLee AvdLee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks a lot!

Copy link
Contributor

@AvdLee AvdLee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, one last thing! Would you mind adding a little chapter in the readme to describe this functionality? It would make your work more visible to others 🙂

@letatas
Copy link
Contributor Author

letatas commented Feb 18, 2021

Sorry for those oversights, it's fixed!

@letatas letatas requested a review from AvdLee February 18, 2021 10:00
README.md Show resolved Hide resolved
@letatas
Copy link
Contributor Author

letatas commented Feb 23, 2021

Hello @AvdLee, if the MR is approved, do you have any release date in mind?

@AvdLee
Copy link
Contributor

AvdLee commented Feb 24, 2021

@letatas I'll do that now! 🙂

@AvdLee AvdLee merged commit 60a9bff into WeTransfer:master Feb 24, 2021
@wetransferplatform
Copy link
Collaborator

Congratulations! 🎉 This was released as part of Release 2.5.0 🚀

Generated by GitBuddy

@letatas
Copy link
Contributor Author

letatas commented Feb 25, 2021

@letatas I'll do that now! 🙂

Great! Thanks for this new release!

@letatas
Copy link
Contributor Author

letatas commented Feb 25, 2021

@AvdLee It seems the .podspec is not up to date and still references the 2.3.0 version.

@AvdLee
Copy link
Contributor

AvdLee commented Feb 26, 2021

@letatas just some feedback, the tests turned out to fail. I'm fixing that now but for future PRs, do check whether all other tests succeed too by running the complete test bundle 🙂

Thanks!

@AvdLee
Copy link
Contributor

AvdLee commented Feb 26, 2021

Also, I'm going to make sure Cocoapods is up to date after #86 is merged in

@letatas
Copy link
Contributor Author

letatas commented Feb 26, 2021

@AvdLee Sorry for the tests, as some tests didn't pass when I forked, even before I changes anything, I thought it was due to my local configuration. I'll be more cautious next time.

@AvdLee
Copy link
Contributor

AvdLee commented Mar 1, 2021

@AvdLee Sorry for the tests, as some tests didn't pass when I forked, even before I changes anything, I thought it was due to my local configuration. I'll be more cautious next time.

Ah, if that was the case it makes total sense. Thanks, it's all running smooth again! 🙂

@letatas
Copy link
Contributor Author

letatas commented Mar 1, 2021

Great! Thanks. One more thing, though: when I try to install Mocker with Cocoapods, it seems to be stalled in v2.2.0, there's no hint of v2.5 or v2.5.1.

pod repo update
pod search Mocker

-> Mocker (2.2.0)
   Mock data requests using a custom URLProtocol and run them offline.
   pod 'Mocker', '~> 2.2.0'
   - Homepage: https://github.com/WeTransfer/Mocker
   - Source:   https://github.com/WeTransfer/Mocker.git
   - Versions: 2.2.0, 2.1.0, 2.0.2, 2.0.1, 2.0.0, 1.3.0, 1.2.1, 1.2.0, 1.1.0, 1.0.0 [cocoapods repo]

Any thoughts?

@AvdLee
Copy link
Contributor

AvdLee commented Mar 1, 2021

@letatas it seems that our pod release train is currently broken:

[09:01:09]: Pod push failed: Exit status of command 'pod trunk push' was 1 instead of 0.
[!] Found podspec `Mocker.podspec`
Adding spec repo `trunk` with CDN `https://cdn.cocoapods.org/`
Updating spec repo `trunk`
Validating podspec
 -> Mocker
 -> Mocker (2.5.1)
    - ERROR | [iOS] xcodebuild: Returned an unsuccessful exit code. You can use `--verbose` for more information.
    - NOTE  | xcodebuild:  note: Using new build system
    - NOTE  | xcodebuild:  note: Building targets in parallel
    - NOTE  | xcodebuild:  note: Using codesigning identity override: -
    - NOTE  | [iOS] xcodebuild:  note: Planning build
    - NOTE  | [iOS] xcodebuild:  note: Constructing build description
    - NOTE  | [iOS] xcodebuild:  warning: Capabilities for Signing & Capabilities may not function correctly because its entitlements use a placeholder team ID. To resolve this, select a development team in the App editor. (in target 'App' from project 'App')
    - NOTE  | [iOS] xcodebuild:  warning: Skipping code signing because the target does not have an Info.plist file and one is not being generated automatically. (in target 'App' from project 'App')
    - ERROR | xcodebuild:  /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator14.4.sdk/usr/lib/swift/XCTest.swiftmodule/x86_64-apple-ios-simulator.swiftinterface:6:19: error: no such module 'XCTest'
    - ERROR | xcodebuild:  Mocker/Sources/Mock.swift:12:8: error: failed to build module 'XCTest' from its module interface; the compiler that produced it, 'Apple Swift version 5.3.3 (swiftlang-1200.2.41.2 clang-1200.0.32.8)', may have used features that aren't supported by this compiler, 'Apple Swift version 5.3.2 (swiftlang-1200.0.45 clang-1200.0.32.28)'
    - ERROR | xcodebuild:  /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator14.4.sdk/usr/lib/swift/XCTest.swiftmodule/i386-apple-ios-simulator.swiftinterface:6:19: error: no such module 'XCTest'

Feel free to have a look at it but at this point, we don't have time to work on this. You can integrate Mocker through SPM as an alternative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants