Suite feature

[warpfork]

This addresses several sources of recurring boilerplate that I'm distilling from multiple experiences of using testmark in practice, and also seeks to address several common footgun issues. especially around detecting whether or not fixtures are actually used.

With this, you are sometimes required to write a more complex TestingPattern.Run function, but in exchange, you get vastly less boilerplate about file loading, parsing, index buildilng, and hunk walking, as well as a great number of guardrails such as warnings about unused hunks, and your overall setup can become very declarative:

For example, the declarative top-level wiring may now look like:

func TestCommandDocs(t *testing.T) {
    suite := sweet.NewSuiteManager(osfs.DirFS(fixtureDir))
    suite.MustWorkWith("fixture-*.md", "testcase/*", customTestingPattern{})
    suite.MustWorkWith("demo-*.md, "*", testexec.TestingPattern{...})
    suite.Run(t)
}

And hopefully we can make more reusable TestingPattern APIs appear throughout other extensions as well (such as testexec). This should make such extensions much easier to wire up, and much easier to combine with other tests or extensions.

[warpfork]

An example of a custom testing pattern looks like this:

type testingPattern struct{}

func (tp testingPattern) Name() string          { return "cli doc test" }
func (tp testingPattern) OwnsAllChildren() bool { return false }
func (tp testingPattern) Run(
	t *testing.T,
	filename string,
	subject *testmark.DirEnt,
	reportUse func(string),
	reportUnrecog func(string),
	patchAccum *testmark.PatchAccumulator,
) error {
	reportUse(subject.Path)
	command := strings.Split(strings.Split(filename, ".md")[0], "-")
	var buf bytes.Buffer
	mycli.App.Writer = &buf
	mycli.App.ErrWriter = &buf
	mycli.App.Run(append(command, "-h"))
	if patchAccum != nil {
		newHunk := *subject.Hunk
		newHunk.Body = buf.Bytes()
		patchAccum.AppendPatch(newHunk)
		return nil
	}
	quicktest.Assert(t, buf.String(), quicktest.Equals, string(subject.Hunk.Body))
	return nil
}

This happens to be (real!) code appropriate for wiring up a certain CLI library to generate its help text and use that as a fixture, so some of the errata in the function body concerns that.

There's still a decent amount of code here -- partially because of the increased complexity of having the test function report for itself which hunks it has used! -- but drastically less than you'd have without using the suite system.

We can also see that some references to the patch accumulator system remain fairly manual. I think this is natural and correct. Sometimes fixture regeneration involves updating exactly one hunk (like this case does); sometimes it's much more complicated (such as in testexec, which may update three different hunks at once!), and so this can't really be reduced any further. Compared to without using the suite system: you also would've had to repeat the filename multiple times, for loading the first time, and later for opening writably to patch, and so by using the suite system you're still saving quite a bit of boilerplate over the prior situation... as well as having the suite manager aggregate all patches to the same file for you, if more than one testing pattern is working on hunks in the same file, which is a nice improvement.

[warpfork]

I'm uncertain this error return makes any sense. I lean towards including an error return when not sure, but, so far when implementing this, I've always just had return nil.

The labeling performed when you return an error might be helpful, but... everything in that message is available in other ways:

• the filename is already in the t.Run name tree
• the hunk name is also already in the t.Run name tree
• the tp.Name() isn't, but... the line number in the t.Log output will point at its guts anyway.

[TripleDogDare]

I'm fine with it returning an error. testing.T.Run returns a bool and it seems more idiomatic to return an error than a bool.

[warpfork]

Right, but the callback parameter to testing.T.Run, which is the more apt comparison, is func(*T) with no return value.

I think I'll still side with you in keeping the error return though, just because as a heuristic, almost any interface that doesn't return errors seems to surprise and frustrate me in the long run, much more often than not. And even if I'm wrong, foisting a "return nil" onto implementers isn't a very big burden.

[warpfork]

@TripleDogDare and I had a little additional review discussion about this outside of github, which I'll replay some notes from:

Mostly, wondered if that Run function interface can get any shorter. And in particular, if the signature of those "report*" funcs -- both func(string) -- are... well, okay.

And we're hovering around "yeah, it's fine".

We don't love the size and verbosity of that function interface, but the only alternatives we can think of are bundling two or more of those parameters into another object that we export from this suite package. Doing so results in:

• neutral: no significant reduction the amount of code either here for for implementers.
• positive: we would gain the ability to add more accessors in the future without it being a breaking change.
  • ... but we could also switch to this by adding variations of the TestingPattern interface to what we support in this package, which is also pretty graceful.
• negative: forces all implementors to import this package.
• positive (mild): name consistency in implementers is forced by accessor func names (vs arg names force nothing).

... which seems to sum to overall to pretty much a wash.

[warpfork]

I admit I gave a little more thought yet to if "adding more accessors without it being a breaking change" thing should be the dominant consideration, but I think I'm gonna halt that train of thought and get on with merging this.

If we start introducing features that make this package less optional, then we might as well move the suite feature to the core, and then that can be the time to revisit the API with whatever we've learned in the meanwhile from usage in the wild.

[warpfork]

I haven't updated the textexec package to interact with this yet. I think it should be pretty easy to do so, without too much backwards compatibility break there, either -- we can just add the report func callbacks to its configuration struct, and then all the interactions can be wired together with that, without any strong dependencies in either direction.

[warpfork]

Okay, I snuck updates to the testexec package into this change set too. It indeed wasn't too bad.

It did cause one more change to the suite API, though: the reportUnrecog function also now takes another string param for a descriptive reason message.

[warpfork]

... I'll continue wondering if this API is perfect in the future 🙃 It's already valuable enough that I really want to start using it downstream, so it's merge time 😆