Switch to a new testing system

[PlasmaPower]

Frameworks

Framework	Simultaneous Testing	Multiple Processes For Multiple Files	Global Error Handling	Builtin Babel	NPM Downloads/Month
Mocha (current)			✓
Ava	✓	✓		✓
Jest	✓	✓		✓

Request Libraries

Request Library	Uses Promises	Builtin Response Testing	Response Testing Covers Common Use Cases	Reasonable Defaults For Testing	NPM Downloads/Month
Superagent				✓
Supertest (current)		✓		✓
Supertest-As-Promised	✓	✓		✓
Assert-Request	✓	✓	✓	✓
Request				✓
Request-Promise	✓
Node-Fetch	✓			✓
Requisition	✓			✓

Notes

• (current) indicates that Koa currently uses it.
• If we use a framework with builtin babel support, we can easily use async/await in place of builtin response testing if the library supports promises.
• If a request library doesn't have reasonable defaults, we can easily create a wrapper that does.
• If the response testing doesn't cover common use cases, a custom helper assertion function can be used instead.

If anybody has any more columns or rows to add to either table, please leave a comment.

[juliangruber]

feature matrixes don't work for software, as apple so well demonstrates. what sucks about current tests?

[tj]

I'm skeptical of stuff like this because the ecosystem will be completely different in a few months. Though I won't stop anyone who wants to refactor things all day :D

[PlasmaPower]

what sucks about current tests?

For one thing, you can't easily detect that there isn't a header (that's the lack of "Response Testing Covers Common Use Cases" column). Also in rare situations it would be useful to have a .then to assert something set elsewhere rather than a custom end method. Another thing is that as tests grow speed matters more, and with how split up tests are multiple processes for multiple files will be very important. Lastly, if we do switch our request library to something without builtin request testing, having async/await will be very nice (builtin babel will be required as that isn't in the near future for node).

[PlasmaPower]

the ecosystem will be completely different in a few months

I'm not sure it will change that much, given that libraries take time to accumulate popularity. Also, if we find a toolset that works well for the job, there's little reason to change.

[PlasmaPower]

feature matrixes don't work for software

I think that in the case of these libraries, they make sense. If you have anything to add that doesn't fit in the table, I can make a note.

[dougwilson]

In mocha

request(server)
.get('/')
.expect(418)
.expect('Content-Type', 'text/plain; charset=utf-8')
.expect('Content-Length', '4')
.end(function(err, res){
  if (err) return done(err);

  res.headers.should.not.have.property('vary');
  res.headers.should.not.have.property('x-csrf-token');

  done();
})

should be written as

request(server)
.get('/')
.expect(418)
.expect('Content-Type', 'text/plain; charset=utf-8')
.expect('Content-Length', '4')
.expect(shouldNotHaveHeader('Vary'))
.expect(shouldNotHaveHeader('X-CSRF-Token'))
.end(done)

with the helper

function shouldNotHaveHeader(header) {
  return function (res) {
    assert.ok(!(header.toLowerCase() in res.headers), 'should not have header ' + header)
  }
}

[PlasmaPower]

@dougwilson Makes sense, I've added a note about that.

[fl0w]

This change feels larger than it probably is, but I'd keep things as they are until v3 and to further analyse tools when ecosystem stabilises post native v8 async/await.

[PlasmaPower]

For scale, IIRC it took me 1.5 hours to change supertest to assert-request. Changing the testing framework as well would be much larger.

[jonathanong]

This change feels larger than it probably is, but I'd keep things as they are until v3 and to further analyse tools when ecosystem stabilises post native v8 async/await.

@fl0w 👍 i've been writing my latest tests w/ async functions using babel and it's been a lot easier

it('should do something', async () => {
  const response = await request('http://...')
  assert.equal(200, response.status)
})

i think it's worth waiting until then

[jonathanong]

another note: i HATE using should. i prefer if we stopped using anything but require('assert').

[PlasmaPower]

@jonathanong +1 on stopping using should, I think that es6 array functions covers pretty much everything it's needed for. What request library are you using with async/await? Also, ava has builtin babel support but IDK if that would slow things down. It also can test files simultaneously, which might result in a speed up.

[haoxins]

What request library are you using with async/await?

Take a look at thenables/requisition :)

[PlasmaPower]

@coderhaoxin Looks neat. I've added it to the table. Comments from skimming through it:

• I like the chaining API
• The Readme.md seems to be somewhat broken in parts
• Making response.text a promise is interesting, personally I don't think it's necessary
• You seem to be handling content-encoding yourself, I don't mind that but you do it in two places (you might want to make a function)
• I think memoizing everything is excessive, and a waste of memory
• You have "TODO: use lodash" in your code, but only once. I'd highly recommend against that
• You have this code twice in index.js, replace it with Object.assign({}, defaults, opts)

[fl0w]

@jonathanong How do you choose to transpile your tests? Babel require hook inside each test? node -r?

[jonathanong]

have an entry point like the following:

// test/index.js
require('babel-register') // or whatever entry point it is now
require('./test1')
require('./test2')

then run it like:

mocha test/index.js

[jonathanong]

ava seems pretty cool just from a simplicity perspective. i don't really like the t argument, though.

the only issue i have with supertest is that it doesn't support promises. not sure if there is any really solid rooms for improvement there

[blakeembrey]

@jonathanong Slightly off-topic, but I wrote and use https://github.com/blakeembrey/popsicle with https://github.com/blakeembrey/popsicle-server to replicate the basic behaviour of supertest while using a promise-based API. The middleware of Popsicle is actually pretty much identical to Koa-2.x now (I actually wanted to use Koa-compose, but I support node 0.10 and the const wouldn't allow me to).

[PlasmaPower]

Once Node v7 comes along with async/await support, will we still be testing Koa v2 on earlier versions?

[dtothefp]

@jonathanong you can hack around with supertest and do something like this

import Test from 'supertest/lib/test';

const {end} = Test.prototype;

Test.prototype.end = function(cb) {
  const self = this;

  if (isFunction(cb)) {
    return end.call(self, cb);
  }

  return new Promise((res, rej) => {
    end.call(self, (err) => {
      if (err) return rej(err);

      res();
    });
  });
};

then

it('should run a basic route', async () => {
  await agent.get('/')
    .expect(200)
    .end();
});

[PlasmaPower]

@dtothefp It still has the problem of .expect being confusing/complicated/missing some functionality due to it's design. Still though, better than a callback IMO.

[bidanjun]

yes，i write 64 tests in two days using ava+supertest-as-promised, port express-graphql to koa v2...
https://github.com/bidanjun/koa-graphql-next. it's easy to use...and seems ,not hard work..

[fundon]

Already landed in https://github.com/trekjs/engine/tree/master/test.
trek-engine inspired by koa. Thanks to Koa Team.

Need a PR?

[PlasmaPower]

I'm always surprised by how many forks Koa has inspired.

I think we'll just rewrite our tests in async once it lands in all supported versions of Node.

[jonathanong]

maybe consider jest. lol. looked better than ava to me at a glance.

[PlasmaPower]

Jest looks like the best I've seen so far.

[fl0w]

@PlasmaPower alright, I'll hold off then. Let me know if you need/want to off load.

README also updated to 7.6 only support so I would assume using async-await to be fine?

[PlasmaPower]

@fl0w it says that only for using async/await though.

[PlasmaPower]

Actually I guess I misread it. I thought it required a new node if you want async/await, but I think in reality it requires a new node so it has async/await.

[PlasmaPower]

For the request library, I think we should use node-fetch. With async/await it looks like the best option.

[fl0w]

@PlasmaPower considering 0c2d881 I would assume it's ok! :)

[PlasmaPower]

I've setup the basis for network tests this way: https://gist.github.com/PlasmaPower/57d361a6d0a90c7a5a6aac890cd2322d

Look good?

[jonathanong]

yes, we dropped babel from tests and only support node v7.6+ to make life easier! feel free to use async functions in tests

[jonathanong]

there should be minimal changes in tests though. only moving around test files to make them jest-like and require('should') where appropriate.

[PlasmaPower]

One problem I'm having is jest seems to break test-console. I'll look into why and alternatives or fixes.

[PlasmaPower]

I fixed that with a custom console.error override. More problems have arisen though. It seems jest overrides more than I thought.

[jonathanong]

I haven't really experienced jest breaking anything. Have you tried setting the environment to node?

[PlasmaPower]

I'm assuming you meant setting the node environment to test, but jest does that automatically.

The problem I'm currently encountering is that this assertion fails for this test. I have no idea why, the error is quite odd:

Error: non-error thrown: Error: ENOENT: no such file or directory, open 'does not exist'

I'll do some more investigation into that error object.

[PlasmaPower]

More information:

process.stderr.write(err + '\n');
process.stderr.write((typeof err) + '\n');
process.stderr.write(err.constructor + '\n');
process.stderr.write(Error + '\n');
process.stderr.write((err instanceof Error) + '\n');

gives

Error: ENOENT: no such file or directory, open 'does not exist'
object
function Error() { [native code] }
function Error() { [native code] }
false

Very odd. JS objects are confusing. I'm also not sure why Jest causes this, it doesn't seem to be messing with the Error object as I initially thought.

[fl0w]

@PlasmaPower I think jonathanong was referring to set jest to omit mocking a browser environment by having following in package.json:

{
  "jest": {
    "testEnvironment": "node"
  }
}

[PlasmaPower]

@fl0w Oh, I didn't know jest did that. I'll try changing that and see if it does anything.

[PlasmaPower]

The error type problem is still maintained, but I'll keep it in the config of course.

[PlasmaPower]

I've pushed my work to this branch if anyone's interested: https://github.com/PlasmaPower/koa/tree/jest-tests

[PlasmaPower]

I'm getting more and more wary of jest. Its breaking stuff knows no bounds. The basic problem is that Node's native libraries (fs, http) will have a different version of objects than JS. This causes numerous problems:

• in Koa: err instanceof Error fails with a Node fs error + jest
• in node-fetch: headers[prop] instanceof Array fails for values in a Node header object (particularly nasty as it only manifests when a header has multiple values, like set-cookie)

[PlasmaPower]

jestjs/jest#2549

[PlasmaPower]

See also: node-fetch/node-fetch#220

[fl0w]

Why are you switching from supertest to node-fetch though? It's probably easier to just upgrade to v3 which supports promises. Maybe take node-fetch on next refactor iteration?

[dead-horse]

keep supertest is ok for me, no need to replace it.

[PlasmaPower]

IDK, I'd prefer to do it all at once. There's little point in async/await without node-fetch. Switching to node-fetch hasn't been too much of an issue, and the headers issue is gone with an upgrade to the v2 alpha.