Hooks should be executed in order and in sequence in shake of determinism

[alfonso-presa]

Environment (please complete the following information):

• WebdriverIO version:: v5 and v6
• Mode: testrunner
• If WDIO Testrunner, running sync/async: both
• Node.js version: any
• NPM version: any
• Browser name and version: any
• Platform name and version: any

Config of WebdriverIO
N/A

Describe the bug

Hooks are executed all in parallel leading for example to unexpected errors. Also the Promise.all, as it's fail fast, leads to the posiblity of hooks mixing ones with the others in case of a failure (for example if an afterTest fails, the other afters might keep executing.

Code for reference:

webdriverio/packages/wdio-utils/src/shim.js

Line 24 in 1f639d2

hooks = hooks.map((hook) => new Promise((resolve) => {

Expected behavior

Hooks should be executed in order and in sequence as declared in the services or the config. I propose changing the map for a reduce. Note it might have performance implications tough. I can hapilly PR if changing to reduce is ok for you.

[christian-bromann]

Note it might have performance implications tough

Where do you see the performance implications?

[alfonso-presa]

Well.... before everything was being executed in parallel, so there might be some people performing tasks (data provisioning, mock service initialization, etc) that might take time. Now those tasks will be executed sequentially, with the corresponding time loss....

[mgrybyk]

I had also idea one day in the past to improve it somehow and even set order of execution: built-in hooks first, then service hooks, then user hooks. I also had issues because of that and because of all the hooks run in parallel

[christian-bromann]

I also had issues because of that

Can you explain what kind of issues you had? I think no services should interfere with each other in any way which is way they have been executed in parallel in the first place.

[alfonso-presa]

En example of issue found because of this is performing reloadSession in a hook, the sauce:context executed by the sauce-service to set the scenario name is executed before the reloadSession.

I've also found issues trying to get the sessionId into the reporter because i had two different services for reloading and reporting... I ended up making only one service for both things, which IMHO lead to bad design....

[christian-bromann]

the sauce:context executed by the sauce-service to set the scenario name is executed before the reloadSession.

That doesn't necessarily sound like a problem. There should be no race condition here. Hook nor sauce:context call run at the same time that could create a indeterminism behavior.

i had two different services for reloading and reporting... I ended up making only one service for both things

I am not sure if a service should be used for reporting.

[alfonso-presa]

@christian-bromann

There should be no race condition here. Hook nor sauce:context call run at the same time that could create a indeterminism behavior.

The problem is that the sauce:context is executed against the old session and then after that session gets reloaded, so the new session lacks the category annotation. Check first execution and second execution

I am not sure if a service should be used for reporting.

Sorry, I explained it wrongly. We use a hook to gather evidences in case or error (sources, screenshots, browser logs, etc) and add them to the allure report.

[christian-bromann]

Check first execution and second execution

I think it depends where you do the reload. I will have to check what's the best way here.

Sorry, I explained it wrongly. We use a hook to gather evidences in case or error (sources, screenshots, browser logs, etc) and add them to the allure report.

So there is one service to gather information and one that reports them to allure? I am not sure why it would be bad to have them in one service. One could argue that having services that need to be configured in a certain order is confusing and bad design. Note that you can run multiple things from different locations in one hook, e.g.:

  // ...
  async afterTest () {
     await captureData()
     await reportData()
  }
  // ...

[alfonso-presa]

I think it depends where you do the reload. I will have to check what's the best way here.

We are doing it in the beforeScenario with a flag to check if the scenario is the first one or not to avoid trash sessions. I was thinking of doing it in the after only if the scenario is not the last... but that's a lot more tricky to find out.

So there is one service to gather information and one that reports them to allure?

There was one that reloads the sessions, and other that gathers evidences. The one gathering evidences adds the session id to the reports at the session start, and it was having race conditions with the one reloading, logging sometimes the wrong session id (the one from the previous test).

Of course this hooks can be redefined, and we could at the end find strategies to overcome this issue, but I think it would be useful to control the hook execution order to ease this things and make the system more predictable. The big problem here is that it might work almost always, so it leads to hard to find, possible source of flakiness, issues that I personally would not like to see in tests.