Automattic / Automattic/hostmgr

RFC: Improve test coverage for script generation via configuration object

Open
#48 0 comments 0 reactions 0 assignees View on GitHub
enhancement hostmgr
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.