-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[services/testSubjects] fix isDisplayed/isEnabled/isSelected, add waitForEnabled #66538
[services/testSubjects] fix isDisplayed/isEnabled/isSelected, add waitForEnabled #66538
Conversation
log.debug(`TestSubjects.append(${selector}, ${text})`); | ||
const input = await this.find(selector); | ||
await input.click(); | ||
await input.type(text); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we don't clear input after click, retrying it may end up with the incorrect text
log.debug(`TestSubjects.doubleClick(${selector})`); | ||
const element = await this.find(selector, timeout); | ||
await element.moveMouseTo(); | ||
await element.doubleClick(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WebElementWrapper has retryCall
with 3 attempts to finish the action:
public async doubleClick() {
await this.retryCall(async function clickMouseButton(wrapper) {
await wrapper.scrollIntoViewIfNecessary();
await wrapper
.getActions()
.doubleClick(wrapper._webElement)
.perform();
});
}
I don't think we need to use retry service here
}); | ||
log.debug(`TestSubjects.isEnabled(${selector})`); | ||
const element = await this.find(selector); | ||
return await element.isEnabled(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same reason as with doubleClick
}); | ||
log.debug(`TestSubjects.isDisplayed(${selector})`); | ||
const element = await this.find(selector); | ||
return await element.isDisplayed(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same reason as with doubleClick
}); | ||
log.debug(`TestSubjects.isSelected(${selector})`); | ||
const element = await this.find(selector); | ||
return await element.isSelected(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same reason as with doubleClick
}); | ||
log.debug(`TestSubjects.getVisibleText(${selector})`); | ||
const element = await this.find(selector); | ||
return await element.getVisibleText(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same reason as with doubleClick
@elasticmachine merge upstream |
@elasticmachine merge upstream |
…usage-in-testSubjects
…mlemeshko/kibana into ftr/remove-retry-usage-in-testSubjects
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - I realized when I used isEnabled that I really wanted waitForEnabled. Code reviewed and Jenkins tested.
…ic#66538) Co-authored-by: Elastic Machine <[email protected]>
…ic#66538) Co-authored-by: Elastic Machine <[email protected]>
… (#66941) Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
… (#66942) Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
* master: (24 commits) [APM] agent config 'profiling_inferred_spans_min_duration' default value is '0ms' but the min value in the field is '1ms' (elastic#66886) [Canvas] Fix flaky custom element functional tests (elastic#65908) Fix IE specific flexbox min-height issue (elastic#66555) [Discover] Unskip doc link functional test (elastic#66884) Index pattern management to Kibana platform (elastic#65026) Warning and link to support matrix for IE11 (elastic#66512) [Reporting] Consolidate Server Type Defs, move some out of Legacy (elastic#66144) [SIEM] [Maps] Fixes Network Map empty tooltip (elastic#66828) [Endpoint] Encode the index of the alert in the id response (elastic#66919) [services/testSubjects] reduce retry usage, add waitForEnabled (elastic#66538) [DOCS] Identifies cloud settings for APM (elastic#66935) [SIEM][CASE] Fix configuration's page user experience (elastic#66029) Resolver: Display node 75% view submenus (elastic#64121) [SIEM] Cases] Capture timeline click and open timeline in case view (elastic#66327) [APM] Lowercase agent names so icons work (elastic#66824) [dev/cli] add support for --no-cache (elastic#66837) [Ingest Manager] Better handling of package installation problems (elastic#66541) [ML] Enhances api docs for modules endpoints (elastic#66738) dont hide errors (elastic#66764) [RFC] Global search API (elastic#64284) ...
Summary
While reviewing #66322 I noticed that
testSubjects.isDisplayed()
(.isEnabled/isSelected as well) always returnstrue
since it is wrapped with retry service.It sounds like a bug or bad naming at least, so I went through
testSubjects
service and removed retry usage where it may introduce flakiness or return an incorrect result,waitForEnabled
was added to retry until element displayed+enabledhttps://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/455/ 40x passed
Checklist
Delete any items that are not applicable to this PR.
For maintainers