-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
[GSoC'24] M2.1 : Fix a part of #20374: Acceptance test coverage for 2 Logged-out Users' CUJs. #20674
base: develop
Are you sure you want to change the base?
Conversation
Hi @Akhilesh-max, can you complete the following:
|
Assigning @imchristie for the first pass review of this PR. Thanks! |
@imchristie These tests ain't really small, there are lots of utilities needed. For now, have only written the specs file and decided what all is needed for the tests. Hoping to complete this PR by today EOD, IST. |
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.
Left some comments. Reassign me for review when all the functions are defined.
// limitations under the License. | ||
|
||
/** | ||
* @fileoverview Acceptance Test for checking if all buttons on the |
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.
Wrong description
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.
Done.
* @param {string} explorationId - The ID of the exploration to play. | ||
*/ | ||
async playExploration(explorationId: string): Promise<void> { | ||
await Promise.all([ |
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.
Use this.goto()
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.
Done.
core/templates/pages/exploration-player-page/layout-directives/feedback-popup.component.ts
Show resolved
Hide resolved
@DubeySandeep @nikitaevg PTAL. |
Unassigning @Akhilesh-max since a re-review was requested. @Akhilesh-max, please make sure you have addressed all review comments. Thanks! |
Hi @Akhilesh-max. 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! |
Hi @Akhilesh-max. 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! |
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.
Left some comments
core/tests/puppeteer-acceptance-tests/utilities/user/logged-out-user.ts
Outdated
Show resolved
Hide resolved
core/tests/puppeteer-acceptance-tests/utilities/user/logged-out-user.ts
Outdated
Show resolved
Hide resolved
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.
Why not delete the whole file?
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.
Some of the functions in this utility file are being used in some other wdio e2e tests, so, couldn't remove it whole. (Only removed the dead functions)
@imchristie PTAL. |
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.
LGTM! But there're 6 failures in CI. Make sure to fix them before merging.
Unassigning @imchristie since they have already approved the 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.
LGTM for the frontend changes
Unassigning @nikitaevg since they have already approved the PR. |
Overview
12.1: select-and-play-topic-from-math-classroom.spec.ts
12.2: browse-and-search-for-lessons-in-community-library.spec.ts
Essential Checklist
Please follow the instructions for making a code change.
Proof that changes are correct
NA
PR Pointers