Refactor serve toward caching results instead of rendered

[petemounce]

Aims at #612. Incomplete.

Step 1: Handle http parts completely within the handler func.
Step 2: Rename res -> runResult for clarity

@aelsabbahy I'm stopping at this point because I think going deeper is going to require a breaking change.

I'll also say up-front that I'm not so familiar with go channels and concurrency, so perhaps I'm ignorant of where a non-breaking-change might be made; happy to take advice there.

Or perhaps I've misunderstood the nested loops in the outputs themselves, and this is how one unwraps the chan array to actual values? Either way, I'm a little lost and would appreciate a steer.

Here's my train of thought

• For serve to be able to cache the test-results, I need to "unwrap" the chan []resource.TestResult to a []resource.TestResult and cache that.
• To do that, I need to add another return value to outputs.Outputer so it becomes (int, []resource.TestResult)
• I'd then be able to throw away the bytes buffer during the "serve needs to cache data" use-case, since now I have an array of TestResult I can...
• ... feed to a new func on each Outputer that takes data rather than chan, and renders that

My intuition says to separate the test-running from the output-generation (i.e. outputs.Outputer#Output doesn't shove data into a buffer, but instead returns results for something else to do that part) - but I'm not really sure how best to achieve how

All the above aside, I think this single commit could probably be merged as-is, since I think it makes the code slightly clearer - what do you think? (If you agree, please lets discuss within the issue itself, so the next iteration can understand the context I've now gained any anything we talk about here. In that case, the comment above should get copied/quoted over there to avoid fragmenting)

Checklist

• make test-all (UNIX) passes. CI will also test this
• unit and/or integration tests are included (if applicable)
• documentation is changed or added (if applicable)

Description of change

[petemounce]

I noticed I'd introduced the func earlier, but not actually used it 🤦

[petemounce]

This comment was a driveby addition, since we talked about output verbosity within the PR.

[aelsabbahy]

Had a little bit of time to take a look at this today. Was trying to cut a release but travis-ci hung on me.

I don't think we can avoid a breaking change here.. but for a different reason than the one you mentioned. For caching to be moved up a layer, iStartTime cannot be an input to the Outputer that is rendered multiple times, we would need to track EndTime too.

I threw together a quick example of how you can cache a go channel in a slice, but this isn't a valid solution for the iStartTime reason I mentioned above. I mostly threw this together in the hopes that it helps you understand how one can cache a channel better. example. Basically chan -> slice/cache -> chan.

A real solution would need to either:

• Switch to a struct with []results, starttime, and endtime - The major downside of this is the user no longer gets "streaming" results, but rather sees them once they're complete, this would be noticeable on slow tests.
• Track start/endtime when using caching.. allow Outputers to support passing in endTime for when cache is used. This is my preference.

@ripienaar I would be curious about your take on this as someone who leverages goss as a library.

Current:

type Outputer interface {
	Output(io.Writer, <-chan []resource.TestResult, time.Time, util.OutputConfig) int
}

Proposed (psudo-codeish):

type Outputer interface {
	Output(io.Writer, resource.TestResults, util.OutputConfig) int
}

type TestResults struct {
   // this
   results []resource.TestResult
   // or this (my preference)
   results <-chan []resource.TestResult
   start time.Time
   end time.Time // optional, when provided sets an explicit endtime for when results are cached
}

[aelsabbahy]

Thinking about this some more a middleware pattern might fit well here for caching and #607 for metrics.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[aelsabbahy]

not stale.. although, I'm thinking there's a refactor using middleware pattern that needs to be done before this ticket and #607.

Would love your thoughts on this @petemounce

[petemounce]

I completely agree that a middleware pattern is appropriate here. Shall we discuss on a new issue?

[aelsabbahy]

Up to you to be honest. This ticket might be small enough that it makes sense to use caching as the reference implementation.

But if you'd to have a more detailed discussion, then a dedicated ticket makes sense.

[petemounce]

What if I adjust this to cache, not at the whole-suite level, but at the individual test level? That is, a cache entry becomes the results from running a command/file/http/etc test's assertions?

That means tradeoffs:

1. - we get test outputs in chunks, rather than per assertion as now. This might not be functionally acceptable but I hope it is.
   • if it isn't, we could cache per assertion with I think only tiny incremental complexity.
     • (I'm thinking here that the cache key generation is basically what segments to concatenate into the cache key string, and the difference is (test id) or (test id + assertion id))
2. - we have a higher number of entries in the cache. I don't really anticipate that being a problem:
   • the quantity of data being stored probably doesn't increase much
   • the cache lookups go up since there are more entries; but this is all in-process and basically a dictionary lookup against a cache key that is a formatted string so unlikely to be significant
3. + we get the extensibility (option!) of finer grained caching (later, if someone asks), so different tests can be cached for different durations (useful for tests that hit remote dependencies mixed together with tests that stay local)
4. + I think tests become simpler because now caching is a middleware, those responsibilities can be split out

Having read that through, I mean towards the per assertion granularity.

What do you think of the above?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.