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.

Sign up for GitHub

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

Jump to bottom

FIX #20253 : Acceptance Flake (Docker): The Watch A Video button does not open the right page! (Docker Setup) #20273

Closed
wants to merge 4 commits into from
Closed

FIX #20253 : Acceptance Flake (Docker): The Watch A Video button does not open the right page! (Docker Setup) #20273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1a3c20f
updates
Akhilesh-max May 7, 2024
a90c72c
updates
Akhilesh-max May 7, 2024
52ac228
updates
Akhilesh-max May 7, 2024
2f0e9fe
Merge branch 'develop' of https://github.com/oppia/oppia into flake-2
Akhilesh-max May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 24 additions & 0 deletions core/tests/puppeteer-acceptance-tests/puppeteer-testing-utilities/puppeteer-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,30 @@ export class BaseUser {
getCurrentUrlWithoutParameters(): string {
return this.page.url().split('?')[0];
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have this function -- why isn't "wait for text to be displayed" sufficient? This method would pick up hidden text in the body or div elements etc., which does not match with what the end user would see on the page.

* This function Waits for a specific text to appear in the body of the page.
Copy link
Member

Choose a reason for hiding this comment

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

"This function waits"...

* This function only waits for rendered text. It does not wait for text in iframes or text that is hidden or not rendered for some reason.
* The text matching is case-sensitive and space-sensitive, so it might not find the text if there are differences in case or white space.
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised these lines aren't throwing lint errors -- aren't they too long?

Why is prettier not running locally on your submitted code? Can you get this fixed in your development environment?

* @param {string} text - The text to wait for.
* @param {number} [timeout=30000] - The maximum time to wait in milliseconds. Defaults to 30000 (30 seconds).
*/
async waitForText(text: string, timeout: number = 30000): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

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

waitForTextToAppearInPageBody

Change timeout to timeoutMsec -- always include units.

try {
await this.page.waitForSelector('body');
await this.page.waitForFunction(
(textToFind: string) => {
const bodyText = document.querySelector('body')?.innerText || '';
return bodyText.includes(textToFind);
},
{ timeout },
text
);
} catch (error) {
error.message = `Failed to find text "${text}" within ${timeout} ms.`;
throw error;
Copy link
Member

Choose a reason for hiding this comment

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

Change it to:
throw new error('message');

}
}
}

export const BaseUserFactory = (): BaseUser => new BaseUser();
3 changes: 2 additions & 1 deletion core/tests/puppeteer-acceptance-tests/user-utilities/logged-in-users-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -674,10 +674,11 @@ export class LoggedInUser extends BaseUser {
throw new Error('The Watch A Video button does not exist!');
}
await Promise.all([
this.page.waitForNavigation(),
this.page.waitForNavigation({waitUntil: ['load', 'networkidle2']}),
Copy link
Member

Choose a reason for hiding this comment

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

If we already have this waitUntil, do we still need the new function waitForText in order to fix the flake?

this.clickOn(watchAVideoButton),
]);

await this.waitForText('Create New Account');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we waiting for a certain text to be displayed instead of finding the relevant selector and using waitForSelector on it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@StephenYu2018 Actually, the page we’re navigating to is a Facebook page. Here, we encounter selectors such as class="x1ey2m1c x9f619 xds687c x10l6tqk x17qophe x13vifvy x1ypdohk". I guess, these are automatically generated ones and may not be reliable.
Additionally, may be, It will work out without this function as well, but I feel there are cases wherein this function could be helpful.

Thank you.

Copy link
Member

Choose a reason for hiding this comment

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

Why do you use the text "Create New Account"?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, for cases like this, I think what I would suggest is to just verify that the URL of the new page is correct and that the navigation stabilizes.

The reason I suggest that is because we don't want our tests to break if Facebook (say) changes how they render their page, which is something that's out of our control. Given that, do you think it would be enough to wait for navigation to stabilize?

const url = this.getCurrentUrlWithoutParameters();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Occasionally, the URL was being fetched before the page had fully loaded, leading to differences with the expected URL. Therefore, made changes to ensure the page is fully loaded before the URL is fetched.

Copy link
Member

Choose a reason for hiding this comment

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

Can you explain what the URL is in the intermediate stage and what it is after the page settles down?

const expectedWatchAVideoUrl = this.isViewportAtMobileWidth()
? mobileWatchAVideoUrl
Expand Down