Feature/native wrapper pr by nonec · Pull Request #50 · DiUS/pact-consumer-swift

nonec · 2019-06-08T00:03:35Z

I wanted to try the native wrapper for a project. So I updated dependencies and made adjustments to make it work for me. It's also using Swift 5 now.
In the end I tried to clean it up but there might be some more necessary.
Made it run on Travis CI and adjusted the tests to pass.

I would be happy to receive feedback :-)

Improves travisCI steps

…nt race condition

Update BrightFutures

Wait on verify to complete before allowing tests to continue

Updates README to encourage usage of pact-ruby-standalone

(Quick/Nimble#608)

Update project and dependencies for Swift 4.2

nonec · 2020-01-30T15:10:51Z

Was the only reason to split the rust binary because of the file size? I used git lfs last time https://git-lfs.github.com/ While I don't think there is any problem in theory in splitting it as you have, it might just be more tricky to upgrade the libs in the future.

I just realised I maybe even tried git-lfs before, but it's not possible to push with git-lfs to a fork.

surpher · 2020-04-13T06:33:18Z

Brewfile

@@ -0,0 +1,4 @@
+tap 'pact-foundation/pact-ruby-standalone'


To avoid having these changes that were not made by you in the PR I would encourage you to rebase origin:master onto your fork:branchPR, then force push (onto your fork:branchPR!!!) the new history.

That way any of the changes made to origin will not show up in your PR.

TODO: (for me)
yes, I agree. I'll have to see when I have time for this. Since I think this is what I did the last time.

surpher · 2020-04-13T06:34:58Z

CHANGELOG.md

@@ -1,5 +1,74 @@
+# 0.7.1 - Bugfix Release


Same applies here regarding rebasing on your PR branch. You haven't made this changes, have you? Therefore they should not be part of your PR. 🤷

Agreed, like above

surpher · 2020-04-13T06:36:41Z

Cartfile

-github "SwiftyJSON/SwiftyJSON" ~> 4.0
+github "Thomvis/BrightFutures" ~> 8.0
+github "Quick/Nimble" ~> 8.0
+github "SwiftyJSON/SwiftyJSON" ~> 5.0.0


After having Codable available to us since Swift 4, I'd encourage you to avoid bringing in more dependencies for things that Swift can do for by just using what Foundation provides.

Also something I agree on. Original the branch was done by @andrewspinks . When I first started working on this my goal was to get it to a point that it can be merged back.
Since Codable came up I thought about this, but the top priority was to get everything else working right first.

surpher · 2020-04-13T06:37:59Z

Cartfile.private

@@ -1 +1,2 @@
-github "Quick/Quick" ~> 1.1
+github "Quick/Quick" ~> 2.0
+github "AliSoftware/OHHTTPStubs"


I know this is not you @nonec but if you run carthage update now it will pull version 9.0.0 and it will break current implementation of tests as this dependency dropped OH prefix from all its class names. All it will bring is a headache. :|

That is a headache, yes :|

surpher · 2020-04-13T06:40:58Z

Sources/Native/DictionaryExtensions.swift

    var newDictionary: [Key: Value] = self
-    for (key, value) in dictionary {
-      newDictionary[key] = value
+    for (dictKey, value) in dictionary {


Nitpicking here... The newDictionary already defines it as [Key: Value], following the convention would make sense.

Are you talking about the type [Key: Value] or the naming (dictKey, value)?

surpher · 2020-04-13T06:42:36Z

Sources/Native/NativeMockServerWrapper.swift

+    import SwiftPactServer
+#endif
+
+public class NativeMockServerWrapper: MockServer {


It is really hard to read through this class where public and private declarations are all over the place. It is hard to read and learn about interface the caller needs to interact with.

I think it's mostly private except the MockServer protocol implementations.
What would you do to improve it?

surpher · 2020-04-13T06:43:38Z

Sources/Native/NativeMockServerWrapper.swift

+
+public class NativeMockServerWrapper: MockServer {
+    private lazy var port: Int32 = {
+        return findUnusedPort()


If this findUnusedPort() is only called here, why not have the logic encapsulated here? it's a private var too.

TODO:
yes. I think this grew and probably was used somewhere else before.

surpher · 2020-04-13T06:45:03Z

Sources/Native/NativeMockServerWrapper.swift

+    private let pactDir: String = ProcessInfo.processInfo.environment["pact_dir"] ?? "/tmp/pacts"
+    private let shouldWritePacts: Bool
+
+    public init(shouldWritePacts: Bool = false) {


is shouldWritePacts used to have a dry run on tests when it's set to false? Or does it have something to do with Filesystem?

If I remember it correctly this was needed for the setup I'm using.
I think part of the problem was this is failing on devices and it only worked for simulators. So I had to control a bit when to write for being able to automate this in our building pipeline.

surpher · 2020-04-13T06:46:40Z

Sources/Native/NativeMockServerWrapper.swift

+
+                // iOS json generation adds extra backslashes to "application/json" --> "application\\/json"
+                // causing the MockServer to fail to parse the file.
+                let sanitizedString = jsonString!.replacingOccurrences(of: "\\/", with: "/")


This sanitisation is occurring in a few places, could pull it out into a reusable method or a String extension.

TODO:
always a good idea

surpher · 2020-04-13T06:55:00Z

Sources/Ruby/RubyMockServer.swift

    case write([String: [String: String]])

-    var method: HTTPMethod {
+    var method: String {


HTTPMethod.rawValue should be the only place where the mapping of these should occur.

I think HTTPMethod was from Alamofire which I removed as a dependency here.
So what would be the todo here?

surpher · 2020-04-13T06:56:07Z

Sources/Ruby/RubyMockServer.swift

    // MARK: URLRequestConvertible
    func asURLRequest() throws -> URLRequest {
-      let url = try Router.baseURLString.asURL()
+      guard let url = URL(string: Router.baseURLString) else { throw NSError(domain: "", code: 1, userInfo: nil) }


A new merge on the origin:master made Router method to throw. This is kind of out of date now. :|

TODO:
Yeah, I'll have to see about that

surpher · 2020-04-13T06:59:24Z

Package.swift

+    .package(url: "https://github.com/Quick/Quick.git", from: "2.1.0"),
+    .package(url: "https://github.com/SwiftyJSON/SwiftyJSON.git", from: "5.0.0"),
+    .package(url: "https://github.com/Thomvis/BrightFutures.git", from: "8.0.0"),
+    .package(url: "https://github.com/nonec/SwiftPactServer.git", from: "1.0.0"),


Instead of having yet another git repo you rely on due to SPM, you could have it as part of the current project and define dependency as .package(path: "path/to/folder/containing/SwiftPactServer")

I remember thinking about that. I think part of the problem was having big files in that and I couldn't use git-lfs on a fork because that would count into the space of the origin.
I'm not a 100% sure, but I think this would be something to resolve if we ever get this to a mergeable state.

surpher · 2020-04-13T07:09:53Z

Great job in doing this @nonec ! 👍 I see it still supports pact spect v2? Have you looked into implementing v3 to match the rust pact_mock_server?

I had a scroll through but there is so much of it and there are so many changed files I know are not files you changed. It is very very hard to provide feedback because of that. I did leave a few comments that I think you made and I hope you can take it onboard (or take with you and apply on other projects).

If you do manage to merge/rebase and align your PR with current DiUS/pact-consumer-swift:master so we can see only your changes, I'm happy to provide more constructive and in-depth feedback.

surpher · 2020-04-14T00:22:39Z

Sources/Native/NativeMockServerWrapper.swift

+        }
+    }
+
+    public func verify(_ pact: Pact) -> Future<String, PactError> {


You're passing Pact into verify() yet it is not used in the method.

That is true. It's part of the protocol (and the ruby implementation uses it). I could just ignore it here (or move away from the ruby alternative and change the protocol)
TODO:

surpher · 2020-04-14T00:27:01Z

Sources/Native/NativeMockServerWrapper.swift

+
+    private func writeFile() -> Int32 {
+        guard shouldWritePacts, checkForPath() else {
+            return 4


It's really hard to understand what this error would be. I'd suggest you use an enum for these. Something like:

enum VerifyResult { case failToWrite case failToConnect case successWrite case successVerifyOnly case unknown(String?) }

Naming could be much better for this example, but you get an idea.
That way when you switch through the case you can read it and understand what is happening (case .successVerifyOnly rather than dealing with 4.

TODO:
agreed. same is valid for the error handling in setup(_ pact: Pact). I think there was a comment on that before as well.

nonec

Great job in doing this @nonec ! 👍 I see it still supports pact spect v2? Have you looked into implementing v3 to match the rust pact_mock_server?

I had a scroll through but there is so much of it and there are so many changed files I know are not files you changed. It is very very hard to provide feedback because of that. I did leave a few comments that I think you made and I hope you can take it onboard (or take with you and apply on other projects).

If you do manage to merge/rebase and align your PR with current DiUS/pact-consumer-swift:master so we can see only your changes, I'm happy to provide more constructive and in-depth feedback.

Thanks for taking the time looking at this. I'm sorry this got outdated, again.
At the moment the biggest work to get this started was done by @andrewspinks . I only picked this up (twice), updated it and get it to run.
So yes, atm it supports v2, yes. I'd really like for it to support v3.

Tbh I'm not sure if I'll do this dance once more or I'll just pick up my learnings and start over with something that only uses the native wrapper and implements v3.
Not sure when I'll have time to get into this, either way :|

So for now I tried to answer your comments and added TODOs for myself to go through, when there's time on my side.

andrewspinks · 2020-06-02T11:09:34Z

Most of the learnings from this PR / branch have been taken and used in a new project here: https://github.com/surpher/PactSwift The idea is to eventually move that to the pact-foundation once it has stabilized. @nonec it would be awesome if you could take a look at that new project when if you get some spare time. I'm going to close this PR to save it getting even more out of date.

nonec · 2020-06-02T13:48:33Z

Most of the learnings from this PR / branch have been taken and used in a new project here: https://github.com/surpher/PactSwift The idea is to eventually move that to the pact-foundation once it has stabilized. @nonec it would be awesome if you could take a look at that new project when if you get some spare time. I'm going to close this PR to save it getting even more out of date.

Oh, that looks great! I hope I'll have some time to look into it soon. Thanks for the heads up!

stephengroat and others added 30 commits January 4, 2018 15:37

Create Brewfile

8bc8fc0

move to brew bundle

db7abe5

move to brew update

3e1edc3

Merge pull request #34 from stephengroat/patch-1

38d5055

Improves travisCI steps

wait on verify to complete before allowing tests to continue to preve…

b538409

…nt race condition

Update BrightFutures

26cab90

Merge pull request #37 from rvoortman/update-bright-futures

c7d1ed5

Update BrightFutures

Merge pull request #38 from stitchfix/master

6749fdc

Wait on verify to complete before allowing tests to continue

Update gems

2bc59b5

Update release script to work better with cocoa pods

52cf980

update changelog for release 0.5.2

2b4f5e0

bump cocoapods version to 0.5.2

5ec6eb3

Updates README to encourage usage of pact-ruby-standalone

2a667c2

Removed reference to non-standalone pact mock service

e1c4596

Merge pull request #40 from surpher/master

58387dd

Updates README to encourage usage of pact-ruby-standalone

Update readme

d639fe7

Update BrightFutures dependency version

6ca5c37

Project updates for Swift 4.2

0e7212b

Updates travis pipeline to run tests on iOS 12.1 and 11.3

5897e28

Bump up Alamofire version

4198e38

Bumps up versions of all dependencies

8c4d3cf

Update travis CI env matrix

596b6b8

Update Quick dependency to use “master”

f8899f8

(Quick/Nimble#608)

Update Cartfile.resolved for use with bootstrap on CI

2e77868

Merge pull request #42 from surpher/master

f2f4ec7

Update project and dependencies for Swift 4.2

Update pact-mock-service versions

5ebc062

Update cocoapods dependencies.

a1cd000

update changelog for release 0.5.3

c4d696b

bump cocoapods version to 0.5.3

3db2587

Fix cocoa pods release version number and release script.

28e1588

Lucas Lürich added 3 commits January 30, 2020 15:25

moved dependencies and bootstrapping to travis

6945ef6

added carthage cache

5b1423a

reduced log output size

d472375

Lucas Lürich added 7 commits January 30, 2020 17:14

reduced debug further

a9a952d

moved building to travis config file

75fbe78

updated phone

db67bdb

only write matching rules once

4940d29

simple test for pact body builder

c31f085

minor changes on pact file structure

23bcc7c

Merge branch 'feature/nativeWrapperCurrent' into feature/nativeWrapperPR

9901d79

surpher reviewed Apr 13, 2020

View reviewed changes

surpher reviewed Apr 14, 2020

View reviewed changes

nonec commented Apr 23, 2020

View reviewed changes

andrewspinks closed this Jun 2, 2020

Conversation

nonec commented Jun 8, 2019

Uh oh!

nonec commented Jan 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surpher Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surpher Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surpher commented Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surpher Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nonec left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewspinks commented Jun 2, 2020

Uh oh!

nonec commented Jun 2, 2020

Uh oh!

Reviewers

Assignees

surpher Apr 13, 2020 •

edited

Loading

surpher Apr 13, 2020 •

edited

Loading

surpher commented Apr 13, 2020 •

edited

Loading

surpher Apr 14, 2020 •

edited

Loading

nonec left a comment •

edited

Loading