expectAsyncConsoleError not throwing error

[zhouyx]

I tried to use the expectAsyncConsoleError() method, but it's not failing a unit test as I expected. Am I missing anything?

Here's my fake test (#28439 is the draft PR)

describe('test', () => {
  afterEach(() => {
    console.log('done');
  });
  it('my fake test', () => {
    expectAsyncConsoleError('fake message');
  });
});

The test passed. Got

When I tried to debug it, I noticed the test was run twice and failed the second time.

[zhouyx]

cc @micajuine-ho

[rsimha]

@zhouyx The source of this bug is #20091, but the root cause lies in mochajs/mocha#1635. Here's my understanding of what is going on:

• If a test cannot predict when an asynchronous expected console.error will actually happen, it can call expectAsyncConsoleError() at the start
• At the end of each test, we do a check in afterEach() to see if the error message was indeed encountered
• Originally, we were calling throw new Error(...) in afterEach() if an expected error wasn't seen
• The problem with throwing is that subsequent tests will not run (🐛 Bug: afterEach halts suite and/or causes simultaneous failure and pass mochajs/mocha#1635)
• In Update error handling after each Mocha test #20091, the throw new Error(...) was effectively changed to this.test.error(new Error(...))
• It turns out that calling this.test.error(new Error(...)) in afterEach doesn't actually fail the test as described here (and as you observed), because it fails a test after it has passed! (see fix report in a case of afterEach conditional fail mochajs/mocha#3362, which was reverted)

The only option I can think of is to go back to throw new Error(...) in afterEach (revert #20091), but this will halt all tests when the first violation of expectAsyncConsoleError is encountered. Is this acceptable to you?

[zhouyx]

Thank you @rsimha for the detailed explanation. Sign...
I find #20091 to be very useful. Killing karma due to a failing test sounds bad.

Can we avoid verifying the console error in afterEach() block? What about having expectAsyncConsoleError() to return a promise and request individual test to wait for that promise to be resolved.

[rsimha]

I find #20091 to be very useful. Killing karma due to a failing test sounds bad.

Totally agree!

Can we avoid verifying the console error in afterEach() block? What about having expectAsyncConsoleError() to return a promise and request individual test to wait for that promise to be resolved.

The original goal of making expectAsyncConsoleError a single-line call instead of a wrapper like allowConsoleError was to make tests easy to write. With your suggestion, will the usage of expectAsyncConsoleError effectively change to a wrapper that waits for the expected error? (All existing usage will need to be converted.)

In any case, I have no objections to your suggestion if it improves the correctness of the test framework. So if you have the bandwidth, go for it!

Related: #15609

[zhouyx]

Yes, all the existing usage will need to be converted. Some of them may silently fail today, will also need to fix that after the conversion.
Can be a good fixit week item : )

[samouri]

I also ran into this issue today. Tracked it down to a bug in karma-mocha: karma-runner/karma-mocha#236.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.