remove inline awaits

[stacey-gammon]

Addresses #13041 for tests.

Turns variations of the form:
await testSubjects.find('globalTimepickerRange').getVisibleText();
into
await testSubjects.getVisibleText('globalTimepickerRange');

or simply turns one line into two by breaking up the inlining of multiple functions with an await.

Having multiple functions on the same line is confusing and can potentially lead to mistakes, especially as with this change testSubjects.find returns a promise, not a webdriver object which can be used as a promise, or as the return value (which is pretty confusing).

[stacey-gammon]

jenkins, test this

[w33ble]

I'm not super well versed in async/await, but I thought it was basically just like wrapping the call in a then handler. If I'm right about that, couldn't this be written like so?

const patternName = await PageObjects.settings.getIndexPageHeading().getVisibleText();

[epixa]

Like return, await affects the entire expression, not just a function invocation. So await doesn't really "kick in" until it finds a newline, a semi-colon, the closing of a block, etc.

[stacey-gammon]

So we don't want to use multiple functions on the same line as an await because it's confusing (there was a little discussion in the kibana slack room and I filed #13041). Does the await apply to the first or second? Sans parens, it applies to the second. This causes extra confusion with tests if a function returns the webdriver API - it can be used as a promise, or chained together with the call that should go after the promise.

testSubjects.find changed so that it's not returning the webdriver API, it's returning a legit promise, which means we can't get away with this subtle behavior anymore. Now, the await has to be used for the first function, not the second. So you could write it like this:

const patternName = (await PageObjects.settings.getIndexPageHeading()).getVisibleText();

But to avoid all this confusion, we just decided to not use parens and not use multiple functions on the same line when an await is required.

[w33ble]

Cool, thanks for the education! 👯‍♂️

[spalger]

No reason to return here

[stacey-gammon]

So I could have sworn you gave me feedback once that said I should add a return for an async function, e.g. like I'm doing above:

    async clickTimeFieldNameField() {
       return await testSubjects.click('createIndexPatternTimeFieldSelect');
    }

The return isn't necessary then either, right? Perhaps I misunderstood your original comment from awhile back.

[spalger]

I think a helper method would be useful here. Maybe something like:

class Find {
  async withTimeout(timeout, block) {
    try {
      const remoteWithTimeout = remote.setFindTimeout(timeout);
      return await block(remoteWithTimeout);
    } finally {
      remote.setFindTimeout(defaultFindTimeout);
    }
  }
}

Then these methods functions could be more focused on their specific details:

class Find {
  async byCssSelector(selector, timeout = defaultFindTimeout) {
    return await this.withTimeout(timeout, async remote => {
      return await remote.findByCssSelector(selector);
    })
  }
}

[spalger]

Not a fan of mixing async/await with .then().catch(). A try/catch would probably pair nicely with this.withTimeout()

[stacey-gammon]

So something like:

let exists = true;
try {
  await remoteWithTimeout.findByCssSelector(selector);
} catch () {
  exists =  false;
}
remoteWithTimeout.setFindTimeout(defaultFindTimeout);
return exists;

?

[spalger]

I'm thinking:

async existsByCssSelector(selector) {
  log.debug(`existsByCssSelector ${selector}`);
  return await this.withTimeout(1000, async remote => {
    try {
      await remote.findByCssSelector(selector)
      return true;
    } catch (error) {
      return false;
    }
  });
}

[stacey-gammon]

ahhh, wasn't aware of that function, sg!

[stacey-gammon]

oh doh, that's the function you are saying to use above. haha, yes, sounds good!