wdio-allure-reporter: Bug fix where tests get unknown status and has no execution information

[ccharnkij]

Proposed changes

This PR is for fixing #4953. It seems like the unknown status happened due to #4773, the added logic messed with the listener lifecycle (specially for "before each" and "after each"). I fixed it by only allowing "before all" and "after all" to be treated as their own test and be affected by the flag "disableMochaHooks". I made "before each" and "after each" unaffected by the flag along with preventing them from displaying in Allure as their own test. Since "before all" and "after all" run only once, it makes sense for them to appear in Allure as their own test. However "before each" and "after each" run with every test, so it makes sense to only include them as part of a test. Let me know if this is ok.

Types of changes

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

Checklist

• I have read the CONTRIBUTING doc
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)
• I have added proper type definitions for new commands (if appropriate)

Further comments

Reviewers: @webdriverio/technical-committee

[jsf-clabot]

All committers have signed the CLA.

[erwinheitzman]

Thank you for the PR, I had already looked into this but didn't find the time to wrap it up yet :)

Added a small comment from my own findings

[erwinheitzman]

I have checked and before the behaviour was like this:

    if (isMochaEachHooks(hook.title)) {
      this.allure.startStep(hook.title)
      this.setCaseParameters(hook.cid);
      return
    }

I assume this behaviour is still needed (the setCaseParameters)

[ccharnkij]

I tried to look into commit history and couldn't find reference to it. For those lines in onHookStart, I only reverted in back to before #4773. If the setCaseParameters is needed, I can try adding it in and test again.

[ccharnkij]

@erwinheitzman I tried adding the setCaseParameters line in, and it didn't look like the hook.cid was used in the report for the mocha each hooks.

[ccharnkij]

@erwinheitzman Is there any other thing I can help with this PR? If it's the CI failed, it is because of the code converage percentage is slightly lower than required. But this happens prior to the PR and happens in the master as well.

[codecov]

Codecov Report

Merging #5040 (47d14b3) into master (c966686) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5040      +/-   ##
==========================================
- Coverage   99.41%   99.39%   -0.03%     
==========================================
  Files         210      210              
  Lines        5509     5622     +113     
  Branches     1180     1213      +33     
==========================================
+ Hits         5477     5588     +111     
- Misses         29       31       +2     
  Partials        3        3              

Impacted Files	Coverage Δ
packages/wdio-allure-reporter/src/index.js	99.14% <100.00%> (ø)
packages/wdio-cli/src/constants.js	80.00% <0.00%> (-20.00%)	⬇️
packages/wdio-runner/src/utils.js	100.00% <0.00%> (ø)
packages/wdio-sauce-service/src/launcher.js	100.00% <0.00%> (ø)
packages/wdio-appium-service/src/launcher.js	100.00% <0.00%> (ø)
packages/wdio-testingbot-service/src/launcher.js	100.00% <0.00%> (ø)
...kages/webdriverio/src/commands/element/addValue.js	100.00% <0.00%> (ø)
...kages/webdriverio/src/commands/element/setValue.js	100.00% <0.00%> (ø)
...ges/webdriverio/src/commands/browser/setCookies.js	100.00% <0.00%> (ø)
...es/webdriverio/src/commands/browser/touchAction.js	100.00% <0.00%> (ø)
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c966686...47d14b3. Read the comment docs.

[christian-bromann]

@sakhisheikh isn't this related to what you have been looking at?

[sakhisheikh]

@sakhisheikh isn't this related to what you have been looking at?

Yes it is.

[BorisOsipov]

It seems this PR is a just partial revert of PR #4773 and I am not sure it won't break anything else.
I'd like to help with this, but currently I don't have enough time.

I would suggest reverting #4773 at all and reopen #4767 to make better fix later.
Seems it is best solution now to fix reporter for most people until #4767 will be resolved.
I saw 10-20 questions about this bug in our gitter and local telegram communities - this bug affects a lot of people who use allure as main reporter.

What do you think @christian-bromann @sakhisheikh @erwinheitzman ?

[ccharnkij]

@BorisOsipov If a full revert of #4773 is done, then "passed" beforeAll/afterAll steps will not appear in the report, since #4773 provides a flag to trigger it on or off from #4406. With this PR, I have made it so that, by default all hooks would be captured. beforeAll/afterAll would have their own step, but beforeEach/afterEach will be under each test.

If disableMochaHooks is set to true, then the "passed" beforeAll/afterAll won't appear in the report, and only the "failed" beforeAll/afterAll will appear. beforeEach/afterEach is not affected by the flag and always appear in the report.

The only negative thing about this is that the "failed" beforeAll/afterAll, with disableMochaHooks = true, will not have the steps information in the report. This is because disableMochaHooks=true prevents the hook to start as a test in the onHookStart.

I have bandwidth to work on this, if this is not the desired behavior. I just need to know what the desired behavior is.

[BorisOsipov]

@ccharnkij thank you for your help with this.

I got confused with the logic when we create or not create hooks in reports depends on what is hook type and disableMocha options. I think it is bad idea to introduce disableMocha option. Reporter should be simple and consistent without magic options.

With this PR, I have made it so that, by default all hooks would be captured. beforeAll/afterAll would have their own step

Unfortunately, it is not good.
It made reporter inconsistent with other reports - allure and e.g. spec reporter will show different total count of test (in allure you add beforeAll as test - so total count of tests will be increased - that is not right)

I will go deeper into this bunch of issues\requirements and come back to you in a few days with some design doc how it should work.

[erwinheitzman]

@ccharnkij @BorisOsipov I have rolled back some changes locally and did many testing and it seems like the way the hooks are handled are broken to begin with and that the code is really frustrating to read (a lot of if/else with guessing work as to why something was done that way).

I think we should refactor this reporter in the future really to start with.

[christian-bromann]

it seems like the way the hooks are handled are broken to begin with

Can you provide more details?

[erwinheitzman]

Before the disableMochaHooks option was introduced the hooks did not work as they where never logged at all as far as I remember. Toggling the option on/off in this branch gives unintended behaviour from what I saw (on works but then off was not right I believe, would have to check again to make sure I guess).

[kfirbhCS]

guys is this on someones plan ?
for me its critical

[christian-bromann]

@kfirbhCS I am not sure what the state of this PR is. From what I can see based on the comments is that the current proposed solution isn't desired and @BorisOsipov wanted to provide some more context how the solution could look like. Are there any updates on this?

[BorisOsipov]

I stil don't have time to look into this. If anyone have time to fix it feel free to pick up this.

[snixer724]

I believe I am also hitting this issue. Using Mocha/WDIO/Allure and I have the 'disableMochaHooks' option set to false as I want the beforeEach/afterEach to be reported if something fails. However for a test that passes, the allure report shows the test:unknown, beforeEach:pass, afterEach:pass.

BeforeEach/AfterEach should be treated as part of the test like the MR creator stated. Could someone take a peek at this PR again?

To recap:

• I expect beforeEach/afterEach to still report something when they fail
• When using this option 'disableMochaHooks=false' I expect the test to show the correct status and not unknown
• beforeEach/afterEach should not be treated as their own tests, because they run as part of every test.

[mittalv3]

Any update on this ticket?

[oversizedhat]

I did some investigation on the issue as we are effected by this in our setup using latest wdio (6.7.0 version) using mocha. Basically it makes the Allure report useless for us.

There seems to be a number of issues, and a bit tricky to find the suitable place for fix. Here are the issues I have found, which at least seem to solve the issues we have.

In wdio-mocha-framework a "currentTest" attribute is added to the hook message. However this is not part of the wdio-reporter contract and thus it seems to be ignored even when set. Ie the following block in mocha-framework does not work:

/**
  * Add the current test title to the payload for cases where it helps to
  * identify the test, e.g. when running inside a beforeEach hook
  */
  if (params.payload.ctx && params.payload.ctx.currentTest) {
    message.currentTest = params.payload.ctx.currentTest.title
  }

But making it part of the contract wont help because I don't believe the value is being used. At least the way I see it onHookStart and onHookEnd in in wdio-allure-reporter only seem to read title, and as title can include "before" and "after" wrappings it starts to add a bunch of duplicated, incomplete, tests in the report. But if I change so I use currentTest in favor for title if its provided, then it looks like it solves the issues we are having if I make one more change, which is to ignore any onTestStarts if a test with the same title is already running in allure-reporter. Ie something like this:

onTestStart(test) {
  // Added
  const title = test.currentTest?test.currenTest:test.title;
  if (this.isAnyTestRunning()){
    if (this.allure.getCurrentTest().name == title){
      this.setCaseParameters(test.cid); 
      return;
    }
  }
  //End Added
  if (this.options.useCucumberStepReporter) {
     return this.allure.startStep(test.title)
  }
  this.allure.startCase(test.title)
  this.setCaseParameters(test.cid)
}

Perhaps not the most elegant of solutions but it feels like it should have low impact on surrounding things. If anyone more that is more knowledgeable could confirm or feedback on a better solution I could try adding it.

[christian-bromann]

@oversizedhat thanks for the investigation. I think it would be fine if we make choices in the allure reporter depending on the framework and the input it gets.

[oversizedhat]

@oversizedhat thanks for the investigation. I think it would be fine if we make choices in the allure reporter depending on the framework and the input it gets.

Thanks @christian-bromann for the response. Please elaborate. I do not understand what you mean.

[christian-bromann]

@ccharnkij I am just saying that we are very interested to fix this and happy for any change it requires.

[ccharnkij]

@ccharnkij I am just saying that we are very interested to fix this and happy for any change it requires.

is this for me? or for @oversizedhat ?

[christian-bromann]

for @oversizedhat , sorry

[christian-bromann]

I think this PR got stale. Issue #4953 is still open and we would love to see contributions to it.