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
Fix part of #17712: Acceptance test for logged-in learner #20043
Fix part of #17712: Acceptance test for logged-in learner #20043
Conversation
Assigning @StephenYu2018 for the first pass review of this PR. Thanks! |
@StephenYu2018 Can you please take a pass on this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 concerns to address before merging.
...ogged-in-user-tests/try-to-add-exploration-to-play-latee-give-feedback-and-report-it.spec.ts
Outdated
Show resolved
Hide resolved
core/tests/puppeteer-acceptance-tests/user-utilities/logged-in-users-utils.ts
Outdated
Show resolved
Hide resolved
core/tests/puppeteer-acceptance-tests/user-utilities/logged-in-users-utils.ts
Outdated
Show resolved
Hide resolved
Unassigning @StephenYu2018 since the review is done. |
@AFZL210 Add the console error as a regex to match it inside puppeteer-testing-utilities/console-reporter.ts under CONSOLE_ERRORS_TO_FIX with a TODO pointing to the existing issue or create a new issue if there isn't one filed. |
@@ -318,6 +318,7 @@ jobs: | |||
- logged-in-user-tests/click-all-buttons-on-navbar | |||
- logged-in-user-tests/click-all-links-in-about-oppia-footer | |||
- logged-in-user-tests/click-all-links-on-get-started-page | |||
- logged-in-user-tests/try-to-add-exploration-to-play-latee-give-feedback-and-report-it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
later is misspelled
@@ -32,6 +32,7 @@ export default { | |||
Contact: 'http://localhost:8181/contact', | |||
ContributorDashboardAdmin: | |||
'http://localhost:8181/contributor-admin-dashboard', | |||
LearnerDashboard: 'http://localhost:8181/learner-dashboard', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep this list in alphabetical order.
'exploration_creator@example.com' | ||
); | ||
|
||
await explorationCreator.createEndExploration(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean? I don't think it makes sense.
'Test Exploration' | ||
); | ||
await testLearner.expectReachedTheEndOfExploration(); | ||
await testLearner.reportTheExploration('Testing the functionality'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reportExploration
'Testing Feedback functionality' | ||
); | ||
|
||
await explorationCreator.openExplorationEditor('Test Exploration'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a more specific method name. Why we are giving input as 'Test Exploration' in openExplorationEditor?
Can we have something like openExplorationWithTitle('test Exploration')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed top level tests and utils. PTAL.
'exploration_creator@example.com' | ||
); | ||
|
||
await explorationCreator.createEndExploration(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can have something like createExplorationWithMinimumInteraction
...
}, DEFAULT_SPEC_TIMEOUT); | ||
|
||
it( | ||
'should add exploration to play later then remove it then give feedback and report it', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should add exploration to play later, remove it, give feedback, and finally report it.
'should add exploration to play later then remove it then give feedback and report it', | ||
async function () { | ||
await testLearner.navigateToCommunityLibraryPage(); | ||
await testLearner.findExplorationInCommunityLibrary('Test Exploration'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
findExplorationInCommunityLibraryWithTitle('Test Exploration');
|
||
await testLearner.navigateToLearnerDashboardPage(); | ||
await testLearner.openCommunityLessonsTab(); | ||
await testLearner.expectExplorationWithTitleToBePresentInPlayLater( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expectExplorationToBePresentInPlayLaterWithTitle('test Exploration');
'Test Exploration' | ||
); | ||
await testLearner.removeExplorationFromPlayLater(); | ||
await testLearner.expectExplorationWithTitleToBeAbsentInPlayLater( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment.
|
||
/** | ||
* This function to add exploration to play later. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove it.
|
||
/** | ||
* This function to remove exploration from play later. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also.
*/ | ||
async addExplorationToPlayLater(): Promise<void> { | ||
await this.page.hover(communityExplorationCard); | ||
await this.page.click('.e2e-test-add-to-playlist-btn'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont use direct selector, this.clickOn();
await this.navigateToCreatorDashboardPage(); | ||
} | ||
|
||
const allExplorations = await this.page.$$('.oppia-activity-summary-tile'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
explorationsList
await this.clickOn(createExplorationButton); | ||
await this.page.waitForSelector( | ||
`${dismissWelcomeModalSelector}:not([disabled])` | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is required here. Please check.
Tests are failing at CLI, I think some waitFor need to be corrected. |
It's because of the mobile test. I'm moving this PR to draft, as the mobile test can only be implemented after this issue is fixed. |
Hi @AFZL210, 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. |
@AFZL210 We can't have this PR be open but blocked long-term. Are you planning to help address the issue with #19443? |
Make sense, Yes just confirmed with harsh he still haven't got the solution, I will try to reproduce the issue today and try to find the fix. |
Hi @AFZL210, 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. |
Hi @AFZL210, 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. |
Overview
Essential Checklist
Proof that changes are correct
References:
-->
Proof of changes in Arabic language
PR Pointers