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

Akhilesh-max · 2024-05-07T12:23:01Z

Overview

This PR fixes or fixes part of Acceptance Flake (Docker): The Watch A Video button does not open the right page! (Docker Setup) #20253 .
This PR does the following: This PR addresses an flake that was occurring due to the destination site’s URL being fetched before it had completely loaded.

Essential Checklist

Please follow the instructions for making a code change.

I have linked the issue that this PR fixes in the "Development" section of the sidebar.
I have checked the "Files Changed" tab and confirmed that the changes are what I want to make.
I have written tests for my code.
The PR title starts with "Fix #bugnum: " or "Fix part of #bugnum: ...", followed by a short, clear summary of the changes.
I have assigned the correct reviewers to this PR (or will leave a comment with the phrase "@{{reviewer_username}} PTAL" if I can't assign them directly).

Proof that changes are correct

PR Pointers

Never force push! If you do, your PR will be closed.
To reply to reviewers, follow these instructions: https://github.com/oppia/oppia/wiki/Make-a-pull-request#step-5-address-review-comments-until-all-reviewers-approve
Some e2e tests are flaky, and can fail for reasons unrelated to your PR. We are working on fixing this, but in the meantime, if you need to restart the tests, please check the "If your build fails" wiki page.
See the Code Owner's wiki page for what code owners will expect.

oppiabot · 2024-05-07T12:23:12Z

Hi @Akhilesh-max please assign the required reviewer(s) for this PR. Thanks!

Akhilesh-max · 2024-05-07T12:23:35Z

@StephenYu2018 @seanlip PTAL

oppiabot · 2024-05-07T12:26:40Z

Unassigning @Akhilesh-max since a re-review was requested. @Akhilesh-max, please make sure you have addressed all review comments. Thanks!

Akhilesh-max · 2024-05-07T12:32:14Z

core/tests/puppeteer-acceptance-tests/user-utilities/logged-in-users-utils.ts

      this.clickOn(watchAVideoButton),
    ]);

+    await this.waitForText('Create New Account');
    const url = this.getCurrentUrlWithoutParameters();


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.

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

StephenYu2018

One bigger concern.

StephenYu2018 · 2024-05-07T12:46:15Z

core/tests/puppeteer-acceptance-tests/user-utilities/logged-in-users-utils.ts

      this.clickOn(watchAVideoButton),
    ]);

+    await this.waitForText('Create New Account');


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

@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.

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

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?

oppiabot · 2024-05-07T12:49:48Z

Unassigning @StephenYu2018 since the review is done.

oppiabot · 2024-05-07T12:49:51Z

Hi @Akhilesh-max, it looks like some changes were requested on this pull request by @StephenYu2018. PTAL. Thanks!

imchristie

Left some comments.

imchristie · 2024-05-08T04:42:04Z

core/tests/puppeteer-acceptance-tests/puppeteer-testing-utilities/puppeteer-utils.ts

@@ -410,6 +410,30 @@ export class BaseUser {
  getCurrentUrlWithoutParameters(): string {
    return this.page.url().split('?')[0];
  }
+
+  /**
+   * This function Waits for a specific text to appear in the body of the page.


"This function waits"...

imchristie · 2024-05-08T04:46:09Z

core/tests/puppeteer-acceptance-tests/user-utilities/logged-in-users-utils.ts

      this.clickOn(watchAVideoButton),
    ]);

+    await this.waitForText('Create New Account');


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

imchristie · 2024-05-08T04:46:52Z

core/tests/puppeteer-acceptance-tests/puppeteer-testing-utilities/puppeteer-utils.ts

+      );
+    } catch (error) {
+      error.message = `Failed to find text "${text}" within ${timeout} ms.`;
+      throw error;


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

imchristie · 2024-05-08T04:49:29Z

core/tests/puppeteer-acceptance-tests/user-utilities/logged-in-users-utils.ts

@@ -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']}),


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

seanlip · 2024-05-08T14:50:42Z

core/tests/puppeteer-acceptance-tests/puppeteer-testing-utilities/puppeteer-utils.ts

+  /**
+   * This function Waits for a specific text to appear in the body of the page.
+   * 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.


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?

seanlip · 2024-05-08T14:51:08Z

core/tests/puppeteer-acceptance-tests/puppeteer-testing-utilities/puppeteer-utils.ts

+   * @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> {


waitForTextToAppearInPageBody

Change timeout to timeoutMsec -- always include units.

seanlip · 2024-05-08T14:52:20Z

core/tests/puppeteer-acceptance-tests/puppeteer-testing-utilities/puppeteer-utils.ts

@@ -410,6 +410,30 @@ export class BaseUser {
  getCurrentUrlWithoutParameters(): string {
    return this.page.url().split('?')[0];
  }
+
+  /**


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.

seanlip · 2024-05-08T14:54:41Z

core/tests/puppeteer-acceptance-tests/user-utilities/logged-in-users-utils.ts

      this.clickOn(watchAVideoButton),
    ]);

+    await this.waitForText('Create New Account');


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?

seanlip · 2024-05-08T14:55:24Z

core/tests/puppeteer-acceptance-tests/user-utilities/logged-in-users-utils.ts

      this.clickOn(watchAVideoButton),
    ]);

+    await this.waitForText('Create New Account');
    const url = this.getCurrentUrlWithoutParameters();


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

oppiabot · 2024-05-15T15:02:26Z

Hi @Akhilesh-max, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 4 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 4 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

updates

1a3c20f

Akhilesh-max requested a review from a team as a code owner May 7, 2024 12:23

Akhilesh-max requested a review from StephenYu2018 May 7, 2024 12:23

Akhilesh-max requested review from imchristie and removed request for StephenYu2018 May 7, 2024 12:23

oppiabot bot assigned Akhilesh-max May 7, 2024

oppiabot bot assigned StephenYu2018 and unassigned Akhilesh-max May 7, 2024

Akhilesh-max requested a review from StephenYu2018 May 7, 2024 12:29

Akhilesh-max commented May 7, 2024

View reviewed changes

updates

a90c72c

StephenYu2018 requested changes May 7, 2024

View reviewed changes

oppiabot bot unassigned StephenYu2018 May 7, 2024

oppiabot bot assigned Akhilesh-max May 7, 2024

Akhilesh-max added 2 commits May 8, 2024 03:12

updates

52ac228

Merge branch 'develop' of https://github.com/oppia/oppia into flake-2

2f0e9fe

imchristie suggested changes May 8, 2024

View reviewed changes

seanlip requested changes May 8, 2024

View reviewed changes

oppiabot bot added the stale label May 15, 2024

oppiabot bot closed this May 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Akhilesh-max commented May 7, 2024

oppiabot bot commented May 7, 2024

Akhilesh-max commented May 7, 2024 •

edited

oppiabot bot commented May 7, 2024

Akhilesh-max May 7, 2024

seanlip May 8, 2024

StephenYu2018 left a comment

StephenYu2018 May 7, 2024

Akhilesh-max May 7, 2024 •

edited

imchristie May 8, 2024

seanlip May 8, 2024

oppiabot bot commented May 7, 2024

oppiabot bot commented May 7, 2024

imchristie left a comment

imchristie May 8, 2024

imchristie May 8, 2024

imchristie May 8, 2024

imchristie May 8, 2024

seanlip May 8, 2024

seanlip May 8, 2024

seanlip May 8, 2024

seanlip May 8, 2024

seanlip May 8, 2024

oppiabot bot commented May 15, 2024

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

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

Conversation

Akhilesh-max commented May 7, 2024

Overview

Essential Checklist

Proof that changes are correct

PR Pointers

oppiabot bot commented May 7, 2024

Akhilesh-max commented May 7, 2024 • edited

oppiabot bot commented May 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenYu2018 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akhilesh-max May 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented May 7, 2024

oppiabot bot commented May 7, 2024

imchristie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented May 15, 2024

Akhilesh-max commented May 7, 2024 •

edited

Akhilesh-max May 7, 2024 •

edited