feat: support parallel in before/afterAll hooks

[tim-yao]

🤔 What's changed?

⚡️ What's your motivation?

Fix #1153

🏷️ What kind of change is this?

• ⚡ New feature (non-breaking change which adds new behaviour)

♻️ Anything particular you want feedback on?

📋 Checklist:

• I agree to respect and uphold the Cucumber Community Code of Conduct
• I've changed the behaviour of the code
  • I have added/updated tests to cover my changes.
• My change requires a change to the documentation.
  • I have updated the documentation accordingly.
• Users should know about my change
  • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

---

This text was originally generated from a template, then edited by hand. You can modify the template here.

[mattwynne]

@davidjgoss I noticed that the errors from the workers' AfterAll hooks surfaced in quite an unexpected way. The Cucumber run itself shows 2 scenarios passed, but the overall run has exited with a non-zero code. If you scroll back up the output, you can see the errors reported from the Workers.

I recall we've discussed this elsewhere, that those worker errors should be printed in the main Cucumber console output. Should we maybe deal with that first?

[davidjgoss]

@mattwynne

I recall we've discussed this elsewhere, that those worker errors should be printed in the main Cucumber console output. Should we maybe deal with that first?

I think that's a different and harder problem than the scope of this PR. It gets back to cucumber/messages#66 - we want to factor global hook failures into the formatter output so that it doesn't hide the failures, but we can't because formatters rely on the supply of messages, and global hooks are not represented in any messages. I'll comment on that issue and see if it goes anywhere.

@tim-yao sorry for the delay - I'll get you some feedback on this shortly.

[tim-yao]

Hi @davidjgoss any plan for this?

[davidjgoss]

A dropped a couple of comments inline which might be helpful for implementing this, plus here's a couple of links to relevant code:

• https://github.com/cucumber/cucumber-js/blob/main/src/support_code_library_builder/index.ts#L276-L297 handles where the global hooks are defined - see also the IDefineTestRunHookOptions and ITestRunHookDefinitionConfig types which will need adding to
• https://github.com/cucumber/cucumber-js/blob/main/src/runtime/parallel/worker.ts#L86-L95 where hooks are currently invoked at the worker level

If you need any pointers give me a nudge!

[davidjgoss]

I'm not sure this will work once we implement the changes - because the workers are in different Node.js processes, they won't be acting on the same counter variable, so I think it will always be 1 assuming the workers take one scenario each.

What might be worth pursuing instead is having each flavour of the hook console.log a string with the some of environment variables that Cucumber sets for the parallel runtime e.g CUCUMBER_WORKER_ID and then assert on that being in the output with a Then the output contains the text: step (see other features) - a bit messy but should work.

[davidjgoss]

This could be a TS enum

[luke-hill]

I've put a comment on the original discussion thread @tim-yao - essentially suggesting we won't fix this.

If you want to continue and go ahead that's fine. But in essence this would be something JS-specific. It's not something I would evangelise porting to other languages, and the very nature of JS' async behaviour maybe lends itself to something that could be used here.

However in practicality (I won't copy my post from the issue), I just think this over-engineers a problem that could (and should), be solved in a different way. I'll let @davidjgoss maybe provide more JS-specific information as he's the subject expert in cucumberJS.

[tim-yao]

I've put a comment on the original discussion thread @tim-yao - essentially suggesting we won't fix this.

If you want to continue and go ahead that's fine. But in essence this would be something JS-specific. It's not something I would evangelise porting to other languages, and the very nature of JS' async behaviour maybe lends itself to something that could be used here.

However in practicality (I won't copy my post from the issue), I just think this over-engineers a problem that could (and should), be solved in a different way. I'll let @davidjgoss maybe provide more JS-specific information as he's the subject expert in cucumberJS.

That makes sense to me. I don't have the original issue anymore. It was from the HTML report plugin which requires after all hooks. It was broken in parallel. Since I move to use the default HTML formatter, I don't have the issue anymore. I will leave this PR to @davidjgoss. Feel free to close this or continue. Thanks.