Automattic / Automattic/hostmgr
RFC: Improve test coverage for script generation via configuration object
- Dominant language
- Swift
- Stars
- 12
- Forks
- 3
- Avg merge
- 4h 50m
- Merged PRs (30d)
- 4
Description
I described a testing limitation I run into in https://github.com/Automattic/hostmgr/pull/47.
TL;DR: We test all the commands we use to generate the Buildkite script, [`BuildkiteScriptBuilderTests`](https://github.com/Automattic/hostmgr/blob/40287d22e53a6c593d37a37b3479bd2459eab94e/Tests/libhostmgrTests/BuildkiteScriptBuilderTests.swift), but not the script generation itself, [`GenerateBuildkiteJobScript`](https://github.com/Automattic/hostmgr/blob/40287d22e53a6c593d37a37b3479bd2459eab94e/Sources/hostmgr/commands/generate/GenerateBuildkiteJobScript.swift).
I think we could improve the test coverage of this crucial step by introducing a configuration object to bridge the `GenerateBuildkiteJobScript` from the executable target with the logic in `BuildkiteScriptBuilder`.
```swift
struct BuildkiteScriptConfiguration {
let dependencyPaths: [String]
let commands: [String]
// ...
}
extension BuildkiteScriptConfiguration {
static let `default` = BuildkiteScriptConfiguration(...
}
// BuildkiteScriptBuilder.swift
static func withConfiguration(_ configuration: BuildkiteScriptConfiguration) -> BuildkiteScriptBuilder {
var builder = BuildkiteScriptBuilder()
configuration.dependcyPaths.forEach { builder.addDependency(atPath: $0) }
configuration.commands.forEach { builder.addCommand($0) }
// ...
return builder
}
// GenerateBuildkiteJobScript.swift
func run() throws {
let scriptText = BuildkiteScriptBuilder.withConfiguration(.default).build()
let path = try FileManager.default.createTemporaryFile(containing: scriptText).path
print(path)
}
```
If the configuration object is too much, we could have constants on `BuildkiteScriptBuilder` with default values and apply the same pattern of applying all those at the `GenerateBuildkiteJobScript` level to reduce the surface area of untested code. E.g.:
```swift
struct BuildkiteScriptBuilder {
static let defaultEnvVarPrefixes = ["BUILDKITE_", "IMAGE_ID"]
}
```
## The case for this
The configuration object would live in the `libhostmgr` target and would allow us to get more confidence in the script we generated. Having this setup would have helped me with #47.
## The case against this
One could argue that there is not much difference between adding a test for how we build a configuration object and do the configuration-to-builder-call ourselves. It might be unnecessary overhead.
---
_My take is that, given how important this step is to our workflow, it's best to err on the side of over-testing._
@jkmassel you mentioned to me that you still have changes to this tool left unpublished. Does this fit with the direction you've taken there?
Contributor guide
No contributing guide indexed for this repository
Research direction
Read Tests/libhostmgrTests/BuildkiteScriptBuilderTests.swift and Sources/hostmgr/commands/generate/GenerateBuildkiteJobScript.swift, then compare the testing gap described against pull request #47. Decide whether a configuration object or builder defaults best exposes script generation to tests. Done means the generated Buildkite script is covered with tests and the chosen design is consistent with the tool's direction.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- swift
- Domain
- cli, testing-qa
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100