Koa: 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 :+1: 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.

[L3au]

vote for jest.

[bidanjun]

yes,use jest...
im use it in an graphql/relay/koa project...and I've been use ava in koa-graphql-next....using jest,could simplely test react and server side,for now,it's faster than ava.js.....
im using jest+supertest-as-promised at current project,no trouble....

[jonathanong]

we should switch to jest. it would remove the need for the makefile options as a lot of it is just to list the files. the only problem I see is should as I don't think jest can easily inject it. we can get rid of the makefile as tests would just be something like jest --testEnvironment node --coverage

anyone want to convert the test framework to jest?

• [ ] make the current tests work

[jonathanong]

would make https://github.com/koajs/koa/issues/559 easier

[fl0w]

@jonathanong Would you be okay with tests using async-await (i.e. only support node 7.6+) or is that a leap made to early?

[fengmk2]

jest look good :)

[PlasmaPower]

I'll start working on that.

@fl0w jest comes with babel support. I'll stick with that for now.

[PlasmaPower]

Actually, it looks like Travis is currently set to only 7.6. I'll not use the babel plugin for now.

[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 https://github.com/koajs/koa/commit/0c2d881b671c0d36c2a944bb525013c61bc1829b 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]

facebook/jest#2549

[PlasmaPower]

See also: bitinn/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.