Conversation
f430469 to
df433dc
Compare
df433dc to
5aade92
Compare
|
jenkins, test this |
* remove inline awaits * testSubjects.find now always returns a promise, so we need to make sure there is no more inlining * more find conversions * need to pass property name * Make sure async functions are awaited on.
| }); | ||
| it('should have index pattern in page header', async function () { | ||
| const indexPageHeading = await PageObjects.settings.getIndexPageHeading(); | ||
| const patternName = await indexPageHeading.getVisibleText(); |
There was a problem hiding this comment.
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();There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Cool, thanks for the education! 👯♂️
| await this.clickTimeFieldNameField(); | ||
| await PageObjects.header.waitUntilLoadingHasFinished(); | ||
| await retry.try(async () => { | ||
| return await retry.try(async () => { |
There was a problem hiding this comment.
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.
| byCssSelector(selector) { | ||
| async byCssSelector(selector, timeout = defaultFindTimeout) { | ||
| log.debug(`findByCssSelector ${selector}`); | ||
| return remote |
There was a problem hiding this comment.
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);
})
}
}| async existsByCssSelector(selector) { | ||
| log.debug(`existsByCssSelector ${selector}`); | ||
| const remoteWithTimeout = remote.setFindTimeout(1000); | ||
| const exists = await remoteWithTimeout.findByCssSelector(selector) |
There was a problem hiding this comment.
Not a fan of mixing async/await with .then().catch(). A try/catch would probably pair nicely with this.withTimeout()
There was a problem hiding this comment.
So something like:
let exists = true;
try {
await remoteWithTimeout.findByCssSelector(selector);
} catch () {
exists = false;
}
remoteWithTimeout.setFindTimeout(defaultFindTimeout);
return exists;
?
There was a problem hiding this comment.
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;
}
});
}There was a problem hiding this comment.
ahhh, wasn't aware of that function, sg!
There was a problem hiding this comment.
oh doh, that's the function you are saying to use above. haha, yes, sounds good!
* remove inline awaits * testSubjects.find now always returns a promise, so we need to make sure there is no more inlining * more find conversions * need to pass property name * Make sure async functions are awaited on.
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).