Nock: Test runner

[paulmelnikow]

tap is a bit of a bear.

It's slowing me down, though I'm not sure by how much.

Mostly tap is unfamiliar; though an unfamiliar test runner is a still a hurdle to contribution.

Some things it does well, like making sure a certain number of assertions are called or holding up a test until two things have each finished. Though those could be accomplished with Promise.all and an async test with an assertion on sinon.stub().

Some of the things I find odd:

1. The requirement for t.end().
2. That it doesn't print a full stack trace on an exception.
3. When things go wrong in some async code, the test just hangs. (Though maybe that would be fixed by an unhandled promise rejection handler?)
4. Sometimes tests hang when everything seems to be fine, on account of the runner – because I've miscounted t.plan or not put the t.end in the right place. IIRC this is something @jlilja and @RichardLitt were running into as well.

There are a chunk of data-driven tests in the codebase (maybe mostly in test_common.js?) In other projects I've taken to writing tests like those using Sazerac. It makes them much easier to read and write and easy to read when they fail. Unfortunately it doesn't support tap.

If we decided to migrate, I imagine we'd need to mix two runners for a while. That seems like a bit of a drag. Though tap is _so_ unopinionated, I imagine there is a way to make it coexist with another runner.

I'm most familiar with Mocha, and especially like the way it handles async. Had a couple bad run-ins with Jest recently (metabolize/apollo-resolver-gcs#4), though I'm game to give it another try, especially if someone wants to champion it.

Could also wait this out – to see if Jest fixes these issues or if we get used to tap.

[gr2m]

I've yet to find a test runner that works great :) I think "mocha" is probably good choice in the longer term when we want to run tests in both Node & browsers. But for now honestly I'd just stick with tap.

I'm not a big fan of things like Sazerac that packages other things up, it's nice when it works, it's hard when there are subtle problems.

I'm not a fan of Jest either.

Another contended is ava, there are great people working on it and I like the design of it. But they just released their v1 and I expect some more problems to be fixed up as they were so many changes.

[paulmelnikow]

For what it's worth, Sazerac doesn't package up a runner, it just registers tests with whichever runner you're using. I've only run into two issues using Sazerac with Mocha: it unexpectedly no-ops when given() is invoked outside of test(). That's not great, though I've only run into it once. The second was a trivial bug which I fixed: mikec/sazerac#12.

Nock doesn't have a ton of data-driven tests; what I did in #1306 is probably sufficient.

But for now honestly I'd just stick with tap.

I'm fine with that for now. Will close this and see how it goes, and comment or mention this issue if/when tap-related challenges arise.

[paulmelnikow]

Is it possible to exclusively run a subtest? If I .only the parent test, the entire thing runs. If I don't, none of it runs.

[gr2m]

I think you have to add the {only: true} setting to both, the parent and the child.

I general I try to avoid sub-tests if possible, where did you run into it?

[paulmelnikow]

Makes sense. Looks like they're in test_back.js, test_basic_auth.js, and test_intercept.js. They're there for grouping more than anything else, and surely could be flattened.

[gr2m]

also we ca split up into more files, I think test_intercept is quite huge

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue and add a reference to this one if it’s related. Thank you!

[paulmelnikow]

I'm reading this stack trace from https://github.com/nock/nock/pull/1336#issuecomment-451014272:

./tests/test_intercept.js ......................... 529/531
  delay works with replyWithError
  not ok this is an error message
    stack: >
      end (lib/request_overrider.js:28:785)

      lib/request_overrider.js:13:2146

      OverriddenClientRequest.RequestOverrider.req.write
      (lib/request_overrider.js:13:1355)

      OverriddenClientRequest.RequestOverrider.req.end
      (lib/request_overrider.js:13:1924)

      Object.module.get (lib/common.js:41:287)

      Test.<anonymous> (tests/test_intercept.js:4008:20)
    at:
      line: 28
      column: 785
      file: lib/request_overrider.js
      function: end
    test: delay works with replyWithError
    source: |
      function setHeader(request, name, value) {

The line numbers seem really wonky.

e.g. lib/request_overrider.js:13:2146 which is line 13, character 2146.

Any idea what's going on?

[paulmelnikow]

Huh… are the line numbers changing because const declarations are being hoisted?

[paulmelnikow]

This test is giving me not ok test unfinished. I tried adding a t.end() but no dice.

test('reply with implicit date header', { only: true }, async t => {
  const clock = lolex.install()
  const date = new Date()

  const scope = nock('http://example.test')
    .replyDate()
    .get('/')
    .reply(200, { hello: 'world' })

  const { headers } = await got('http://example.test/')

  t.equal(headers.date, date.toUTCString())

  scope.done()
  clock.uninstall()
})

Update: This particular issue was related to got + timers, not tap: #1785

[gr2m]

that usually means that some process is still hanging. I’ll have a look

[paulmelnikow]

That makes sense. I didn't manage to track it down, though. I know you saw in #1359 but for benefit of anyone else reading, I added a callback-based version instead.

[jlilja]

I can agree that tap is at times strange to work with. Before this I'd never heard of tap before.

I've only worked with projects where we've migrated from mocha so I don't really have an opinion on if I like mocha or not, but I found it much more readable than comparing to tap.

Jest is where I'm at usually, but if there are issues like @paulmelnikow mentions that could be a blocker somewhere along the way then I'm ok with looking for another test runner that's right for the job.

[gr2m]

As we keep running into these weird test cases, we could use them as an opportunity to try another runner. I’m tending towards giving ava a try, but happy to use mocha, too.

[paulmelnikow]

After stepping away from the library for a couple weeks, I'm back and running these tests again.

With that perspective, I'm struck by the poor ergonomics of the output (compared to Mocha). It's hard to find the failing tests, and the stack traces aren't formatted in a way that makes them easy to find.

I'm also struck by the relative difficulty of running individual tests.

It's clear that the test runner is slowing down the development process. So, I think it makes sense to plan a migration to a new test runner.

Where to start? #1427 is a "greenfield" test project. That could be a good opportunity to take ava for a spin.

Added: One more gripe: the difficulty of adding global hooks!

[gr2m]

+1 from me, let’s give Ava a try. Thanks Paul for all the great work on the tests!

[RichardLitt]

Ava has always treated me well. I do wish it was easier to get individual tests working - I feel it's a bit hard, at the moment. let's do it.

[paulmelnikow]

Yikes! Today I discovered: since t.rejects technically creates a _descendent test_, our afterEach hook runs after each t.rejects. This led to unexpected results, where assertions after a t.rejects intended to test the post-reject-pre-cleanup state are instead testing the post-cleanup state.

I'm not sure how to fix that in the short term, other than to replace the calls to t.rejects with a non-tap assertion.

[RichardLitt]

That's not ideal. Maybe this is just another reason to get rid of tap?

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We try to do our best, but nock is maintained by volunteers and there is only so much we can do at a time. Thank you for your contributions.

[paulmelnikow]

Tap often produces incomprehensible test failures. Here is my second of the day:

 FAIL  ./tests/test_intercept.js
 ✖ type is null

  tests/test_intercept.js
  18 |   ...Object.keys(global),
  19 |   '_key',
> 20 |   '__core-js_shared__',
  21 |   'fetch',
  22 |   'Response',
  23 |   'Headers',

  --- wanted
  +++ found
  -"null"
  +object

  test: ./tests/test_intercept.js match domain using regexp with path as callback
  stack: |
    Request._callback (tests/test_intercept.js:20:387)
    self.callback (node_modules/request/request.js:185:22)
    Request.start (node_modules/request/request.js:753:10)
    Request.end (node_modules/request/request.js:1511:10)
    end (node_modules/request/request.js:564:14)
    Immediate._onImmediate (node_modules/request/request.js:578:7)

 FAIL  ./tests/test_intercept.js
 ✖ Cannot read property 'statusCode' of undefined

  tests/test_intercept.js
  18 |   ...Object.keys(global),
  19 |   '_key',
> 20 |   '__core-js_shared__',
  21 |   'fetch',
  22 |   'Response',
  23 |   'Headers',

  test: match domain using regexp with path as callback
  stack: |
    Request._callback (tests/test_intercept.js:20:440)
    self.callback (node_modules/request/request.js:185:22)
    Request.start (node_modules/request/request.js:753:10)
    Request.end (node_modules/request/request.js:1511:10)
    end (node_modules/request/request.js:564:14)
    Immediate.<anonymous> (node_modules/request/request.js:578:7)
  type: TypeError
  tapCaught: uncaughtException

In this case I can understand the test by name, though it would be really nice if it pointed to the correct line number where the exception was thrown rather than line 20 which clearly has nothing to do with it.

This kind of friction is frustrating.

As far as a path forward with another test runner, I'm inclined to use Mocha which I know well and can work in the browser. I'm open to using Ava but I'd want to gain some experience with it in a smaller, less complicated project like #1427.

[RichardLitt]

Mocha sounds fine to me. Tap seems to be a bit too buggy. :/

[gr2m]

+1 for Mocha. I don't think Tap is buggy, but it is more constraint as doesn't create magical global behavior as Mocha or Jest.

[mastermatt]

I'm also partial to Mocha, mostly because it's what I use daily at work.
Is there a good way to tackle this piecemeal or should one of us just crank out a conversion of the existing tests?

[paulmelnikow]

Mocha it is! ☕️

I suppose we could start by replacing the _assertions_ piecemeal. In particular the tests which involve t.plan() will need their logic reworked (either by setting variables or calling ~stub~ spy functions). I usually use Chai for assertions; are there alternatives we should consider?

With the assertions migrated away from tap, it might be possible to put a mock layer in place that could invoke Mocha's it under the hood. If so, we could make the rest of the conversions piecemeal.

[paulmelnikow]

At https://github.com/nock/nock/pull/1890#issuecomment-584182161 I was encouraging @nikaspran to convert some of the modules to Chai assertions (and then to Mocha DSL).

@mastermatt just opened #1891 for allow_unmocked.

I've go a PR up #1811 for Mocha DSL in test_logging.

Going forward, I'd suggest we all post here when we're starting work on something new. That way we won't duplicate effort.

[paulmelnikow]

1891 reminds me that we're also in the process of rewriting as many of our tests as possible to use async/got. (A few things, like the mock surface, can't be tested through got, and instead are tested using http.request.) In general, all the mikealRequest tests should be rewritten using got.

[mastermatt]

1891 was merged that converted the allow unmocked tests to Chai asserts and Got.

I'm going to follow it up by updating the same tests to the Mocha DSL now.

Update: https://github.com/nock/nock/pull/1894

[mastermatt]

I'm going to tackle test_common.js next.

Update: https://github.com/nock/nock/pull/1912 & https://github.com/nock/nock/pull/1919

[paulmelnikow]

I'll start on test_intercept. Might do this one in pieces since it's very large.

[mastermatt]

I opened a PR for assertions on destroy and dynamic response bodies: https://github.com/nock/nock/pull/1938
And I'm starting on the delay tests next.

[github-actions[bot]]

:tada: This issue has been resolved in version 13.0.9 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: