Add e2e tests#2
Conversation
surpher
left a comment
surpher
left a comment
There was a problem hiding this comment.
This looks very promising! Thanks for taking time to look into this and help out. 🙇
A few nitpicks I left for you but I'll be happy to accept your PR once you're ready to open it. 👍
| dependencies: [ | ||
| .package(url: "https://github.com/ittybittyapps/PactSwiftMockServer.git", branch: "main") | ||
| .package(url: "https://github.com/ittybittyapps/PactSwiftMockServer.git", branch: "main"), | ||
| .package(url: "https://github.com/pointfreeco/swift-snapshot-testing", exact: "1.16.0") |
There was a problem hiding this comment.
note: As a general rule in PactSwift I wanted to avoid 3rd party dependencies and stick to what Swift and Xcode provide us. Saying that, I'm a fan of PointFree's mission and what they do myself and pulling in their tools/frameworks that can increase confidence is okay with me. 👍
There was a problem hiding this comment.
I'm planning to make a couple PRs one with snapshot testing and one without. Like I say, I'm not set on it so I want you to choose what you like.
There was a problem hiding this comment.
Great stuff. Really appreciate your help! 🙇
| dependencies: [ | ||
| "PactSwift" | ||
| "PactSwift", | ||
| .product(name: "InlineSnapshotTesting", package: "swift-snapshot-testing") |
There was a problem hiding this comment.
nitpick: Indentation to match "PactSwift". (applies to other spots too, but I'll nitpick only here 😉 )
reasoning: Because this is an open source project that could (hopefully 🤞) be picked up by other contributors in a very async manner it is important to stick to a certain code style (open to discussion on what that might be). It might not seem like a big deal now, but as the project evolves and contributions introduce code style deviance it gets harder and harder to read through the code and maintain it.
|
|
||
| private var pactDirectory: String { | ||
| ProcessInfo.processInfo.environment["PACT_OUTPUT_DIR"]! | ||
| ProcessInfo.processInfo.environment["PACT_OUTPUT_DIR"] ?? "/tmp/pacts" |
There was a problem hiding this comment.
thought: I'm personally a fan of force-unwrapping in tests. But only in tests. Why? Fail fast. If my test crashes it's most likely "developer issue" and should be looked into much deeper.
There was a problem hiding this comment.
That force unwrap is finding nil for me. What should I be doing differently?
There was a problem hiding this comment.
Ah of course, that thing is a bit obscure in PactSwift's readme paragraph Generated Pact Contracts:
If that doesn't work, great, we can find a setup that works across contributors' systems.
I'm digging into re-introducing Linux support with adding Swift Package Manager Plugin to pull down a .so from pact-foundation/pact-reference releases.
There definitely was an interface that would take URL:
MockService(
consumer: "consumer",
provider: "provider",
writePactTo: URL(fileURLWithPath: "/absolute/path/pacts/folder", isDirectory: true)
)But I'm not quite sure anymore if it's still there. Like I said, I haven't used PactSwift on a project in more than a year 😞
| // | ||
| // PactContractTests2.swift | ||
| // | ||
| // | ||
| // Created by Jason Sunde on 4/30/24. | ||
| // |
There was a problem hiding this comment.
note-to-self: I need to add the file template to the project that renders the author and MIT licence block of text.
| } | ||
|
|
||
|
|
||
| extension PactContractTests2 { |
There was a problem hiding this comment.
suggestion: Could private extension this one and maybe split it with a // MARK: - Private comment above it. This way it helps in your minimap:
If you think this could be a useful re-usable utillity in other tests as well, maybe extract it into a "base test" that you then inherit from or a protocol with default implementation you conform your test class to.
| guard let data = fileContents.data(using: .utf8), | ||
| let jsonObject = try JSONSerialization.jsonObject(with: data, options: []) as? [String: Any] else { | ||
| return [:] | ||
| } |
There was a problem hiding this comment.
thought: This is obviously a very subjective thing. Though if there's guarding against multiple optionals, maybe moving them all to their own lines could be easier to read? If there's only one, then guard expression else { is fine on one line. IMO, and again, this is very subjective, and it ties into the comment I left above regarding sticking to code style across the project and project timeline.
guard
let data = fileContents.data(using: .utf8),
let jsonObject = try JSONSerialization.jsonObject(with: data, options: []) as? [String: Any]
else {
return [:]
}note-to-self: Should add/update guard let code style in PactSwift ADRs.
3 participants
Provide a more descriptive summary of the changes in this PR here.
📝 Summary of Changes
Changes proposed in this pull request:
Document anything here that you think the reviewer(s) of this PR may need to know, or would be of specific interest.
🧐🗒 Reviewer Notes
💁 Example
Add an example of using the changes you've implemented (eg, a screenshot or text dump of the output).
🔨 How To Test
Provide instructions on how to test your changes.