-
-
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
Implement part of #17712 : Acceptance tests for Exploration Creator Section(CUJ 10). #20203
base: develop
Are you sure you want to change the base?
Conversation
Hi @rahat2134 please assign the required reviewer(s) for this PR. Thanks! |
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, | ||
}); |
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.
@seanlip PTAL. Although this has already been reviewed by you. But for the sake of convenience take a look again. |
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.
...ptance-tests/spec/exploration-editor-tests/savedraft-publish-and-discard-the-changes.spec.ts
Outdated
Show resolved
Hide resolved
...ptance-tests/spec/exploration-editor-tests/savedraft-publish-and-discard-the-changes.spec.ts
Outdated
Show resolved
Hide resolved
// 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); |
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.
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)
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.
No network request. Have tried this also. Thanks for this.
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.
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?
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.
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 :)
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.
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.
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.
@seanlip @Akhilesh-max is the video visible in your machines?
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.
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.
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.
Yes, this video format took a while to load and seek the video.
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.
Is it worth using a different format to upload the video, then? Other contributors' videos don't seem to have this problem...
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.
Sure, I will start using a different video format from now on. Thank you :)
@@ -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. |
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.
Thanks for catching this! :)
.github/workflows/e2e_lighthouse_performance_acceptance_tests.yml
Outdated
Show resolved
Hide resolved
Hi @rahat2134, it looks like some changes were requested on this pull request by @StephenYu2018. PTAL. Thanks! |
@StephenYu2018 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.
@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.
'explorationEditor', | ||
'exploration_editor@example.com' | ||
); | ||
showMessage('explorationEditor is signed up successfully.'); |
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.
'explorationEditor has successfully signed up'.
Here, explorationEditor is the user, right?
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
'explorationVisitor', | ||
'exploration_visitor@example.com' | ||
); | ||
showMessage('explorationVisitor is signed up successfully.'); |
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.
'explorationVisitor has successfully signed up' ?
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
await explorationEditor.dismissWelcomeModal(); | ||
|
||
await explorationEditor.createExplorationWithMinimumContent( | ||
'Exploration intro text', |
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.
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?
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.
Seems reasonable. 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.
done
|
||
await explorationEditor.saveExplorationDraft(); | ||
explorationId = await explorationEditor.publishExplorationWithContent( | ||
'Old Title', |
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.
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?
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.
Although the previous name was clear. But this is more specific. Thanks for suggestion :)
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
// 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); |
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.
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?
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, | ||
}); | ||
} | ||
} |
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.
Maybe, we can have a separate function to wait for Auto Save? maybe, waitForAutoSaveIndicator()
?
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.
Good idea. Thanks for this :)
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
// limitations under the License. | ||
|
||
/** | ||
* @fileoverview Acceptance Test for savedraft, publish and discard the changes. |
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.
* @fileoverview Acceptance Test for savedraft, publish and discard the changes. | |
* @fileoverview Acceptance Test for saving drafts, publishing, and discarding changes. |
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
… into ExplorationEditor_
@seanlip PTAL. |
Unassigning @rahat2134 since a re-review was requested. @rahat2134, please make sure you have addressed all review comments. Thanks! |
@rahat2134 Apologies, I don't have bandwidth to review, sorry. I'll approve once you get approval from either @imchristie or @Akhilesh-max and all tests pass (please reassign me once you have a clear comment from either of them stating that). |
@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.
I left some comments.
@@ -410,6 +412,46 @@ export class BaseUser { | |||
getCurrentUrlWithoutParameters(): string { | |||
return this.page.url().split('?')[0]; | |||
} | |||
|
|||
/** | |||
* This function checks the exploration accessibility with help of explorationID. |
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 think it's better to say "... checks the exploration accessibility by navigating to the exploration page based on the explorationID"
} | ||
} | ||
|
||
async expectExplorationToBeNotAccessibleByUrl( |
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.
Missing comments for this function
* This function Waits for the autosave indicator to appear and then disappear. | ||
*/ | ||
async waitForAutosaveIndicator(): Promise<void> { | ||
await this.page.waitForSelector('span.e2e-test-autosave-indicator', { |
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.
Please store it as a constant at the beginning of this file. Ditto for the one below.
@@ -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. |
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 description is not clear enough. I suggest changing it to "creating and modifying an exploration on/via the settings tab"
@@ -119,11 +119,10 @@ | |||
</div> | |||
|
|||
<ul class="dropdown-menu" aria-labelledby="discardButtonPopup"> | |||
<!-- The 'disabled' class is added when autosave is in progress. This disables user interactions with the discard button, preventing changes from being discarded before they are saved. --> | |||
<div class="e2e-test-mobile-exploration-discard-tab" (click)="discardChanges()" [class.disabled]="autosaveIsInProgress"> | |||
<button class="btn btn-light e2e-test-mobile-exploration-discard-tab" (click)="discardChanges()" [disabled]="autosaveIsInProgress"> |
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.
Is it necessary to change the HTML tag from div to button? We try not to modify the HTML elements.
Unassigning @imchristie since the review is done. |
Hi @rahat2134, it looks like some changes were requested on this pull request by @imchristie. PTAL. Thanks! |
Overview
fixes part of Acceptance Testing - covering all the Creator's and Contributor's CUJs. #17712 .
Points:
10- User can Publish the latest changes, User can draft the latest changes)
Essential Checklist
Proof that changes are correct
PR Pointers