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

rahat2134 · 2024-02-20T08:49:49Z

Overview

This PR
fixes part of Acceptance Testing - covering all the Creator's and Contributor's CUJs. #17712 .
This PR does the following: Adds acceptance test for exploration editor section.
Points:
( 9- User can do the Basic setting: Title, Goal, Add a category, Language, Name of the first card, Tags, Advance features, Roles, Voice artist, Permissions ……..
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.

…lorationEditor

oppiabot · 2024-02-20T08:50:00Z

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

seanlip

Thanks @rahat2134. The "top-level" of the tests needs some work so I've focused on that in this review, PTAL.

seanlip · 2024-02-22T09:34:38Z

core/templates/components/ck-editor-helpers/ck-editor-4-rte.component.html

@@ -2,7 +2,7 @@
  <div class="d-flex flex-column-reverse">
    <span class="oppia-cke-offline-warning" *ngIf="!connectedToInternet">* Tools with dark background can not be used when offline.</span>
  </div>
-  <div contenteditable="true" class="oppia-rte-resizer oppia-rte e2e-test-rte" #oppiaRTE></div>
+  <div contenteditable="true" class="oppia-rte-resizer oppia-rte e2e-test-rte" id="oppia-exploration-introd-box-input" #oppiaRTE></div>


Please don't use IDs. See existing tests which use classes. We try not to use IDs because IDs are supposed to be unique within the page and it's hard to guarantee that (especially when the page is made up of multiple components).

@seanlip I have resolved this issue. No id used. ✅

seanlip · 2024-02-22T09:36:16Z

core/tests/puppeteer-acceptance-tests/puppeteer-testing-utilities/user-factory.js

+ * @param {string} username - The username of the exploration creator.
+ * @returns {e2eExplorationCreator} - The instance of the exploration creator.
+ */
+let createExplorationCreator = async function(username) {


Please follow the existing pattern set by other function names in this file.

@seanlip its also changed to createNewExplorationCreator() as per pattern.✅

seanlip · 2024-02-22T09:37:40Z

...puppeteer-acceptance-tests/spec/exploration-editor-tests/exploration-creator-manager.spec.js

+    superAdmin = await userFactory.createNewSuperAdmin('Leader');
+  }, DEFAULT_SPEC_TIMEOUT);
+
+  it('should perform exploration creation and basic actions',


This is too vague. Explain more clearly what the user is doing in this test. I can't tell by reading the test description.

Also I think you might need to split this into multiple tests, each testing a particular scenario. I notice that all of these tests are from the creator's point of view only and don't have adequate verification. Think through what is needed to verify these actions from a user perspective. For example, if you want to verify that the title has changed then there should be another actor (a lesson player) who views the exploration and confirms that the title there has changed as well. Or if you want to verify that the exploration is deleted then another actor (a lesson player) should go to that URL and find a 404. Similarly for the others.

You might want to try this with just one scenario first, get that reviewed and merged, then tackle the other scenarios.

@seanlip PTAL.

1)split this into multiple tests - ✅
2)If we want to verify whether the title has changed or any other aspect of the exploration from the user's perspective, the exploration creator needs to publish the exploration. This is because a private exploration will not be visible to a guest user. To publish the exploration, we need to fill in all the required fields such as title, goal, language, category, etc. Only after filling in all these details, we can publish the exploration and verify it. However, this approach of checking/verifying after all step is not efficient. Rather, it would be better to use the exploration creator and check the exploration at each step. For other aspects of the exploration, we can check them after publishing from user perspective.

Seems reasonable, feel free to structure the test so that it is reasonably efficient from a "flow" perspective.

However, I think creators might be able to view the /explore/... page for their own private explorations (not 100% sure about this though). If so, then maybe that might help simplify things.

Hmm, correct they are able to view. That's what i am trying to use in tests also. Thanks.

seanlip · 2024-02-22T09:38:15Z

...puppeteer-acceptance-tests/spec/exploration-editor-tests/exploration-creator-manager.spec.js

+
+      await explorationCreator.createExploration();
+      await explorationCreator.goToBasicSettingsTab();
+      await explorationCreator.expectTitleToHaveMaxLength(36);


This is not something the exploration creator "expects" -- write these actions from the perspective of a user.

@seanlip i think this is now resolved .

I'm not sure it's resolved. If I understand Sean correctly, the creator doesn't only expect the length to be below 36, they expect the title to be equal to "Your Title here".

@seanlip its also answered, as it was mentioned in doc to check only for title to be below 36 so..

This acceptance test must be written from the perspective of an end user. An end user will not go to the exploration editor, update the title, and expect it to have a length of less than 36. They might expect a specific warning to be shown. Or they might expect that, if they try to enter a title that has > 36 chars, it shows an error (or if they enter characters past a certain length, it shows the truncated title only). And so on.

Your test should be written so that it reads similar to end user actions, as described above. If the spreadsheet does not match this then the spreadsheet needs modification and you might want to request clarification for those cases.

Also the link in your last comment doesn't load for me FYI.

seanlip · 2024-02-22T09:38:29Z

...puppeteer-acceptance-tests/spec/exploration-editor-tests/exploration-creator-manager.spec.js

+        'explorationAdm', 'voiceover admin');
+
+      await explorationCreator.createExploration();
+      await explorationCreator.goToBasicSettingsTab();


We only have a settings tab, not a "basic settings" tab.

resolved by new commits ✅

seanlip · 2024-02-22T18:00:50Z

...puppeteer-acceptance-tests/spec/exploration-editor-tests/exploration-creator-manager.spec.js

+      await explorationCreator.makeExplorationPublic();
+      await explorationCreator.expectExplorationAccessibility();
+      await explorationCreator.voiceArtistAdded();
+      await explorationCreator.expectVoiceArtistToBeAdded();


Which voice artist? I would expect this to be parameterized.

Sure, I will parameterize all the needed functions. I had hard-coded the person as 'guestUsr3' in the utils.js file. If it needs to be parameterized, I will rectify it.

Yup, in general your utility functions should be general things that do the desired action (and the internals of the function should not make assumptions about data on the server, which users there are, etc. -- those things would need to be parameterized).

@seanlip its also resolved as now i have parameterised all the required functions->

etc.

seanlip · 2024-02-22T18:00:58Z

...puppeteer-acceptance-tests/spec/exploration-editor-tests/exploration-creator-manager.spec.js

+      await explorationCreator.expectExplorationAccessibility();
+      await explorationCreator.voiceArtistAdded();
+      await explorationCreator.expectVoiceArtistToBeAdded();
+      await explorationCreator.selectVoiceArtist();


Don't know what this means from PM perspective.

@seanlip it need to be deleted, so its deleted. Main steps include -> add voice artists.

...puppeteer-acceptance-tests/spec/exploration-editor-tests/exploration-creator-manager.spec.js

seanlip · 2024-02-22T18:01:40Z

...puppeteer-acceptance-tests/spec/exploration-editor-tests/exploration-creator-manager.spec.js

+      await explorationCreator.expectVoiceArtistToBeAdded();
+      await explorationCreator.selectVoiceArtist();
+      await explorationCreator.chooseToRecieveNotification();
+      await explorationCreator.expectFeedbackNotificationChoice();


Generally, any expect() methods should have a parameter with the expected outcome (unless the expected outcome is obvious from the name of the method, like expectExplorationToBeDeleted).

I will keep this in mind.

@seanlip its also resolved in above comments.

seanlip · 2024-02-22T18:02:15Z

...ppeteer-acceptance-tests/spec/exploration-editor-tests/exploration-publisher-drafter.spec.js

+
+  it('should perform exploration management actions',
+    async function() {
+      await explorationCreator.createExploration();


You may need to separate these.

Think very carefully about how you would verify the outcomes of these actions, too. Don't just simply have the actions be executed. You will probably want multiple separate tests, with each test testing a flow in detail.

@seanlip It is now resolved. I have separated it and made it more readable.

rahat2134 · 2024-02-22T18:33:41Z

Thanks @rahat2134. The "top-level" of the tests needs some work so I've focused on that in this review, PTAL.

Hmm, most of the changes required the naming convention. Will keep this in mind and try to solve this and utils.js file also in next commit.
Thank You.

…lorationEditor

oppiabot · 2024-02-25T17:42:21Z

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!

oppiabot · 2024-02-25T17:42:24Z

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!

…into ExplorationEditor

seanlip

Thanks @rahat2134. One small thing, otherwise basically LGTM!

seanlip · 2024-04-20T13:48:09Z

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

@@ -188,6 +188,9 @@ export class BaseUser {
   */
  async clearAllTextFrom(selector: string): Promise<void> {
    await this.waitForElementToBeClickable(selector);
+    /**


For inline comments, use this format:

// Clicking three times ...

seanlip · 2024-04-20T13:50:38Z

(I think you can get this merged once @StephenYu2018 approves, too. Well done!)

oppiabot · 2024-04-20T13:53:02Z

Assigning @U8NWXD for code owner reviews. Thanks!

rahat2134 · 2024-04-20T13:57:59Z

@StephenYu2018 PTAL. Addressed all the changes. If you feel satisfied please add this to the merge queue. Thanks

oppiabot · 2024-04-20T14:39:53Z

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!

StephenYu2018

LGTM! :)

oppiabot · 2024-04-22T03:00:52Z

Unassigning @StephenYu2018 since they have already approved the PR.

oppiabot · 2024-04-22T03:00:56Z

Hi @rahat2134, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. Thanks!

rahat2134 · 2024-04-22T04:17:10Z

@seanlip Please add this to the merge queue if no further review is required. Thanks.

rahat2134 added 7 commits February 16, 2024 15:33

firstcommitBasicSetting/advancedsettingCompleted

d8f80a2

Some flaky modification needed else done with the pr

25a0c27

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

3d8a5ea

…lorationEditor

Lint Checks Failed

b2cda04

lint-checks-failed

12bede7

Lint-checks-breaking

c72ab37

Lint-updates

ddd8f77

oppiabot bot assigned rahat2134 Feb 20, 2024

Merge branch 'develop' into ExplorationEditor

6412e7f

rahat2134 mentioned this pull request Feb 20, 2024

Acceptance Testing - covering all the Creator's and Contributor's CUJs. #17712

Open

14 tasks

Merge branch 'develop' into ExplorationEditor

130ffc4

seanlip requested changes Feb 22, 2024

View reviewed changes

rahat2134 and others added 7 commits February 24, 2024 16:36

mention-changes-seperateTesting-

00ee106

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

49aee90

…lorationEditor

Lint-Check-Passed

7381d8b

Lint-Checked-Passed

f96e6be

Merge branch 'develop' into ExplorationEditor

6182607

Merge branch 'develop' into ExplorationEditor

d7e01d8

Parameterised-required-functions

2561473

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

rahat2134 added 6 commits February 26, 2024 02:31

allign-functions-correctly

0d21f82

lintpass

e4059cb

done

51995bb

Merged changes from both branches

3949c2b

resolve merge conflict

4c7a0bc

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

7cc39de

…into ExplorationEditor

seanlip approved these changes Apr 20, 2024

View reviewed changes

seanlip assigned rahat2134 and unassigned seanlip Apr 20, 2024

seanlip enabled auto-merge April 20, 2024 13:50

seanlip disabled auto-merge April 20, 2024 13:50

oppiabot bot assigned U8NWXD Apr 20, 2024

change

9b55d86

change

7b96a5d

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

rahat2134 added 2 commits April 21, 2024 11:08

resolve merge confilct

4fa469d

lint check

038c7df

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

rahat2134 removed their assignment Apr 21, 2024

changes

835af8a

U8NWXD approved these changes Apr 21, 2024

View reviewed changes

U8NWXD removed their assignment Apr 21, 2024

StephenYu2018 approved these changes Apr 22, 2024

View reviewed changes

oppiabot bot unassigned StephenYu2018 Apr 22, 2024

oppiabot bot added the PR: LGTM label Apr 22, 2024

oppiabot bot assigned rahat2134 Apr 22, 2024

seanlip added this pull request to the merge queue Apr 22, 2024

Merged via the queue into oppia:develop with commit 5abffd3 Apr 22, 2024
80 checks passed

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

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

Conversation

rahat2134 commented Feb 20, 2024 • edited Loading

Overview

Essential Checklist

Proof that changes are correct

PR Pointers

oppiabot bot commented Feb 20, 2024

seanlip 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

rahat2134 Feb 25, 2024 • edited Loading

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 Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahat2134 Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahat2134 Feb 22, 2024 • edited Loading

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

rahat2134 commented Feb 22, 2024

oppiabot bot commented Feb 25, 2024

oppiabot bot commented Feb 25, 2024

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Apr 20, 2024 • edited Loading

oppiabot bot commented Apr 20, 2024

rahat2134 commented Apr 20, 2024 • edited Loading

oppiabot bot commented Apr 20, 2024

StephenYu2018 left a comment

Choose a reason for hiding this comment

oppiabot bot commented Apr 22, 2024

oppiabot bot commented Apr 22, 2024

rahat2134 commented Apr 22, 2024

rahat2134 commented Feb 20, 2024 •

edited

Loading

rahat2134 Feb 25, 2024 •

edited

Loading

rahat2134 Feb 28, 2024 •

edited

Loading

rahat2134 Feb 26, 2024 •

edited

Loading

rahat2134 Feb 22, 2024 •

edited

Loading

seanlip commented Apr 20, 2024 •

edited

Loading

rahat2134 commented Apr 20, 2024 •

edited

Loading