Wait for promises explicitly, rather than putting them on the control flow

[sjelin]

Right now if an it() block returns a promise, it gets put directly on the control flow, which causes problems if it isn't a WebDriver promise. Instead, we should explicitly use the promise's .then() block. This will also help with the transition to supporting SELENIUM_PROMISE_MANAGER=0

[thorn0]

I think we should wait not only for the promise to get resolved, but also for all the control flow task queues to finish their execution.

[sjelin]

That will happen automatically because the it() blocks are run within flow.execute

[sjelin]

To be clear, this won't solve all issues with async/await. For instance:

browser.wait(() => {
  return (await elem.getText()) == 'go go go';
});

still has exactly the same problem. So I'm going to leave in the advice about leaving async/await out of your tests.

[thorn0]

That will happen automatically because the it() blocks are run within flow.execute

I'm not that sure after @jleyba 's explanations. The queues are independent.

[thorn0]

ControlFlow.html#execute:

Returns: A promise that will be resolved with the task result.

So it doesn't wait for the rest of the scheduled tasks.

[sjelin]

We don't use the promise returned by the control flow to synchronize, we use the flow itself. flow.execute won't start running a new block until the current block, and all its children, have finished. If it started running future tasks without waiting for previous ones then the flow wouldn't do much

[thorn0]

What about this example?

'use strict';
const {promise} = require('selenium-webdriver');
const flow = promise.controlFlow();

flow.execute(() => {
  console.log('a');
  return promise.delayed(10);
});
flow.execute(() => console.log('b'));
flow.execute(() => console.log('c'));

setTimeout(() => {
  // This is a new turn of the event loop, so tasks are scheduled in
  // a separate queue
  flow.execute(() => console.log('d'));
  flow.execute(() => console.log('e'));
}, 0);
// a
// d
// e
// b
// c

Inside the setTimeout callback, the flow.execute calls don't wait for the other queue. Why would flow.execute wait for other queues in our case?

[sjelin]

If you did:

it('', (done) => {
  flow.execute(() => {
    check1()
    return promise.delayed(10);
  });
  flow.execute(() => check2);
  flow.execute(() => check3);

  setTimeout(() => {
    // This is a new turn of the event loop, so tasks are scheduled in
    // a separate queue
    flow.execute(() => check4);
    flow.execute(() => check5);
    done();
  }, 0);
})

Then everything will work fine. You'll still have check2 and check3 run after check4 and check5, but jasminewd will wait until done() is called and the control flow is empty before running the next task. If you did:

it('', () => { // No callback or returned promise
  setTimeout(() => {
    flow.execute(() => {
      ...
    });
  }, 10);
})

You could have problems. But there's nothing we can really do about this case

[thorn0]

But there's nothing we can really do about this case

We can. We should subscribe to the idle event of the flow object.

flow.once('idle', ...);

[thorn0]

I see your point that such tests are clearly a bad idea, but the idea of the current issue (as I see it) is to isolate bad tests as best as possible. Why not use the idle event for this? If we see that there still are scheduled tasks in the flow, why would we allow the next test to start running?

[sjelin]

idle will not solve this problem. Let's consider this code:

it('test A', () => {
  setTimeout(() => {
    flow.execute(() => {
      ...
    });
  }, 10);
})
it('test B', () => {
  setTimeout(() => {
    ...
  }, 5);
})

And this timeline:

0ms  - 'test A` gets put on the control flow
1ms  - 'test A' gets run by the control flow
2ms  - 'test A' schedules its setTimeout
3ms  - 'test A' returns
4ms  - 'test A' is removed from the control flow
5ms  - The control flow emits 'IDLE'
6ms  - jasminewd receives 'IDLE'
7ms  - 'test B' gets put on the control flow
8ms  - 'test B' gets run by the control flow
9ms  - 'test B' schedules its timeout event
10ms - 'test B' returns
11ms - 'test B' is removed from the control flow
12ms - the setTimeout from 'test A' fires
13ms - The rest of 'test A' runs, but inside the context of 'test B'
14ms - The rest of 'test B' runs

[thorn0]

Of course, it won't help in such a case. It won't give us a 100% protection. But it's still better than nothing.

What I have in mind is code like my initial example from SeleniumHQ/selenium#3037:

const savedAllScriptsTimeout = browser.allScriptsTimeout;
browser.allScriptsTimeout = 25000;
await browser.get('web2.aspx/DB/METAGANTT/ALL');
browser.allScriptsTimeout = savedAllScriptsTimeout;
$('.foo').click();
$('.bar').click();
// ...

Basically, it's a mix of the async and sync styles. And idle will protect other tests from being affected by such a test.

[sjelin]

Your case will be handled by my change already. Async will cause the it() block to return a promise, and we'll wait for that promise before calling the callback for flow.execute

[thorn0]

The thing is that this returned promise won't wait for the tasks scheduled by $('.foo').click(); and $('.bar').click();.

[sjelin]

Hmm, I'm not sure. The code:

it('', async function() {
  const savedAllScriptsTimeout = browser.allScriptsTimeout;
  browser.allScriptsTimeout = 25000;
  await browser.get('web2.aspx/DB/METAGANTT/ALL');
  browser.allScriptsTimeout = savedAllScriptsTimeout;
  $('.foo').click();
  $('.bar').click();
});

Compiles to:

const savedAllScriptsTimeout = browser.allScriptsTimeout;
browser.allScriptsTimeout = 25000;
it('', () => {
  const savedAllScriptsTimeout = browser.allScriptsTimeout;
  browser.allScriptsTimeout = 25000;
  return browser.get('web2.aspx/DB/METAGANTT/ALL').then(() => {
    browser.allScriptsTimeout = savedAllScriptsTimeout;
    $('.foo').click();
    $('.bar').click();
  });
});

jasminewd will wait for the returned promise to resolve. By the time this happens, the click events will already be on the control flow. So I'm pretty sure the next test will be blocked until they are done. But I guess they could be scheduled in the wrong place on the control flow. I'll check

[thorn0]

await isn't compiled like you wrote. It's compiled to native promises, whose then is like setTimeout from the point of view of ControlFlow.

[sjelin]

I know 😄

[sjelin]

You're right 😞

[sjelin]

@thorn0 The PR has been updated to wait on the idle event