RFC: Improve test coverage for script generation via configuration object

[mokagio]

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, but not the script generation itself, GenerateBuildkiteJobScript.

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.

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.:

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?