pinojs / pino-http

🌲 high-speed HTTP logger for Node.js
MIT License
549 stars 116 forks source link

rewrite all tests to avoid timers #81

Open mcollina opened 4 years ago

mcollina commented 4 years ago

Timers are known to cause flaky test. It's important we fix this before it hit us.

baterson commented 4 years ago

Hello @mcollina I would like to take this Issue. You want to rewrite all tests with setTimeout calls but not all of the tests? If so I have one question. In the case of this test:


test('responseTime', function (t) {
  var dest = split(JSON.parse)
  var logger = pinoHttp(dest)

  function handle (req, res) {
    logger(req, res)
    setTimeout(function () {
      res.end('hello world')
    }, 100)
  }

  expectResponseTime(t, dest, logger, handle)
})

I can rewrite it in this manner:

test('responseTime', function (t) {
  var dest = split(JSON.parse)
  var logger = pinoHttp(dest)

  setup(t, logger, function (err, server) {
    t.error(err)
    doGet(server)
  })

  dest.on('data', function (line) {
    t.ok(line.responseTime, 'responseTime is presented')
    t.end()
  })
})

But what about tests like this one when you need to wait some time to be ensure that nothing was logged

test('no auto logging with autoLogging set to false', function (t) {
  var dest = split(JSON.parse)
  var logger = pinoHttp({ autoLogging: false }, dest)
  var timeout

  setup(t, logger, function (err, server) {
    t.error(err)
    doGet(server)
  })

  dest.on('data', function (line) {
    t.ok(line.responseTime, 'responseTime is presented')
    t.end()
  })

  function handle (req, res) {
    logger(req, res)
    setTimeout(function () {
      res.end('hello world')
    }, 100)
  }

  dest.on('data', function (line) {
    clearTimeout(timeout)
    t.error(line)
    t.end()
  })

  setup(t, logger, function (err, server) {
    t.error(err)
    doGet(server)

    timeout = setTimeout(function () {
      t.end()
    }, 200)
  }, handle)
})

Could you give a tip on how to handle those cases with timeout please?

mcollina commented 4 years ago

You can call t.fail() in case the something is logged: in that case an error will fail the full suite instead.