Error thrown yet the test fails

[Berkmann18]

Prerequisites

• Checked that your issue hasn't already been filed by cross-referencing issues with the faq label
• Checked next-gen ES issues and syntax problems by using the same environment and/or transpiler configuration without Mocha to ensure it isn't just a feature that actually isn't supported in the environment in question or a bug in your code.
• 'Smoke tested' the code to be tested by running it outside the real test suite to get a better sense of whether the problem is in the code under test, your usage of Mocha, or Mocha itself
• Ensured that there is no discrepancy between the locally and globally installed versions of Mocha. You can find them with: node node_modules/.bin/mocha --version(Local) and mocha --version(Global). We recommend avoiding the use of globally installed Mocha.

Description

When I'm expecting an error to be thrown, mocha highlights in red the relevant it block but without any details (when the assertion inside should pass.
When I'm changing the expect assertion to expect no thrown errors, it shows up on the console (as I would expect from NodeJS) like it was supposed to throw an error (which is expected).

Steps to Reproduce

Test code:

const Server = require('serverbuilder');
const smallApp = (req, res) => {};

it('should alert', () => {
  let port = 5e3;
  expect(() => { new Server(smallApp, port) }, 'EADDRINUSE').to.throw(`Port ${port} is already in use`);
});

Use mocha test --exit as the NPM test script and run npm test

Expected behavior: [What you expect to happen]
1) should alert should pass (ie. be in grey on the CLI).
Actual behavior: [What actually happens]
1) should alert fails since it's in red.

Reproduces how often: [What percentage of the time does it reproduce?]
Everytime even with the following test code:

const Server = require('serverbuilder');
const smallApp = (req, res) => {};

it('should alert', () => {
  let port = 5e3;
  try {
    expect(() => { new Server(smallApp, port) }, 'EADDRINUSE').to.throw(`Port ${port} is already in use`);
  } catch (err) {
    console.log('alert err:', err);
  }
});

Which leads to this on the output:

should alert:
     Uncaught Error: Port 5000 is already in use
      at Server.<anonymous> (index.js:340:17)
      at emitErrorNT (net.js:1317:8)
      at process.internalTickCallback (internal/process/next_tick.js:72:19)

Versions

• The output of mocha --version and node node_modules/.bin/mocha --version: 5.2.0
• The output of node --version: v11.1.0
• The version and architecture of your operating system:
• Your shell (bash, zsh, PowerShell, cmd, etc.): Linux tuxer 4.18.16.a-1-hardened Add tagging #1 SMP PREEMPT Sat Oct 20 17:34:54 CEST 2018 x86_64 GNU/Linux (bash 4.4.23)
• Your browser and version (if running browser tests): N/A
• Any other third party Mocha related modules (with versions): N/A
• The code transpiler being used: N/A

Additional Information

[boneskull]

I'm going to guess that Server attempts to bind to a port, but does so asynchronously (because that's the only way it can do so). It can't bind because the address is in use, so the Error is thrown asynchronously and ends up uncaught.

Effectively, something like this happens:

try {
  setTimeout(() => throw new Error());
} catch (err) {
  // do stuff
}

The catch doesn't catch the Error thrown, because it's thrown asynchronously. The same thing is happening in your test.

This is more of a design issue; if I'm correct, the code under test isn't easily testable.

My suggestion is to make a separate method which will actually bind to the port (you could also accept a callback parameter in your constructor, or use an EventEmitter). The method should accept a callback or return a Promise. Then you can make assertions about its behavior.

[Berkmann18]

@boneskull So I guess using expect(fx).to.throw is useless in situations like this unless it's Promiseable right?

[boneskull]

Yes, IIRC Chai doesn’t have Promise-handling stuff build in. Try chai-as-promised plug-in. Or try unexpected which has support built-in and will be familiar to Chai users.

[Berkmann18]

I tried chai-as-promised and ended with this test case:

it('should alert', () => {
  let port = 5e3;
  let fx = () => { new Server(smallApp, port, {}) };
  return fx().should.eventually.throw(`Port ${port} is already in use`);
}

But it fails even if I use not.

I also tried unexpected with this:

it('should alert', () => {
  let port = 5e3;
  let fx = () => { new Server(smallApp, port, {}) };
  unexpect(fx, 'to throw', `Port ${port} is already in use`);
}

That also fails but it doesn't seem to give as much details as I expected after looking at the website.

[boneskull]

Your constructor returns a Server object, not a Promise. Unless a Promise is returned, you can't make assertions about Promises.

Again, my advice is to not attempt to make a connection in the constructor.

If you feel you must, you can do this:

const {EventEmitter} = require('events');

class Server extends EventEmitter {
  constructor(thing, port, opts) {
    super();

    connect(err => {
      if (err) {
        this.emit('error', err);
      }
    });
  }
}

Then you can test it like this:

it('should alert', function(done) {
  const server = new Server(smallApp, port, {});
  server.on('error', err => {
    expect(err.message).to.equal(`Port ${port} is already in use`);
    done();
  });
});

[boneskull]

If you go this route, chai-eventemitter is helpful.

[Berkmann18]

@boneskull Thanks for the suggestion, I'll look into refactoring it and see if it's more testable.

[Berkmann18]

I've managed to get around this issue (by refactoring the code where the connection is done in an async run method) but errors like EACCES and such are even harder to deal with when it comes to catching and testing them.