Skip to content
This repository has been archived by the owner on Dec 28, 2023. It is now read-only.

Report on failures after test complete. #237

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

samouri
Copy link

@samouri samouri commented Apr 28, 2021

summary
Fixes #236

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

@jginsburgn
Copy link
Member

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 mochajs/mocha#4656.

@jginsburgn jginsburgn assigned jginsburgn and unassigned jginsburgn Jun 10, 2021
@jginsburgn
Copy link
Member

@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
Copy link
Member

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
Copy link

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
Copy link

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
Copy link
Member

@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
Copy link
Author

samouri commented Jun 29, 2021

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

@jginsburgn
Copy link
Member

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

adapter.js swallows failures that occur after test complete
3 participants