this.test.error() in "after all" hook should throw

[papb]

Prerequisites

• [X] Checked that your issue hasn't already been filed by cross-referencing issues with the faq label
• [X] 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.
• [X] '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
• [X] 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 that you not install Mocha globally.

Description

Steps to Reproduce

• mkdir temp && cd temp && npm init -y && npm i -D mocha

• Create test/test.js:

const assert = require('assert');

describe('First test block', function() {
	after(function() {
		// I know this code doesn't make sense in real life, it's just to show the issue
		this.test.error(new Error());
	});

	it('First test', function() {
		assert.strictEqual(true, true);
	});
});

describe('Second test block', function() {
	it('Second test', function() {
		assert.strictEqual(true, true);
	});
});

• Run npx mocha test/*.js && echo 'ok'

Expected behavior: Mocha should exit with nonzero exit code and tell me that something is wrong.

Actual behavior:

  First test block
    √ First test
ok

Mocha completely swallowed the error and exited with exit code 0. Some tests didn't even run!

Reproduces how often: Every time

Versions

• The output of npx mocha --version: 8.2.1
• The output of node --version: v12.14.1

[papb]

Hello, @boneskull! I decided to ping you because I think this issue is quite severe.

[boneskull]

I'm surprised that this.test.error does anything. it's a rather obscure API, and I don't recall ever seeing a test for it. I'll have to go back to see when and why it was added and what it's supposed to do...

[papb]

Thank you for the fast reply! I learned about that API in issue #1635. See this comment.

[boneskull]

~~I'm not sure when this API was removed, but it was a long time ago. this.test.error() is not part of Mocha's API.~~

edit: was not removed, just not obvious

[papb]

@boneskull You mean undocumented, right? Because it was definitely not removed. I am using it successfully right now.

Edit: actually it is still documented: https://github.com/mochajs/mocha/wiki/HOW-TO:-Conditionally-fail-a-test-after-completion

[boneskull]

no, you're right, it still exists.. it's just that the function doesn't seem to be meaningfully tested in any way.

well, we need tests for it, then we need to figure out what broke it...

[boneskull]

also: don't rely on the wiki for documentation if you can help it. if you don't see an API mentioned on mochajs.org, you probably don't want to use it.

the API here is documented as a public API of the Hook object, but that's intended for programmatic users.

[boneskull]

OK, so:

1. I've confirmed this doesn't work with "after all" (after) hooks.
2. I have not confirmed that it should work with after hooks.
3. I'm going to guess is that it should not work with "after all" hooks, because an "after all" hook, conceptually, does not have its own test context. In other words, a failure in an "after all" hook could not tell you which test it was supposed to fail. Think of it as bound to the suite, not the test.
4. I've confirmed this does work with "after each" (afterEach) hooks, because these hooks share context with a test.

Still, there were no tests asserting this behavior works (I've now written one).

But, we could improve the situation in two ways:

1. "Officially" document the API, and/or
2. Throw if this.test.error() is used in an "after all" hook.

I am not sure I'd want to encourage use of this API by adding it to the official docs, but I think it would have saved you some time if it threw.

If anyone is interested in helping, I think what I'd like to see is 2. above; make this.test.error() throw with a reasonable error message if called from an "after all" hook. Add a error factory function and associated error code to lib/errors.js, and tests/fixtures for the change.

[boneskull]

I wonder if @evaline-ju would be interested in this one

[boneskull]

@papb to be clear, rewriting your example as this works:

const assert = require('assert');

describe('First test block', function() {
	afterEach(function() {
		// I know this code doesn't make sense in real life, it's just to show the issue
		this.test.error(new Error());
	});

	it('First test', function() {
		assert.strictEqual(true, true);
	});
});

describe('Second test block', function() {
	it('Second test', function() {
		assert.strictEqual(true, true);
	});
});

[boneskull]

It also looks like this does not provide a reasonable stack trace. I'm going to guess that's because the error is never thrown and the stack is not created. You'd want to read the stack prop of the error or throw it, catch it, then pass it to this.test.error().

We should not change it so errors thrown in "after each" mark the test itself as failed, since that'd break the current behavior. But it's such a flimsy API atm I'm not sure building it out further makes sense. Maybe a custom interface would work.

[papb]

Hi @boneskull, thank you for taking the time to look into this so quickly! I agree with you. In fact, I only stumbled upon this obscure this.test.error API while trying to find a way to have an afterEach hook fail the single associated test, instead of breaking the whole Mocha execution. And while searching I found this comment from four years ago.

I opened this issue because I was worried that I ended up finding something that causes mocha to exit abruptly with exit code 0. How exactly it happens wasn't my focus; I wonder if there are other things that could also cause mocha to exit abruptly with exit code 0, since that's the scary part.

In summary: what I really want is a solution to #1635, but I found this scary behavior in the process of looking for a workaround.

[papb]

We should not change it so errors thrown in "after each" mark the test itself as failed, since that'd break the current behavior.

Maybe it could be put into a new option, such as --dont-bail-on-hooks-if-possible (of course a better name could be used)?

I also considered starting a PR to get a proper solution for #1635, but after skimming at the source code it didn't look so easy.

[papb]

By the way, I think #3345 should be reopened as well.

[boneskull]

yeah, dunno if that one will ever be addressed. the intent of hooks is not as a place to put assertions, though many people seem to want to. in many cases, programmatically generating tests would solve the same problems.

[papb]

programmatically generating tests would solve the same problems

Hmm, this looks interesting, can you give me a link where I can learn more? Or show an example?

[boneskull]

I don't think a new option is on the table. it's just not going to impact enough users to justify the maintenance cost.

if you need to make the same assertions for a bunch of tests, use a loop in a describe block which calls it on each iteration.

describe('something', function() {
  // or use a diff data structure w/ titles, like a Map
  // these should be unique
  const tests = [
    function() { 
      assert.ok(); 
    },
    function() {
      assert.something()
    }
  ];
  
  for (const t of tests) {
    it('x', function() {
      // may want to try/catch here
      t.call(this);
      assert.everyTime();
    });
  }
});

google for dynamic mocha tests. there might be something on the docs site; I don't recall.

[papb]

Ok! Thank you!

[karpov-denys]

I would like to add a question. I work with TS. I can use this.test.error() in my project. TS compile throw error Property 'error' does not exist on type 'TestFunction'. this.test.error(msg); ~~~~~ Has lib any solution for that?