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
Implements part of #17712 : Acceptance tests for CUJs in the history tab of the exploration creator. #20191
Conversation
Hi @Akhilesh-max, can you complete the following:
|
Hi @Akhilesh-max please assign the required reviewer(s) for this PR. 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.
Several concerns.
core/tests/puppeteer-acceptance-tests/user-utilities/exploration-editor-utils.ts
Outdated
Show resolved
Hide resolved
core/tests/puppeteer-acceptance-tests/user-utilities/exploration-editor-utils.ts
Outdated
Show resolved
Hide resolved
core/tests/puppeteer-acceptance-tests/user-utilities/exploration-editor-utils.ts
Outdated
Show resolved
Hide resolved
core/tests/puppeteer-acceptance-tests/user-utilities/exploration-editor-utils.ts
Outdated
Show resolved
Hide resolved
core/tests/puppeteer-acceptance-tests/user-utilities/exploration-editor-utils.ts
Outdated
Show resolved
Hide resolved
core/tests/puppeteer-acceptance-tests/user-utilities/exploration-editor-utils.ts
Outdated
Show resolved
Hide resolved
core/tests/puppeteer-acceptance-tests/user-utilities/exploration-editor-utils.ts
Outdated
Show resolved
Hide resolved
core/tests/puppeteer-acceptance-tests/user-utilities/exploration-editor-utils.ts
Outdated
Show resolved
Hide resolved
core/tests/puppeteer-acceptance-tests/user-utilities/exploration-editor-utils.ts
Outdated
Show resolved
Hide resolved
core/tests/puppeteer-acceptance-tests/user-utilities/exploration-editor-utils.ts
Show resolved
Hide resolved
Unassigning @Akhilesh-max since a re-review was requested. @Akhilesh-max, please make sure you have addressed all review comments. 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.
Resolved most of my previous comments, with the exception of one.
Also, there's one new concern from me.
core/tests/puppeteer-acceptance-tests/user-utilities/exploration-editor-utils.ts
Outdated
Show resolved
Hide resolved
Unassigning @StephenYu2018 since the review is done. |
Hi @Akhilesh-max, it looks like some changes were requested on this pull request by @StephenYu2018. PTAL. Thanks! |
Thanks @StephenYu2018. Addressed all your comments, I am stuck somewhere, as well. Have left a comment. |
* Downloads a specific version. | ||
* @param {number} version - The version number to be downloaded. | ||
*/ | ||
async downloadVersion(version: number): Promise<void> { |
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.
@imchristie @seanlip @StephenYu2018
I need to confirm this download, wherein this download initiates on clicking the download button which in turn opens a new tab. However, this tab closes too quickly to read any event or fetch its URL for download confirmation. I attempted to monitor network activity, but the file appears to be served directly, without any network request. Additionally, I'm unable to access the download directory as this is set to run on CI.
I tried looking if puppeteer provides some way to confirm the download, but couldn't find any.
May you please share some approach to confirm the download of the revisions?
Thank you.
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.
@Akhilesh-max I don't know offhand, but what online resources have you looked at for handling downloads in puppeteer? That's where I would start -- a quick search showed a bunch of results that might be promising and are probably worth investigating.
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.
Also I think it's probably fine to make slight modifications to the flow for downloading these if needed, if it helps make things a bit easier on the acceptance tests side of things.
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!
Assigning @nikitaevg, @U8NWXD for code owner reviews. 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.
LGTM on the frontend changes
Unassigning @nikitaevg since they have already approved the PR. |
Hi @Akhilesh-max, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. 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, 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
(serial numbers same as testing spreadsheet row number for each user type)
Essential Checklist
Please follow the instructions for making a code change.
PR Pointers