Another round. Hopefully done after this one!


https://codereview.appspot.com/36540043/diff/80001/utils/tailer/tailer.go
File utils/tailer/tailer.go (right):

https://codereview.appspot.com/36540043/diff/80001/utils/tailer/tailer.go#newcode49
utils/tailer/tailer.go:49: func NewTailer(readSeeker io.ReadSeeker,
writeCloser io.WriteCloser, lines int, filter TailerFilterFunc) *Tailer
{
I don't think there's any particular reason we want to give this code
the responsibility for closing the writer. It's quite possible we might
have several tailers writing to the same writer, for example.

Better would be a Wait method (and perhaps a Dead method) to wait until
it's finished. Then the caller has the option of closing the writer.

https://codereview.appspot.com/36540043/diff/80001/utils/tailer/tailer_test.go
File utils/tailer/tailer_test.go (right):

https://codereview.appspot.com/36540043/diff/80001/utils/tailer/tailer_test.go#newcode40
utils/tailer/tailer_test.go:40: {
If we move this brace onto the previous line, we can save a level of
indentation in all these tests.

https://codereview.appspot.com/36540043/diff/80001/utils/tailer/tailer_test.go#newcode42
utils/tailer/tailer_test.go:42: data:                  data[26:29],
I like the new tests much better now. One thought though:

I'm not convinced that referring to "data" in all these tests makes them
more readable - I have no idea what's in data[26:29] without counting or
using grep -n.

Something like the below shows me exactly what's going
on without me needing to refer to multiple places,
and is independent of any changes else in the code.

It would make it considerably easier for me to scan down
the tests to ensure that each one represents reasonable
behaviour.

{
     description: "lines are longer than buffer size",
     data: []string{
         "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz\n",
         "0123456789012345678901234567890123456789012345678901\n",
         "the quick brown fox ",
     },
     initialLinesWritten:   1,
     initialLinesRequested: 1,
     bufferSize:            5,
     initialCollectedData:  []string{
         "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz\n",
     },
     appendedCollectedData: []string{
         "0123456789012345678901234567890123456789012345678901\n",
     },
},

https://codereview.appspot.com/36540043/diff/80001/utils/tailer/tailer_test.go#newcode215
utils/tailer/tailer_test.go:215: line, err := buffer.ReadString('\n')
If the tailer doesn't produce enough data, we'll block here forever I
think, regardless of the timeout.

I think you probably want something more like the below:

func assertCollected(c *gc.C, reader io.Reader, compare []string,
injection func([]string)
      ) {
    lineChan := make(chan string)
    go func() {
        defer close(lineChan)
        reader := bufio.NewReader(reader)
        for {
            line, err := reader.ReadString('\n')
            if !c.Check(err, gc.IsNil) {
                return
            }
            lineChan <- line
        }
    }()
    timeout := time.After(testing.LongWait)
    for {
         select {
         case line, ok := <-lineChan:
              c.Assert(ok, jc.IsTrue)
              deal with line as currently
         case <-timeout:
               timed out
         }
     }
}

https://codereview.appspot.com/36540043/diff/80001/utils/tailer/tailer_test.go#newcode222
utils/tailer/tailer_test.go:222: for i := 0; i < len(lines); i++ {
c.Assert(lines, gc.DeepEquals, compare) ?

https://codereview.appspot.com/36540043/diff/80001/utils/tailer/tailer_test.go#newcode228
utils/tailer/tailer_test.go:228: if err == io.EOF {
Once we've got EOF once on a pipe, I don't think we can get any more
data, can we?

https://codereview.appspot.com/36540043/