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
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! |
@@ -38,12 +39,14 @@ ConsoleReporter.setConsoleErrorsToIgnore([ | |||
/Error: Could not find the resource http:\/\/localhost:8181\/explorehandler\/features\/[a-zA-Z0-9]+\.?/, | |||
/Could not find the resource http:\/\/localhost:8181\/createhandler\/permissions\/[a-zA-Z0-9]+\.?/, | |||
/http:\/\/localhost:8181\/build\/webpack_bundles\/exploration_editor\.[a-f0-9]+\.bundle\.js/, | |||
/http:\/\/localhost:8181\/create\/[a-zA-Z0-9]+#\/gui\/Introduction Failed to load resource: the server responded with a status of 404 \(Not Found\)/, |
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.
If these errors are unexpected, these should be linked to issues, and we should drop the statement here when the issue is fixed.
In any case a comment should be left to explain the context of each error and the reason we exclude 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.
Actually I have already added a statement before all these errors.
These errors occurred when we deleted the exploration and then tried to access it. Most of these are not found
type issues.
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, I think that's a problem. Each console error should have its own comment (because otherwise, if we add new console errors and functionality in future, the comment won't apply to it). Secondly, only a single 404 console error should arise in cases like this, so it's probably worth filing a bug to fix that -- we shouldn't even get to the point of loading the rest of the page once the initial 404 is received.
@@ -277,6 +276,8 @@ export class ExplorationEditor extends BaseUser { | |||
await this.type(addTitleBar, title); | |||
await this.page.keyboard.press('Tab'); | |||
|
|||
// Auto save pop up bar is visible only when current input is different from |
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 autosave pop-up bar is ...
// 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.
I cannot approve a PR that has waitForTimeout in it. Please fix before submitting.
/cc @StephenYu2018
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.
// 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.
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.
Changing PR to draft PR till #20231 got merge |
@@ -38,12 +39,14 @@ ConsoleReporter.setConsoleErrorsToIgnore([ | |||
/Error: Could not find the resource http:\/\/localhost:8181\/explorehandler\/features\/[a-zA-Z0-9]+\.?/, | |||
/Could not find the resource http:\/\/localhost:8181\/createhandler\/permissions\/[a-zA-Z0-9]+\.?/, | |||
/http:\/\/localhost:8181\/build\/webpack_bundles\/exploration_editor\.[a-f0-9]+\.bundle\.js/, | |||
/http:\/\/localhost:8181\/create\/[a-zA-Z0-9]+#\/gui\/Introduction Failed to load resource: the server responded with a status of 404 \(Not Found\)/, |
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, I think that's a problem. Each console error should have its own comment (because otherwise, if we add new console errors and functionality in future, the comment won't apply to it). Secondly, only a single 404 console error should arise in cases like this, so it's probably worth filing a bug to fix that -- we shouldn't even get to the point of loading the rest of the page once the initial 404 is received.
… into ExplorationEditor_
Hi @rahat2134, 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. |
… into ExplorationEditor_
Overview
fixes part of Acceptance Testing - covering all user journeys in form of user stories #17712 .
Points:
10- User can Publish the latest changes, User can draft the latest changes)
Essential Checklist
Proof that changes are correct
PR Pointers