Handle buffered tests much more simply

This also updates to make buffered child bailouts less clunky

Author: isaacs

lib/test.js

@@ -566,28 +566,17 @@ Test.prototype._runChild = function runChild (child, name, extra, cb) {
     self._currentChild = null
     var time = results ? '# time=' + results.time + 'ms' : ''
     if (extra.buffered) {
-      var bail = false
-      if (self._bail && !child._ok) {
-        bail = true
-        self._bail = false
-      }
-
       var endThis = false
       if (self._plan === self._count + 1) {
         endThis = true
         self._ending = true
       }
 
-      self.ok(child._ok, child._name + ' {', { diagnostic: false })
-      var b = child._buffer.trim().split('\n').join('\n    ') + '\n'
-      b = b.split('\n    \n').join('\n\n')
-      if (time) {
-        b += '    ' + time + '\n'
-      }
-      self.push('    ' + b + '}\n')
-      if (bail) {
-        self.bailout(child._name)
-      } else if (endThis) {
+      self.ok(child._ok, child._name, {
+        _tapChildBuffer: child._buffer
+      })
+
+      if (endThis) {
         self._ending = false
         self.end(IMPLICIT)
       }
@@ -598,16 +587,16 @@ Test.prototype._runChild = function runChild (child, name, extra, cb) {
       self.push('\n')
     }
   })
+
   child.on('bailout', function (message) {
     if (extra.buffered) {
+      self._currentChild = null
       self._bail = false
-      if (!ended) {
-        child.emit('end')
-      }
-      self.bailout(message)
-    } else {
-      rootBail(self, message)
+      self.fail(child._name, {
+        _tapChildBuffer: child._buffer
+      })
     }
+    rootBail(self, message)
   })
 
   // still need try/catch for synchronous errors
@@ -709,7 +698,7 @@ Test.prototype.stdin = function (name, extra, deferred) {
     self._processQueue()
   })
 
-  parser.on('bailout', function (message) {
+  parser.once('bailout', function (message) {
     rootBail(self, message)
   })
 
@@ -732,14 +721,17 @@ function pruneFailures (res) {
 function rootBail (self, message) {
   var p = self
   while (p._parent) {
+    // end any buffered tests along the way
+    if (p._buffered) {
+      p.emit('end')
+    }
     p._bailedOut = true
     p = p._parent
   }
   p.bailout(message)
 }
 
 function childStream (self, child, parser) {
-  var bailedOut = false
   var emitter = parser || child
 
   if (parser) {
@@ -754,17 +746,14 @@ function childStream (self, child, parser) {
     rootBail(self, reason)
   })
 
-  emitter.on('line', function (line) {
-    if (line.match(/^TAP version \d+\n$/)) {
+  var sawVersion = false
+  emitter.on('line', function onEmitterLine (line) {
+    if (!sawVersion && line.match(/^TAP version \d+\n$/)) {
+      sawVersion = true
       return
     }
 
-    if (line.trim()) {
-      line = '    ' + line
-    } else {
-      line = '\n'
-    }
-
+    line = '    ' + line
     self.push(line)
   })
 }
@@ -879,9 +868,11 @@ Test.prototype.spawn = function spawnTest (cmd, args, options, name, extra, defe
     childStream(self, child.stdout, parser)
   } else {
     var buffer = ''
+    var sawVersion = false
     child.stdout.pipe(parser)
     parser.on('line', function (line) {
-      if (line.match(/TAP version \d+\n$/)) {
+      if (!sawVersion && line.match(/^TAP version \d+\n$/)) {
+        sawVersion = true
         return
       }
       buffer += line
@@ -943,40 +934,27 @@ Test.prototype.spawn = function spawnTest (cmd, args, options, name, extra, defe
 
     var ok = results.ok && !code && !signal
     if (extra.buffered) {
-      if (extra.skip) {
-        if (typeof extra.skip === 'string') {
-          extra.skip += ' {'
-        } else {
-          extra.skip = ' {'
-        }
-      } else if (extra.todo) {
-        if (typeof extra.todo === 'string') {
-          extra.todo += ' {'
-        } else {
-          extra.todo = ' {'
-        }
-      } else {
-        name += ' {'
-      }
-      extra.diagnostic = false
-
-      self.ok(ok, name, extra)
-      var b = buffer.trim().split('\n').join('\n    ') + '\n'
-      b = b.split('\n    \n').join('\n\n')
-      b += '    ' + time + '\n'
-      self.push('    ' + b + '}\n')
-    } else {
-      self.ok(ok, name, extra)
+      extra._tapChildBuffer = buffer
     }
+    self.ok(ok, name, extra)
     if (!self._ended) {
       self.push('\n')
     }
+    if (child._bailedOut) {
+      rootBail(self, child._bailedOut || name)
+    }
     deferred.resolve(self)
     self._processQueue()
   })
 
-  parser.on('bailout', function (message) {
-    child.kill('SIGTERM')
+  parser.once('bailout', function (message) {
+    // bailouts have 1 second to quit before being forcibly killed.
+    var timer = setTimeout(function () {
+      child.kill('SIGKILL')
+    }, 1000)
+    timer.unref()
+    child.once('close', clearTimeout.bind(null, timer))
+    child._bailedOut = message
   })
 
   if (child.stderr) {
@@ -1033,9 +1011,9 @@ function bailOnFail (self, stream, parser, root) {
   })
 
   parser.on('assert', function (res) {
-    if (!res.todo && !res.skip && !res.ok) {
+    if (!res.todo && !res.skip && !res.ok && res.name.slice(-1) !== '{') {
       var ind = new Array(parser.level + 1).join('    ')
-      var line = ind + 'Bail out! # ' + res.name + '\n'
+      var line = ind + 'Bail out! # ' + res.name.trim() + '\n'
       root.buffer = ''
       root.write(line)
     }
@@ -1376,6 +1354,12 @@ Test.prototype.printResult = function printResult (ok, message, extra) {
     return
   }
 
+  var buffer
+  if (extra._tapChildBuffer) {
+    buffer = extra._tapChildBuffer
+    delete extra._tapChildBuffer
+  }
+
   var res = { ok: ok, message: message, extra: extra }
 
   if (extra.todo) {
@@ -1404,6 +1388,10 @@ Test.prototype.printResult = function printResult (ok, message, extra) {
 
   message = message.replace(/[\n\r]/g, ' ').replace(/\t/g, '  ')
 
+  if (buffer) {
+    message += ' {'
+  }
+
   this.push(util.format('%s %d%s', ok ? 'ok' : 'not ok', n, message))
 
   var ending = false
@@ -1431,11 +1419,30 @@ Test.prototype.printResult = function printResult (ok, message, extra) {
   this.push('\n')
 
   // If we're skipping, no need for diags.
-  if (!ok && !extra.skip || extra.diagnostic) {
-    this.writeDiags(extra)
+  // Also, never print diags if there's a child buffer to show
+  if (buffer) {
+    var b = buffer.trim().split('\n').join('\n    ') + '\n'
+    this.push('    ' + b + '}\n')
+  } else {
+    var diagnostic = process.env.TAP_DIAG === '1'
+    if (!ok) {
+      diagnostic = true
+    }
+    if (extra.skip) {
+      diagnostic = false
+    }
+    if (process.env.TAP_DIAG === '0') {
+      diagnostic = false
+    }
+    if (typeof extra.diagnostic === 'boolean') {
+      diagnostic = extra.diagnostic
+    }
+    if (diagnostic) {
+      this.writeDiags(extra)
+    }
   }
 
-  if (this._bail && !ok && !extra.skip && !extra.todo) {
+  if (this._bail && !ok && !extra.skip && !extra.todo && message.slice(-1) !== '{') {
     this.bailout(message.replace(/^ - /, ''))
   }
 
@@ -1634,9 +1641,6 @@ Test.prototype.push = function (c, e) {
       return
     }
 
-    if (!line.trim())
-      line = ''
-
     this.emit('line', line + '\n')
   }, this)
 
@@ -1662,7 +1666,7 @@ Test.prototype.bailout = function (message) {
   this._bailedOut = true
   this._ok = false
   this.emit('bailout', message)
-  this.push(null)
+  this.end(IMPLICIT)
 }
 
 Test.prototype._addMoment = function (which, fn) {

test/segv.js

@@ -9,7 +9,7 @@ var fs = require('fs')
 var spawn = require('child_process').spawn
 
 var segv =
-'int main (void) {\n' +
+  'int main (void) {\n' +
   '   char *s = "hello world";\n' +
   "   *s = 'H';\n" +
   '}\n'
@@ -18,34 +18,41 @@ var segv =
 // we slice the actual output later to just compare the fronts
 // also sort the lines, because Node v0.8 sets keys on objects
 // in different order.
-var expect =
-('TAP version 13\n' +
-'# Subtest: ./segv\n' +
-'    1..0\n' +
-'not ok 1 - ./segv  # time=\n' +
-'  ---\n' +
-'  at:\n' +
-'    file: test/segv.js\n' +
-'    line: \n' +
-'    column: \n' +
-'  results:\n' +
-'    ok: false\n' +
-'    count: 0\n' +
-'    pass: 0\n' +
-'    plan:\n' +
-'      start: 1\n' +
-'      end: 0\n' +
-'      skipAll: true\n' +
-'  signal: SIG\n' +
-'  command: ./segv\n' +
-'  arguments: []\n' +
-'  source: |\n' +
-"    tt.spawn('./segv')\n" +
-'  ...\n' +
-'\n' +
-'1..1\n' +
-'# failed 1 of 1 tests\n' +
-'# time=\n').trim().split('\n')
+var expect = [
+  'TAP version 13',
+  '# Subtest: ./segv ',
+  '    1..0',
+  'not ok 1 - ./segv  # time=',
+  '  ---',
+  '  at:',
+  '    line: ',
+  '    column: ',
+  '    file: test/segv.js',
+  '  results:',
+  '    ok: false',
+  '    count: 0',
+  '    pass: 0',
+  '    fail: 0',
+  '    bailout: false',
+  '    todo: 0',
+  '    skip: 0',
+  '    plan:',
+  '      start: 1',
+  '      end: 0',
+  '      skipAll: true',
+  '      skipReason: \'\'',
+  '      comment: \'\'',
+  '  signal: SIG',
+  '  command: ./segv',
+  '  arguments: []',
+  '  source: |',
+  '    tt.spawn(\'./segv\')',
+  '  ...',
+  '',
+  '1..1',
+  '# failed 1 of 1 tests',
+  '# time='
+]
 
 test('setup', function (t) {
   fs.writeFile('segv.c', segv, 'utf8', function (er) {
@@ -72,21 +79,29 @@ test('segv', function (t) {
     res += c
   })
   tt.on('end', function () {
+    // TODO: parse the yaml and test it against an object pattern
+    // much more future-proof than this string monkeybusiness.
     res = res.trim().split('\n')
     res = res.sort()
     expect = expect.sort()
     var ok = true
-    expect.forEach(function (line, i) {
-      if (ok) {
-        ok = t.equal(res[i].substr(0, line.length), line)
+    for (var e = 0, r = 0;
+        e < expect.length && r < res.length && ok;
+        e++, r++) {
+      // skip the type: global that newer nodes add to the error.
+      if (res[r].match(/^\s*type: global$/)) {
+        e --
+        continue
       }
-    })
+      ok = t.equal(res[r].substr(0, expect[e].length), expect[e])
+    }
     t.end()
   })
   tt.end()
 })
 
 test('cleanup', function (t) {
+  return t.end()
   t.plan(2)
   fs.unlink('segv', function (er) {
     t.ifError(er, 'clean up segv')