Report on failures after test complete.

[samouri]

summary Fixes https://github.com/karma-runner/karma-mocha/issues/236

Note: I am extremely unfamiliar with this repo, feel free to be harsh in your review.

[jginsburgn]

The only concern that I have for now is that the reporters would receive two events for a single spec. The first one of a success and the subsequent one, contradicting the first, of a failure. I wanted to reproduce this but yak shaved on Mocha and ended up filing https://github.com/mochajs/mocha/issues/4656.

[jginsburgn]

@devoto13, could you also take a look at this?

I will try to reproduce the concern that I have. Also, @samouri if you can help with this, we would hasten this PR :)

[jginsburgn]

Maybe we should hold the temporary test end event to wait for the fail event with test.type === test, before reporting it to Karma?

[devoto13]

I've checked how Mocha's built-in reporter does it and it uses pass/pending/fail events instead of test end resulting in much cleaner logic. I wonder if there was a particular reason why we use test end here. I'll try to play with it soon and post back.

More information about Mocha custom reporters: https://mochajs.org/api/tutorial-custom-reporter.html

[devoto13]

I gave the above approach a try and it seems to work as expected. The problem is that existing tests are written to test the existing implementation and it's hard to say if this change will introduce any regressions. Unfortunately, I won't have time to invest a reasonable amount of time into this refactoring in the near future, so if somebody else wants to give it go, feel free to do so.

The below spec:

describe('adapter mocha', function () {
  it('passes', () => {

  })

  it('fails', () => {
    throw new Error('foo')
  })

  it.skip('pending', () => {

  })

  describe('bar', () => {
    it('works', () => {})

    afterEach(() => {
      expect(42).to.eq(43)
    })
  })
})

with the (incomplete) adapter implementation:

    runner.on('pending', function (test) {
      reportTestResult(tc, test)
    })

    runner.on('fail', function (test, error) {
      reportTestResult(tc, test)
    })

    runner.on('pass', function (test) {
      reportTestResult(tc, test)
    })

results in the below output

11 06 2021 07:52:13.892:INFO [Chrome 91.0.4472 (Mac OS X 10.15.7)]: Connected on socket lAViZN5z0jzm0N-RAAAD with id 43833069
Chrome 91.0.4472 (Mac OS X 10.15.7) adapter mocha fails FAILED
Chrome 91.0.4472 (Mac OS X 10.15.7) adapter mocha bar "after each" hook for "works" FAILED
Chrome 91.0.4472 (Mac OS X 10.15.7): Executed 4 of 4 (2 FAILED) (skipped 1) (0.01 secs / 0.002 secs)

[jginsburgn]

@samouri, will you get a chance to rearrange the logic so that the adapter reports on pass/pending/fail events instead of test end? In Karma we want to make sure that karma.result gets called exactly once per spec, so the above approach should work, considering that indeed, pass/pending/fail gets called once per spec, although test end might be called more than once.

[samouri]

Would this be considered a breaking change / major version bump?

[jginsburgn]

I think it should at most be a minor bump as we are not introducing a backwards incompatible change.