Use jest and node-fetch for tests #926

PlasmaPower · 2017-03-02T01:26:55Z

Resolves #703. One test (currently set to skip) is tied to #848. I'll make the change to unskip it on the PR last to get merged.

codecov · 2017-03-02T01:34:09Z

Codecov Report

Merging #926 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #926    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files           4      6     +2     
  Lines         431    367    -64     
  Branches      103      0   -103     
======================================
- Hits          431    367    -64

Impacted Files	Coverage Δ
test/helpers/request.js	`100% <100%> (ø)`
lib/response.js	`100% <0%> (ø)`	✅
lib/context.js	`100% <0%> (ø)`	✅
lib/request.js	`100% <0%> (ø)`	✅
lib/application.js	`100% <0%> (ø)`	✅
test/helpers/context.js	`100% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebed04f...033a5ef. Read the comment docs.

PlasmaPower · 2017-03-02T01:43:27Z

Hmm... the travis error is very confusing to me. Can anyone replicate it locally?

The denoted file+line is just });. Also, there's no combination of request and done in any test, as the error would suggest. There's also never a circumstance where request(...).get would be accessed.

PlasmaPower · 2017-03-02T01:48:06Z

Oh, that's because the tests were preformed on a rebase, and new tests have been added. I'll fix that.

PlasmaPower · 2017-03-02T02:02:55Z

Rebased. Not sure why Travis is taking some time to run, but it should work when it does.

fengmk2 · 2017-03-02T02:02:58Z

-1

No, we use supertest for a long time and it work great for me.
I don't see any good reason change to use node-fetch.

PlasmaPower · 2017-03-02T02:05:44Z

@fengmk2 Supertest is:

~~no longer maintained~~ Edit: no longer the case
~~based on callbacks not promises~~ Edit: now possible with .end()
without a good way to test for the absence of headers

~~I think that we need to switch at some point in time without a doubt. I'd much prefer to switch now than later.~~

PlasmaPower · 2017-03-02T02:10:32Z

Also, I can't see any advantage of supertest over node-fetch. I've already done all the work to switch, and in some cases the code clearly gets simpler, so why not?

PlasmaPower · 2017-03-02T02:16:30Z

Travis has passed.

dougwilson · 2017-03-02T02:18:14Z

R.I.P. Travis

magicdawn · 2017-03-02T02:30:26Z

supertest is extend from superagent. and superagent has faux Promise support since v1.3.0
https://github.com/visionmedia/superagent/blob/v1.3.0/lib/node/index.js#L1027

-1 on node-fetch. fetch always reminds that's GET 😂

dead-horse · 2017-03-02T02:31:44Z

test/application/use.js

-    });
-
+  it('should output deprecation message for generator functions', () => {
+    const warningPromise = eventToPromise(process, 'deprecation')


use promise here is strange, I'd like to use the original syntax

With the original syntax, the asserts for the result of the request were run after the test has finished. I don't know if they would be ignored or result in a strange error, but I do know that it wouldn't work properly. I can return it to its original syntax and remove the request asserts though.

I was thinking of another use of eventToPromise, I can switch this back.

PlasmaPower · 2017-03-02T02:33:39Z

Ah, I see. I wasn't aware that superagent was updated. ~~However, I still don't like using supertest, as once again it is unmaintained.~~ Edit: no longer the case.

If we'd like to explicitly specify GET, then I can add that to the wrapper API. However, I still support node-fetch as the base.

dead-horse · 2017-03-02T02:34:19Z

test/context/cookies.js

-
-        done();
-      });
+    const res = await request(app, '/');


use supertest@3 with promise support is much better than node-fetch

Looking into it, it does have a new maintainer, so my point there isn't valid. However, I still prefer the syntax of node-fetch. May I ask why you prefer supertest?

Also, I'd like to point out that for the specific test you've commented on using node-fetch is significantly cleaner (12 lines to 3 lines).

Using supertest way

return request(app) .get('/') .expect(204) .expect(res => { assert(res.headers['set-cookie'].join(';').match(/^name=/)); });

I'm assuming even if we switch back to supertest we'd keep expect?

yeah, I think we should stick with supertest@3. fewer changes and it works fine

PlasmaPower · 2017-03-02T02:41:40Z

Sorry for switching to node-fetch without asking first, I thought the consensus was once we had async/await we should move away from supertest.

PlasmaPower · 2017-03-02T02:59:06Z

One other thing about my node-fetch wrapper: it automatically closes the HTTP server, so the Jest process will exit after finishing the tests (not the case with our current supertest usage).

fengmk2 · 2017-03-02T03:20:55Z

without a good way to test for the absence of headers

We can send a pull request to supertest then make it happen.

fengmk2 · 2017-03-02T03:22:23Z

test/application/response.js

-    request(app1.listen())
-      .get('/')
-      .expect(204, done);
+    return request(app1, '/').then(res => expect(res.status).toBe(204));


this equal to supertest

return request(app1) .get('/') .expect(204);

I'm not sure if Jest would recognize it as a Promise, given that it isn't true Promise, it just has .then and .catch.

Looks like jest does recognize it as a promise.

anything with a .then() method is a true promise ;)

PlasmaPower · 2017-03-02T03:33:22Z

The one thing I realized supertest has going for it is that it's error messages are more relevant to the context of the assertion.

At this point, I think supertest is acceptable. I'm still not sure it's better though.

gdams

LGTM, It will be really useful to not have to use make anymore as this will fix all the issues with AIX

gibfahn · 2017-03-02T16:44:31Z

.travis.yml

-  - make test-travis
-  - make bench
+  - npm run lint
+  - jest --coverage


Might it be worth having an npm script for coverage (so you could donpm run coverage)?

I'm not sure that would be very helpful, I think npm test --coverage is good enough.

It's helpful for people who are new to the module, if all the basic testing commands are npm run scripts, then you can just instantly work out what the testing commands are.

How do you know to run npm test --coverage?

gibfahn · 2017-03-02T16:45:11Z

package.json

@@ -4,7 +4,8 @@
  "description": "Koa web app framework",
  "main": "lib/application.js",
  "scripts": {
-    "test": "make test"
+    "test": "jest",
+    "lint": "./node_modules/.bin/eslint benchmarks lib test"


I think you can just do "lint": "eslint benchmarks lib test" (npm should be able to work out where eslint is.

I don't think so, we also don't want to use a global eslint even if it exists (plugins).

npm run prepends ./node_modules/.bin to the $PATH, so it will use the local eslint even if it's also installed globally.

You can test it by adding "tmp": "echo $PATH", to the package.json.

npm run docs

PlasmaPower · 2017-03-02T19:24:38Z

@gibfahn Good points. I've added those, except I chose npm run test-cov instead of npm run coverage as that's what we previously had.

gibfahn · 2017-03-02T19:39:52Z

package.json

-    "test": "make test"
+    "test": "jest",
+    "test-cov": "jest --coverage",
+    "lint": "./node_modules/.bin/eslint benchmarks lib test"


Did you miss committing the lint change? I see it in the commit message 😄

Yeah, oops! Fixed.

PlasmaPower · 2017-03-07T16:23:01Z

Hmm, that Travis failure is very odd. Retrying.

PlasmaPower · 2017-03-07T16:25:44Z

That time it passed. Given that it was a syntax error I'm not sure it was caused by jest. I'll investigate further if it occurs again.

PlasmaPower · 2017-03-08T14:47:37Z

The test relying on #848 is proving to be a bit more difficult than thought. I'll look into it later today.

I thought this was a test only thing, but it turns out that it's global to Koa.

PlasmaPower · 2017-03-08T16:50:31Z

Okay I've updated the socket.writable test (depends on app.callback returning a promise). It's maybe a bit too complicated, but it should be fairly secure against future changes.

jonathanong · 2017-03-15T13:39:34Z

test/application/use.js

-        done();
-      });
+    const res = await request(server, '/');
+    expect(res.status).toBe(404);


not sure I like using expect. anyone else prefer we use node's assert?

It comes with jest, so I figured that's what we'd use.

assert +1, don't like expect either, maybe we can try power-assert later, it has the same API with node's assert.

Hmm since we've managed to avoid transformations thus far, I'm not sure we'd want to add one. However, I agree that it'd be very useful.

jonathanong · 2017-03-15T13:40:47Z

test/context/cookies.js

-        request(app.listen())
-          .get('/')
-          .expect('.keys required for signed cookies', done);
+        const text = await (await request(app, '/')).text();


lol. I always try to only do 1 async action per line. gets crazy like this otherwise

Yeah I probably should've split that up.

jonathanong · 2017-03-15T13:41:53Z

my opinions:

stick to supertest@3 instead of node-fetch, which seems to be the consensus
use assert instead of expect, which I don't really care about

PlasmaPower · 2017-03-15T13:42:46Z

OK, I'll open a new PR with those changes later today hopefully.

jonathanong · 2017-03-15T13:43:55Z

@PlasmaPower thanks for all your work!!!

PlasmaPower · 2017-03-15T22:55:30Z

I'm leaning towards a custom supertest wrapper, as we also need to shutdown the server. Is that okay?

Also, for now, I think I'll implement header not existing tests through that wrapper, then maybe make a PR to supertest in the future.

PlasmaPower · 2017-03-16T23:58:06Z

I haven't had much time to work on this, if anyone wants to pick it up feel free.

PlasmaPower force-pushed the jest-tests branch from 29b7b48 to 9f81563 Compare March 2, 2017 01:57

dead-horse reviewed Mar 2, 2017

View reviewed changes

fengmk2 reviewed Mar 2, 2017

View reviewed changes

PlasmaPower mentioned this pull request Mar 2, 2017

Support UNIX variants where make is not an alias for gmake #884

Closed

gdams approved these changes Mar 2, 2017

View reviewed changes

gibfahn reviewed Mar 2, 2017

View reviewed changes

PlasmaPower force-pushed the jest-tests branch 2 times, most recently from 9fda81a to a5ad829 Compare March 7, 2017 16:22

PlasmaPower added 14 commits March 8, 2017 09:27

Begun context tests

43cb141

Continue context tests, node-fetch v2

8e960e0

Finish context tests

8a14276

Request testing

41a51ca

Start response tests; fix request include

5dac512

Finished response testing

be6cb2c

Remove should from deps

2bd9326

Appease eslint

dfcae87

Run benchmarks for a few more configurations

6f39d58

Add back in beginning newlines

fdcb010

I thought this was a test only thing, but it turns out that it's global to Koa.

Remove duplicate beginning newlines

4494dd8

Update new onerror test

b7de3fa

Remove leading newline for eslintrc

85d0932

Add npm run test-cov, simplify npm run lint

589b112

PlasmaPower force-pushed the jest-tests branch 2 times, most recently from 57fc767 to 28fa978 Compare March 8, 2017 16:48

Fix new header test

5c0e52d

PlasmaPower force-pushed the jest-tests branch from 28fa978 to d456a7b Compare March 8, 2017 16:53

Fix writable test

033a5ef

PlasmaPower force-pushed the jest-tests branch from d456a7b to 033a5ef Compare March 8, 2017 16:57

jonathanong reviewed Mar 15, 2017

View reviewed changes

PlasmaPower closed this Mar 15, 2017

gillesdemey mentioned this pull request May 8, 2017

Use Jest #981

Merged

Use jest and node-fetch for tests #926

Use jest and node-fetch for tests #926

Conversation

PlasmaPower commented Mar 2, 2017

codecov bot commented Mar 2, 2017 • edited Loading

Codecov Report

PlasmaPower commented Mar 2, 2017

PlasmaPower commented Mar 2, 2017

PlasmaPower commented Mar 2, 2017

fengmk2 commented Mar 2, 2017 • edited Loading

PlasmaPower commented Mar 2, 2017 • edited Loading

PlasmaPower commented Mar 2, 2017

PlasmaPower commented Mar 2, 2017

dougwilson commented Mar 2, 2017

magicdawn commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PlasmaPower commented Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PlasmaPower commented Mar 2, 2017 • edited Loading

PlasmaPower commented Mar 2, 2017

fengmk2 commented Mar 2, 2017

fengmk2 Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

PlasmaPower Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PlasmaPower commented Mar 2, 2017

gdams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibfahn Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

gibfahn Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PlasmaPower commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PlasmaPower commented Mar 7, 2017

PlasmaPower commented Mar 7, 2017

PlasmaPower commented Mar 8, 2017

PlasmaPower commented Mar 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanong commented Mar 15, 2017

PlasmaPower commented Mar 15, 2017

jonathanong commented Mar 15, 2017

PlasmaPower commented Mar 15, 2017

PlasmaPower commented Mar 16, 2017

codecov bot commented Mar 2, 2017 •

edited

Loading

fengmk2 commented Mar 2, 2017 •

edited

Loading

PlasmaPower commented Mar 2, 2017 •

edited

Loading

PlasmaPower commented Mar 2, 2017 •

edited

Loading

PlasmaPower commented Mar 2, 2017 •

edited

Loading

fengmk2 Mar 2, 2017 •

edited

Loading

PlasmaPower Mar 2, 2017 •

edited

Loading

gibfahn Mar 2, 2017 •

edited

Loading

gibfahn Mar 2, 2017 •

edited

Loading