add option "displayFailedHookAsFailedTest" to not show passing hooks as tests and display failed hooks as failed tests in the correct teamcity format

fernyb commented 2 years ago

Proposed changes

add option "displayFailedHookAsFailedTest" to not show passing hooks as tests and display failed hooks as failed tests in the correct teamcity format.

Types of changes

What types of changes does your code introduce Put an x in the boxes that apply

[ ] Bugfix (non-breaking change which fixes an issue)
[ x ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

[ x ] Lint and unit tests pass locally with my changes
[ x ] I have added tests that prove my fix is effective or that my feature works
[ x ] I have added necessary documentation (if appropriate)

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

fernyb commented 2 years ago

I got an email saying this (https://teamcity.jetbrains.com/viewLog.html?buildId=3703204&buildTypeId=TeamCityThirdPartyPlugins_MochaTeamcityReporter_Build) failed but I'm not sure what that is for. All tests are passing.

jamie-sherriff commented 2 years ago

Thanks @fernyb for this.

Additional test failures would be expected coming from the validate reporter if you have added more tests that expose failures to be reported in TeamCity.

I will have a good play with these changes and ill update the expected failed test count in on the validate reporter configuration to match the new expected count assuming they are all expected failures.

jamie-sherriff commented 2 years ago

Hey @fernyb, have thought of some potentially breaking scenario for these changes, open to any suggestions.

Now that all test results are getting proceed at the end what if a test caused the mocha process to crash or timeout, no test results will get propagated right? Under the previous implementation at least previous tests would be propagated to Teamcity messages as they came through.

Received timestamps on Teamcity are not going to be accurate in relation to the service message, does this matter or should every message now have a timestamp property?

I would prefer to keep the approach of logging messages as soon as events come through, can you think of a way to implement the displayFailedHookAsFailedTest functionality without processing all messages at the end? Potentially a way to mitigate lost messages would be to process them in chunks per each hook?

Also if anyone else if requiring this change please comment so we can establish potential usage of it.

DJ-Glock commented 2 years ago

@fernyb tries to solve really annoying issue: We do not really want to see hooks in TC tests section if they are executed successfully. But we do want to see these hooks in TC tests section if they failed.

Some time ago I fixed an issue https://github.com/travisjeffery/mocha-teamcity-reporter/issues/35 by adding testStarted to every hook because without it TeamCity was not able to show failed hook as a failed test (with recordHookFailures=true). Actually I tried to fix this issue by simple code change. It was not the best fix, because it caused an issue with showing all hooks as tests (when recordHookFailures=true). So I added skipHooksWithTag in scope of https://github.com/travisjeffery/mocha-teamcity-reporter/issues/56.

I believe this feature is needed. But I am not sure what is the best way to implement it. I have a feeling that it should be a part of recordHookFailures. And behavior with logging hooks should be reworked. Probably ignoreHookWithName should be removed at all.

And depending on your decision, I would suggest that you should amend the following description (if new flag is going to be added):

Here we should add a notice that all hooks are treated as tests if flag is enabled.

jamie-sherriff commented 2 years ago

Thanks for the Info @DJ-Glock

An alternative right now could be to publish this PR as a pre-release version if there is some demand for this.

I still think a alternative way needs to be thought of to eliminate the risk of loosing log messages and timestamp inconstancies or as you suggested the new functionality here is controlled all by a feature flag and the old way of logging messages is retained when the feature is not enabled.

Baune8D commented 2 years ago

Hi. I am very interested in trying out this PR. We sometimes run into errors in hooks, and to have them show up in TeamCity would speed up troubleshooting.

travisjeffery / mocha-teamcity-reporter