Implement part of #17712 : Acceptance tests for Exploration Creator Section(CUJ 10). #20203

rahat2134 · 2024-04-23T16:54:01Z

Overview

This PR
fixes part of Acceptance Testing - covering all user journeys in form of user stories #17712 .
This PR does the following: Adds acceptance test for exploration editor section.
Points:
10- User can Publish the latest changes, User can draft the latest changes)

Essential Checklist

The PR title starts with "Fix #bugnum: " or "Fix part of #bugnum: ...", followed by a short, clear summary of the changes.
I have followed the instructions for making a code change.
I have assigned the correct reviewers to this PR (or will leave a comment with the phrase "@{{reviewer_username}} PTAL" if I don't have permissions to assign reviewers directly).
The linter/Karma presubmit checks pass on my local machine, and my PR follows the coding style guide).
"Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)

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-04-23T16:54:13Z

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

rahat2134 · 2024-04-23T17:19:47Z

core/tests/puppeteer-acceptance-tests/user-utilities/exploration-editor-utils.ts

+    if (oldTitle !== title) {
+      if (this.isViewportAtMobileWidth()) {
+        // Navbar text is hidden in mobile view port due to less screen so there is no visible
+        // change in UI after we update the input bar. Hence we need to explicitly wait for 2 sec.
+        await this.page.waitForTimeout(2000);
+      } else {
+        await this.page.waitForSelector('span.e2e-test-autosave-indicator', {
+          visible: true,
+        });
+        await this.page.waitForSelector('span.e2e-test-autosave-indicator', {
+          hidden: true,
+        });


#20180 (comment)

rahat2134 · 2024-04-23T17:45:02Z

@seanlip PTAL. Although this has already been reviewed by you. But for the sake of convenience take a look again.
Modification -
• Add a wait for autosave pop to be visible and then hidden (for desktop) when we make any change.
• Have to add external wait For of 2 seconds because in mobile viewport we don't have Auto saving.... pop up. There is no UI change. So, At last, I have to use waitForTimeout

StephenYu2018

Several concerns.

...ptance-tests/spec/exploration-editor-tests/savedraft-publish-and-discard-the-changes.spec.ts

StephenYu2018 · 2024-04-23T18:16:04Z

core/tests/puppeteer-acceptance-tests/user-utilities/exploration-editor-utils.ts

+        // Navbar text is hidden in mobile view port due to less screen so there is no visible
+        // change in the UI (specially Auto save pop up bar) after we update the input bar.
+        // Hence we need to explicitly wait for 2 seconds.
+        await this.page.waitForTimeout(2000);


Are there any network requests after updating the input bar? If so, maybe try page.waitForNetworkIdle() instead of page.waitForTimeout(2000)? (Documentation for the method is here)

No network request. Have tried this also. Thanks for this.

Waiting for a fixed timeout doesn't guarantee that the autosave has completed. It's a bit of a guess and could lead to flaky tests if the autosave takes longer than expected. Maybe we can confirm the same via some other way?

Have to use 2 seconds.webm
PTAL. No UI change, no network request. I've attempted over 30 times. It's not exceeding 1.6 seconds. For ease, we've set it at 2 seconds (to avoid flakiness). If you have any ideas, feel free to suggest. Thanks :)

No waiting in acceptance tests. This is an absolute rule. If there is any issue with something not being clear in the UI then please fix the UI, because if the acceptance test does not know when something finishes then an end user will not know either.

@seanlip @Akhilesh-max is the video visible in your machines?

The video is visible but "weird". I cannot scrub it or see the full length of the video timeline. I suggest using a different format too if possible.

Yes, this video format took a while to load and seek the video.

Is it worth using a different format to upload the video, then? Other contributors' videos don't seem to have this problem...

Sure, I will start using a different video format from now on. Thank you :)

StephenYu2018 · 2024-04-23T18:19:46Z

...nce-tests/spec/exploration-editor-tests/create-exploration-and-change-basic-settings.spec.ts

@@ -13,7 +13,7 @@
 // limitations under the License.

 /**
- * @fileoverview Acceptance Test for Exploration Creator and Exploration Manager
+ * @fileoverview Acceptance Test for settings tab in exploration editor.


Thanks for catching this! :)

.github/workflows/e2e_lighthouse_performance_acceptance_tests.yml

scripts/common.py

oppiabot · 2024-04-23T18:25:19Z

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

rahat2134 · 2024-04-24T10:01:00Z

@StephenYu2018 PTAL

Akhilesh-max

@rahat2134, moving forward, I, along with others, will be reviewing the Acceptance tests. I’ve left some comments for your consideration. Please have a look when you get a chance.
Thanks.

Akhilesh-max · 2024-04-24T10:09:17Z

...tance-tests/spec/exploration-editor-tests/save-draft-publish-and-discard-the-changes.spec.ts

+      'explorationEditor',
+      'exploration_editor@example.com'
+    );
+    showMessage('explorationEditor is signed up successfully.');


'explorationEditor has successfully signed up'.
Here, explorationEditor is the user, right?

Akhilesh-max · 2024-04-24T10:10:45Z

...tance-tests/spec/exploration-editor-tests/save-draft-publish-and-discard-the-changes.spec.ts

+      'explorationVisitor',
+      'exploration_visitor@example.com'
+    );
+    showMessage('explorationVisitor is signed up successfully.');


'explorationVisitor has successfully signed up' ?

Akhilesh-max · 2024-04-24T10:20:50Z

...tance-tests/spec/exploration-editor-tests/save-draft-publish-and-discard-the-changes.spec.ts

+      await explorationEditor.dismissWelcomeModal();
+
+      await explorationEditor.createExplorationWithMinimumContent(
+        'Exploration intro text',


Considering that the function receives both content and interaction parameters and not just content, it might be more appropriate to use createMinimalExploration(). What do you think?

Seems reasonable. Thanks :)

Akhilesh-max · 2024-04-24T10:31:50Z

...tance-tests/spec/exploration-editor-tests/save-draft-publish-and-discard-the-changes.spec.ts

+
+      await explorationEditor.saveExplorationDraft();
+      explorationId = await explorationEditor.publishExplorationWithContent(
+        'Old Title',


Actually, while publishing exploration, we don't really pass any content, what we pass is a title (if it's not already set in the settings tab), objective and category. so maybe we can use publishExplorationWithMetadata()? What do you think?

Although the previous name was clear. But this is more specific. Thanks for suggestion :)

Akhilesh-max · 2024-04-24T11:08:00Z

core/tests/puppeteer-acceptance-tests/user-utilities/exploration-editor-utils.ts

+        // Navbar text is hidden in mobile view port due to less screen so there is no visible
+        // change in the UI (specially Auto save pop up bar) after we update the input bar.
+        // Hence we need to explicitly wait for 2 seconds.
+        await this.page.waitForTimeout(2000);


Waiting for a fixed timeout doesn't guarantee that the autosave has completed. It's a bit of a guess and could lead to flaky tests if the autosave takes longer than expected. Maybe we can confirm the same via some other way?

Akhilesh-max · 2024-04-24T11:19:08Z

core/tests/puppeteer-acceptance-tests/user-utilities/exploration-editor-utils.ts

+      if (this.isViewportAtMobileWidth()) {
+        // Navbar text is hidden in mobile view port due to less screen so there is no visible
+        // change in the UI (specially Auto save pop up bar) after we update the input bar.
+        // Hence we need to explicitly wait for 2 seconds.
+        await this.page.waitForTimeout(2000);
+      } else {
+        await this.page.waitForSelector('span.e2e-test-autosave-indicator', {
+          visible: true,
+        });
+        await this.page.waitForSelector('span.e2e-test-autosave-indicator', {
+          hidden: true,
+        });
+      }
+    }


Maybe, we can have a separate function to wait for Auto Save? maybe, waitForAutoSaveIndicator()?

Good idea. Thanks for this :)

Akhilesh-max · 2024-04-24T11:26:50Z

...tance-tests/spec/exploration-editor-tests/save-draft-publish-and-discard-the-changes.spec.ts

+// limitations under the License.
+
+/**
+ * @fileoverview Acceptance Test for savedraft, publish and discard the changes.


Suggested change

* @fileoverview Acceptance Test for savedraft, publish and discard the changes.

* @fileoverview Acceptance Test for saving drafts, publishing, and discarding changes.

…tionEditor_

oppiabot · 2024-04-26T19:28:13Z

Hi @rahat2134. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

seanlip · 2024-04-27T11:36:51Z

...nce-tests/spec/exploration-editor-tests/create-exploration-and-change-basic-settings.spec.ts

@@ -38,12 +39,14 @@ ConsoleReporter.setConsoleErrorsToIgnore([
  /Error: Could not find the resource http:\/\/localhost:8181\/explorehandler\/features\/[a-zA-Z0-9]+\.?/,
  /Could not find the resource http:\/\/localhost:8181\/createhandler\/permissions\/[a-zA-Z0-9]+\.?/,
  /http:\/\/localhost:8181\/build\/webpack_bundles\/exploration_editor\.[a-f0-9]+\.bundle\.js/,
+  /http:\/\/localhost:8181\/create\/[a-zA-Z0-9]+#\/gui\/Introduction Failed to load resource: the server responded with a status of 404 \(Not Found\)/,


If these errors are unexpected, these should be linked to issues, and we should drop the statement here when the issue is fixed.

In any case a comment should be left to explain the context of each error and the reason we exclude it.

Actually I have already added a statement before all these errors.
These errors occurred when we deleted the exploration and then tried to access it. Most of these are not found type issues.

Actually, I think that's a problem. Each console error should have its own comment (because otherwise, if we add new console errors and functionality in future, the comment won't apply to it). Secondly, only a single 404 console error should arise in cases like this, so it's probably worth filing a bug to fix that -- we shouldn't even get to the point of loading the rest of the page once the initial 404 is received.

seanlip · 2024-04-27T11:37:55Z

core/tests/puppeteer-acceptance-tests/user-utilities/exploration-editor-utils.ts

@@ -277,6 +276,8 @@ export class ExplorationEditor extends BaseUser {
    await this.type(addTitleBar, title);
    await this.page.keyboard.press('Tab');

+    // Auto save pop up bar is visible only when current input is different from


The autosave pop-up bar is ...

seanlip · 2024-04-27T11:39:44Z

core/tests/puppeteer-acceptance-tests/user-utilities/exploration-editor-utils.ts

+        // Navbar text is hidden in mobile view port due to less screen so there is no visible
+        // change in the UI (specially Auto save pop up bar) after we update the input bar.
+        // Hence we need to explicitly wait for 2 seconds.
+        await this.page.waitForTimeout(2000);


I cannot approve a PR that has waitForTimeout in it. Please fix before submitting.

/cc @StephenYu2018

@seanlip I have already raised a PR #20231 and trying to get it merge.

seanlip · 2024-04-27T11:40:19Z

core/tests/puppeteer-acceptance-tests/user-utilities/exploration-editor-utils.ts

+        // Navbar text is hidden in mobile view port due to less screen so there is no visible
+        // change in the UI (specially Auto save pop up bar) after we update the input bar.
+        // Hence we need to explicitly wait for 2 seconds.
+        await this.page.waitForTimeout(2000);


The video is visible but "weird". I cannot scrub it or see the full length of the video timeline. I suggest using a different format too if possible.

rahat2134 · 2024-04-27T16:43:20Z

Changing PR to draft PR till #20231 got merge

seanlip · 2024-04-27T17:20:39Z

...nce-tests/spec/exploration-editor-tests/create-exploration-and-change-basic-settings.spec.ts

@@ -38,12 +39,14 @@ ConsoleReporter.setConsoleErrorsToIgnore([
  /Error: Could not find the resource http:\/\/localhost:8181\/explorehandler\/features\/[a-zA-Z0-9]+\.?/,
  /Could not find the resource http:\/\/localhost:8181\/createhandler\/permissions\/[a-zA-Z0-9]+\.?/,
  /http:\/\/localhost:8181\/build\/webpack_bundles\/exploration_editor\.[a-f0-9]+\.bundle\.js/,
+  /http:\/\/localhost:8181\/create\/[a-zA-Z0-9]+#\/gui\/Introduction Failed to load resource: the server responded with a status of 404 \(Not Found\)/,


Actually, I think that's a problem. Each console error should have its own comment (because otherwise, if we add new console errors and functionality in future, the comment won't apply to it). Secondly, only a single 404 console error should arise in cases like this, so it's probably worth filing a bug to fix that -- we shouldn't even get to the point of loading the rest of the page once the initial 404 is received.

…tionEditor_

… into ExplorationEditor_

oppiabot · 2024-05-15T18:02:33Z

Hi @rahat2134, 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!

…tionEditor_

… into ExplorationEditor_

saveDraft spec

60e18a2

rahat2134 requested review from a team as code owners April 23, 2024 16:54

rahat2134 requested review from DubeySandeep and StephenYu2018 April 23, 2024 16:54

oppiabot bot assigned rahat2134 Apr 23, 2024

rahat2134 requested review from seanlip and removed request for DubeySandeep and StephenYu2018 April 23, 2024 16:54

change

35fd534

rahat2134 commented Apr 23, 2024

View reviewed changes

change

8acdeb1

rahat2134 assigned seanlip and unassigned rahat2134 Apr 23, 2024

StephenYu2018 requested changes Apr 23, 2024

View reviewed changes

oppiabot bot assigned rahat2134 Apr 23, 2024

rahat2134 and others added 3 commits April 24, 2024 00:38

change

e892a74

Merge branch 'develop' into ExplorationEditor_

dc9bdf8

Merge branch 'develop' into ExplorationEditor_

940a919

rahat2134 assigned StephenYu2018 and unassigned rahat2134 Apr 24, 2024

Akhilesh-max reviewed Apr 24, 2024

View reviewed changes

rahat2134 added 3 commits April 24, 2024 17:55

changes

d129dd7

changes

5679fba

Merge branch 'develop' of https://github.com/oppia/oppia into Explora…

1694b1f

…tionEditor_

oppiabot bot added the PR: don't merge - HAS MERGE CONFLICTS label Apr 26, 2024

resolve merge conflict

b3336ca

oppiabot bot removed the PR: don't merge - HAS MERGE CONFLICTS label Apr 27, 2024

rahat2134 and others added 3 commits April 27, 2024 14:47

correct function name in new PR

92c6bda

Merge branch 'develop' into ExplorationEditor_

6cb4377

Merge branch 'develop' into ExplorationEditor_

96055da

seanlip requested changes Apr 27, 2024

View reviewed changes

rahat2134 mentioned this pull request Apr 27, 2024

Fix#20230: Disable the discard button until Autosave is in progress. #20231

Merged

5 tasks

rahat2134 marked this pull request as draft April 27, 2024 16:43

Merge branch 'develop' into ExplorationEditor_

2b0696b

seanlip requested changes Apr 27, 2024

View reviewed changes

rahat2134 and others added 7 commits April 30, 2024 11:36

Merge branch 'develop' into ExplorationEditor_

2f3ce5c

Merge branch 'develop' into ExplorationEditor_

33cd472

Merge branch 'develop' into ExplorationEditor_

563badb

Merge branch 'develop' of https://github.com/oppia/oppia into Explora…

2342c99

…tionEditor_

Merge branch 'develop' into ExplorationEditor_

86f6696

removed timeout

c03be55

Merge branch 'ExplorationEditor_' of https://github.com/rahat2134/oppia…

ae0dca2

… into ExplorationEditor_

oppiabot bot added the stale label May 15, 2024

Merge branch 'develop' into ExplorationEditor_

ee3eab4

oppiabot bot removed the stale label May 15, 2024

rahat2134 and others added 6 commits May 19, 2024 23:38

Merge branch 'develop' of https://github.com/oppia/oppia into Explora…

dbf8ad9

…tionEditor_

Merge branch 'develop' into ExplorationEditor_

c9b86e2

Merge branch 'develop' of https://github.com/oppia/oppia into Explora…

4b0771d

…tionEditor_

div -> button

e99c3a0

change

421ef21

Merge branch 'ExplorationEditor_' of https://github.com/rahat2134/oppia…

94d8192

… into ExplorationEditor_

	* @fileoverview Acceptance Test for savedraft, publish and discard the changes.
	* @fileoverview Acceptance Test for saving drafts, publishing, and discarding changes.

Implement part of #17712 : Acceptance tests for Exploration Creator Section(CUJ 10). #20203

Are you sure you want to change the base?

Implement part of #17712 : Acceptance tests for Exploration Creator Section(CUJ 10). #20203

Conversation

rahat2134 commented Apr 23, 2024

Overview

Essential Checklist

Proof that changes are correct

PR Pointers

oppiabot bot commented Apr 23, 2024

Choose a reason for hiding this comment

rahat2134 commented Apr 23, 2024

StephenYu2018 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahat2134 Apr 23, 2024 • edited

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 Apr 23, 2024

rahat2134 commented Apr 24, 2024

Akhilesh-max 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

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 Apr 26, 2024

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

rahat2134 commented Apr 27, 2024

Choose a reason for hiding this comment

oppiabot bot commented May 15, 2024

rahat2134 Apr 23, 2024 •

edited