Skip to content
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

quick check of current URL to skip timepicker #146462

Merged
merged 20 commits into from
Dec 4, 2022
Merged
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion test/functional/page_objects/time_picker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,21 @@ export class TimePickerPageObject extends FtrService {
/**
* @param {String} fromTime MMM D, YYYY @ HH:mm:ss.SSS
* @param {String} toTime MMM D, YYYY @ HH:mm:ss.SSS
* @param {Boolean} force time picker force update, default is false
*/
public async setAbsoluteRange(fromTime: string, toTime: string) {
public async setAbsoluteRange(fromTime: string, toTime: string, force = false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under what circumstances does it make sense to force?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Off the top of my head, anytime flakiness is encountered (timepicker can be notorious sometimes).
This pr is a pr from a former elastic employee (our boss) and we did discuss at some length.
Further, there is some use of force flag within this pr.

@dmlemeshko may have more to weigh in on this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewctate , it is a good question. For some reason Lee picked area chart, maybe he was doing some testing.
You might know more about this test, and if there is no good reason re-resetting the time picker with already the same range, I can push an update and remove it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmlemeshko I agree with Andrew, this test was not flaky. I might miss something here but I would propose to remove it!

if (!force) {
const currentUrl = decodeURI(await this.browser.getCurrentUrl());
const DEFAULT_DATE_FORMAT = 'MMM D, YYYY @ HH:mm:ss.SSS';
const startMoment = moment.utc(fromTime, DEFAULT_DATE_FORMAT).toISOString();
const endMoment = moment.utc(toTime, DEFAULT_DATE_FORMAT).toISOString();
if (currentUrl.includes(`time:(from:'${startMoment}',to:'${endMoment}'`)) {
this.log.debug(
`We already have the desired start (${fromTime}) and end (${toTime}) in the URL, returning from setAbsoluteRange`
);
return;
}
}
this.log.debug(`Setting absolute range to ${fromTime} to ${toTime}`);
await this.showStartEndTimes();
let panel!: WebElementWrapper;
Expand Down