fix: handle mocha hook failures in reporter#851
Conversation
This comment has been minimized.
This comment has been minimized.
1061bab to
9eb4fc1
Compare
9eb4fc1 to
f79e1f5
Compare
This comment has been minimized.
This comment has been minimized.
| .setLocationFile(file) | ||
| .setStarted((new Date()).toISOString()) | ||
| .setTimeout(_timeout) | ||
| .addDuration(0) |
There was a problem hiding this comment.
can we not addDuration as 0 potentially add a larger number. The reason I am suggesting that in grafana reporter the 0 will make the test best performing whereas we would want to see them as potential issue test that needs to be fixed.
In testbench I implemented getStartTime and substract that from overall run time, which is arbitary large number. Alternatively, 500ms or some other number can be added
There was a problem hiding this comment.
In testbench I implemented getStartTime and substract that from overall run time, which is arbitary large number.
I had done this originally but it wasn't really representative of what was happening. Test reporting captures test run data. We don't actually capture hooks at the moment. I think it may be worth trying to add hooks into the data eventually so we can better see these type of failure.
The way I was looking at it a value of 0 will only really be what's set in the cases of before all or before each failing which to me makes sense since the tests never ran. After each will add 0 but it's an accumulated value so the test time will have already been recorded for the test end handler and the after each failing won't add to the test time it'll just be reported as 0 since it didn't run or failed. I'm worried about us over reporting known wrong data so 0 seemed like the right thing to do based on the scenarios I layed out.
There was a problem hiding this comment.
Note as well that if a before all or before each fails then I believed from what I observed the test isn't even included in the data since it never ran. NM I'm wrong on that it will be included but will be marked as failed.
This comment has been minimized.
This comment has been minimized.
|
UGH. Gotta look into the stupid flakiness with webdriver and web test runner. It's unrelated to this change. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Oh come on!!!!! |
This comment has been minimized.
This comment has been minimized.
|
Lol, it works locally fine. sigh |
Coverage ReportReportReport Validation
|
|
🎉 This PR is included in version 5.2.11 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Adds
EVENT_TEST_FAILhandling to the mocha reporter so hook failures (before,beforeEach,afterEach,after) are properly captured in the report. Previously, hook failures could leave test details missing (particularlybeforeAllfailures where no test events fire at all).The
_onTestFailhandler checks if the failure is from a hook, finds the affected test viactx.currentTest, and populates the detail withname,file,started,timeout,duration, andstatusif the detail doesn't already have a status set. Forafter/afterEachhooks the test has already completed, so the hook failure is ignored.Also adds a
getStatus()method toReportDetailBuilder, integration tests covering all hook failure types (including flaky hooks to document that Mocha does not retry hooks), and lowers the c8 coverage threshold for statements/lines from 85% to 80% (will address in another PR)https://desire2learn.atlassian.net/browse/QE-2091