-
Notifications
You must be signed in to change notification settings - Fork 5
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: add testing utilities #178
Conversation
Signed-off-by: Christoph Schmatzler <[email protected]>
Signed-off-by: Christoph Schmatzler <[email protected]>
Signed-off-by: Christoph Schmatzler <[email protected]>
Signed-off-by: Christoph Schmatzler <[email protected]>
Signed-off-by: Christoph Schmatzler <[email protected]>
Signed-off-by: Christoph Schmatzler <[email protected]>
Signed-off-by: Christoph Schmatzler <[email protected]>
Sources/Noora/NooraMock.swift
Outdated
#if DEBUG | ||
import Rainbow | ||
|
||
public class MockNoora: Noora, CustomStringConvertible { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fortmarek we ended up going for subclassing here because we only wanted to extend the parent with some convenient API for testing, and dependency-inject a pipeline that would record the output. Let us know what you think of the architecture and if you'd do things differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of subclassing, I'd lean to using a protocol and then using the real implementation under-the-hood:
public struct NooraMock: Noorable {
private let noora: Noorable
public init(theme: Theme = .default, terminal: Terminaling = Terminal()) {
self.noora = Noora(theme: theme, terminal: terminal)
}
func yesOrNoChoicePrompt(
title: TerminalText?,
question: TerminalText
) -> Bool {
noora.yesOrNoChoicePrompt(...)
}
}
It does mean the mock needs to define all Noorable
methods but I think that's an ok trade-off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've applied this, I think. Would appreciate some comments on implementation details - will add documentation once the actual structure is signed off by you guys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aligned 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, very much onboard with this 🙂
Since the NooraMock
is something consumers of this library will use, I would add:
- some documentation for how to use the mock to the
docs
- some tests that use
NooraMock
to snapshot test the output.
Sources/Noora/NooraMock.swift
Outdated
#if DEBUG | ||
import Rainbow | ||
|
||
public class MockNoora: Noora, CustomStringConvertible { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of subclassing, I'd lean to using a protocol and then using the real implementation under-the-hood:
public struct NooraMock: Noorable {
private let noora: Noorable
public init(theme: Theme = .default, terminal: Terminaling = Terminal()) {
self.noora = Noora(theme: theme, terminal: terminal)
}
func yesOrNoChoicePrompt(
title: TerminalText?,
question: TerminalText
) -> Bool {
noora.yesOrNoChoicePrompt(...)
}
}
It does mean the mock needs to define all Noorable
methods but I think that's an ok trade-off.
Sources/Noora/NooraMock.swift
Outdated
#if DEBUG | ||
import Rainbow | ||
|
||
public class MockNoora: Noora, CustomStringConvertible { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this is something we expect consumers of Noora
to use to snapshot test the output, I'd add documentation along some usage guidance.
Signed-off-by: Christoph Schmatzler <[email protected]>
Signed-off-by: Christoph Schmatzler <[email protected]>
Sources/Noora/NooraMock.swift
Outdated
public class StandardPipelineEventsRecorder { | ||
var events: [StandardOutputEvent] = [] | ||
} | ||
|
||
public struct StandardOutputEvent: Equatable { | ||
let type: StandardPipelineType | ||
let content: String | ||
} | ||
|
||
public enum StandardPipelineType: CustomStringConvertible { | ||
public var description: String { | ||
switch self { | ||
case .error: "stderr" | ||
case .output: "stdout" | ||
} | ||
} | ||
|
||
case output | ||
case error | ||
} | ||
|
||
public struct StandardPipeline: StandardPipelining { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do all of these types actually need to be public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Sources/Noora/NooraMock.swift
Outdated
CustomStringConvertible | ||
{ | ||
private let noora: Noorable | ||
var standardPipelineEventsRecorder = StandardPipelineEventsRecorder() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this what developer should be using for snapshot testing or rather the description
? Having an example using this mock in action would help a bit with the review as it's not super obvious how this is supposed to be used.
Either way, this property should either be private
or public
, but I don't see why it would be internal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tuist/tuist@main...cs/noora-success-messages
Here's a branch with what Pedro and I imagined in regards to usage.
Signed-off-by: Christoph Schmatzler <[email protected]>
Signed-off-by: Christoph Schmatzler <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cschmatzler I'm onboard with the current changes. We can merge this once we add some documentation, including an example 🙏
Signed-off-by: Christoph Schmatzler <[email protected]>
Signed-off-by: Christoph Schmatzler <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solid work 👏🏼 |
Adds testing utilities to Noora in debug builds that record output and allow us to do snapshot testing.
The output follows the format of