Fix part of #17712 Add more acceptance tests for logged in users #20126
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| // Copyright 2024 The Oppia Authors. All Rights Reserved. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS-IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| /** | ||
| * @fileoverview Acceptance Test for checking if logged-in users | ||
| * can click all the buttons on the "Teach" page. | ||
| */ | ||
|
|
||
| import {UserFactory} from '../../puppeteer-testing-utilities/user-factory'; | ||
| import {LoggedInUser} from '../../user-utilities/logged-in-users-utils'; | ||
| import testConstants from '../../puppeteer-testing-utilities/test-constants'; | ||
|
|
||
| const DEFAULT_SPEC_TIMEOUT = testConstants.DEFAULT_SPEC_TIMEOUT; | ||
|
|
||
| describe('Logged-in Users', function () { | ||
| let testUser: LoggedInUser; | ||
|
|
||
| beforeAll(async function () { | ||
| testUser = await UserFactory.createNewUser( | ||
| 'testuser', | ||
| 'testuser@example.com' | ||
| ); | ||
| }, DEFAULT_SPEC_TIMEOUT); | ||
|
|
||
| it( | ||
| 'should be able to navigate to the Teach page using the footer', | ||
| async function () { | ||
| await testUser.navigateToAboutFoundationPage(); | ||
| await testUser.navigateToTeachPageViaFooter(); | ||
| }, | ||
| DEFAULT_SPEC_TIMEOUT | ||
| ); | ||
|
|
||
| describe('on the Teach page', function () { | ||
|
There was a problem hiding this comment. I noticed that you are using a describe function inside another describe function in order to check the footer button and then the buttons on the page. It's kinda confusing because the names of these files only mention clicking all the buttons on the page, not the button in the footer. Maybe we should move all the checks for the buttons on the footer into one file, which is the "click-more-links-in-oppia-footer.spec.ts", and change that file name to "click-all-links-in-teach-and-learn-footer.spec.ts" (just like "click-all-links-in-about-oppia-footer.spec.ts"). @seanlip What do you think? There was a problem hiding this comment. Yes, I think I agree -- having the nested describe is kind of confusing. There was a problem hiding this comment. Moved the footer navigation tests to a new file. |
||
| beforeEach(async function () { | ||
| await testUser.navigateToTeachPage(); | ||
| }, DEFAULT_SPEC_TIMEOUT); | ||
|
|
||
| it( | ||
| 'should be able to use the "Browse our Lessons" button', | ||
| async function () { | ||
| await testUser.clickBrowseOurLessonsOnTeachPage(); | ||
|
There was a problem hiding this comment. I should have brought this up earlier, but I'm wondering why the results are not explicitly mentioned in the top-level tests for logged-in users? For example, instead of: await testUser.clickBrowseOurLessonsOnTeachPage()we explicitly mention the result of the prior action: await testUser.clickBrowseOurLessonsOnTeachPage();
await testUser.expectToBeAbleToBrowseLessons();I understand that it may seem obvious on what the effect is for a good amount of these tests. On the flip side, explicitly mentioning the expected result avoids the reader having to spend time thinking about it (even if it isn't for long) and makes the top-level test adhere to "Act, Assert" test structure. There was a problem hiding this comment. I'm okay with pulling the expectations to the top level. I was mostly just following existing paradigms for these "Just click this link" tests already present in this package. @imchristie Do you have an opinion here? I can refactor all the logged-in user tests in a separate PR if that's okay, to not do too much in this PR. There was a problem hiding this comment. For this user story, we are mainly focusing on whether the buttons can open the links, so I personally don't think it's necessary to make another function to check if the link is opened explicitly. I see "expectToBeAbleTo..." functions are mostly used for actions like checking if the user is assigned to a role or checking if the user can publish something, which require more steps. But checking if a link is opened is mostly just a few lines so I don't think it's necessary to add "expectTo..." kind of functions for these. However, I noticed that the names of the functions are not very clear (I might be the one who started it, my bad). Take this function as an example, maybe we should change the name from "clickBrowseOurLessonsOnTeachPage" to "clickAndCheckBrowseOurLessonOnTeachPage"? It might be a bit too long though but at least it explains the functionality of the function. Anyways, we can fix these later in a separate PR so let's discuss it in the group chat. There was a problem hiding this comment. I think it depends on what the failure cases are. If we're saying "user takes an action" (e.g. click) then you can have a follow-up check that verifies that the new page URL is the desired one. Or if the next action in the test is clearly only possible to take on the new page (or only possible to do if the click was registered correctly) then you can omit the expectation. My suggestion in general would be to think about how an end user would validate it. Maybe they look for a bit of text in the new page, and if so you can have something like expectTextToBePresent(...). Or maybe they do some other action on that page (e.g. clicking a particular button) and that way it's clear it's the correct page. So while I don't think a separate expect() call is always necessary, there should always be some kind of validation that the action has succeeded and done the right thing. |
||
| }, | ||
| DEFAULT_SPEC_TIMEOUT | ||
| ); | ||
|
|
||
| it( | ||
| 'should be able to use the "Access the Android app" button', | ||
| async function () { | ||
| await testUser.clickAccessTheAndroidAppOnTeachPage(); | ||
| }, | ||
| DEFAULT_SPEC_TIMEOUT | ||
| ); | ||
|
|
||
| it( | ||
| 'should be able to use the "VISIT CLASSROOM" button', | ||
| async function () { | ||
| await testUser.clickVisitClassroomOnTeachPage(); | ||
| }, | ||
| DEFAULT_SPEC_TIMEOUT | ||
| ); | ||
|
|
||
| it( | ||
| 'should be able to use the "BROWSE LIBRARY" button', | ||
| async function () { | ||
| await testUser.clickBrowseLibraryOnTeachPage(); | ||
| }, | ||
| DEFAULT_SPEC_TIMEOUT | ||
| ); | ||
|
|
||
| it( | ||
| 'should be able to use the "EXPLORE LESSONS" button', | ||
| async function () { | ||
| await testUser.clickExploreLessonsOnTeachPage(); | ||
| }, | ||
| DEFAULT_SPEC_TIMEOUT | ||
| ); | ||
| }); | ||
|
|
||
| afterAll(async function () { | ||
| await UserFactory.closeAllBrowsers(); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| // Copyright 2024 The Oppia Authors. All Rights Reserved. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS-IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| /** | ||
| * @fileoverview Acceptance Test for checking if logged-in users can | ||
| * navigate using all the links under the "TEACH/LEARN" footer section. | ||
| */ | ||
|
|
||
| import {UserFactory} from '../../puppeteer-testing-utilities/user-factory'; | ||
| import {LoggedInUser} from '../../user-utilities/logged-in-users-utils'; | ||
| import testConstants from '../../puppeteer-testing-utilities/test-constants'; | ||
|
|
||
| const DEFAULT_SPEC_TIMEOUT = testConstants.DEFAULT_SPEC_TIMEOUT; | ||
|
|
||
| describe('Logged-in User', function () { | ||
| let testUser: LoggedInUser; | ||
|
|
||
| beforeAll(async function () { | ||
| testUser = await UserFactory.createNewUser( | ||
| 'testuser', | ||
| 'testuser@example.com' | ||
| ); | ||
| }, DEFAULT_SPEC_TIMEOUT); | ||
|
|
||
| beforeEach(async function () { | ||
| // Navigate to a page that has the oppia footer. | ||
| await testUser.navigateToAboutFoundationPage(); | ||
| }); | ||
|
|
||
| it( | ||
| 'should open "Get Started" page via the footer', | ||
| async function () { | ||
| await testUser.navigateToGetStartedPageViaFooter(); | ||
| }, | ||
| DEFAULT_SPEC_TIMEOUT | ||
| ); | ||
|
|
||
| it( | ||
| 'should open "Creator Guidelines" page via the footer', | ||
| async function () { | ||
| await testUser.navigateToCreatorGuidelinesPage(); | ||
| }, | ||
| DEFAULT_SPEC_TIMEOUT | ||
| ); | ||
|
|
||
| it( | ||
| 'should open "Teach" page via the footer', | ||
| async function () { | ||
| await testUser.navigateToTeachPageViaFooter(); | ||
| }, | ||
| DEFAULT_SPEC_TIMEOUT | ||
| ); | ||
|
|
||
| it( | ||
| 'should open "Community Library" page via the footer', | ||
| async function () { | ||
| await testUser.navigateToCommunityLibraryPageViaFooter(); | ||
| }, | ||
| DEFAULT_SPEC_TIMEOUT | ||
| ); | ||
|
|
||
| it( | ||
| 'should open "Contact" page via the footer', | ||
| async function () { | ||
| await testUser.navigateToContactPageViaFooter(); | ||
| }, | ||
| DEFAULT_SPEC_TIMEOUT | ||
| ); | ||
|
|
||
| afterAll(async function () { | ||
| await UserFactory.closeAllBrowsers(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| // Copyright 2024 The Oppia Authors. All Rights Reserved. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the "License"); | ||
| // you may not use this file except in compliance with the License. | ||
| // You may obtain a copy of the License at | ||
| // | ||
| // http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // Unless required by applicable law or agreed to in writing, software | ||
| // distributed under the License is distributed on an "AS-IS" BASIS, | ||
| // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| /** | ||
| * @fileoverview Acceptance Test for checking if logged-in users | ||
| * can open all the links on the "Creator Guidelines" page. | ||
| */ | ||
|
|
||
| import {UserFactory} from '../../puppeteer-testing-utilities/user-factory'; | ||
| import {LoggedInUser} from '../../user-utilities/logged-in-users-utils'; | ||
| import testConstants from '../../puppeteer-testing-utilities/test-constants'; | ||
|
|
||
| const DEFAULT_SPEC_TIMEOUT = testConstants.DEFAULT_SPEC_TIMEOUT; | ||
|
|
||
| describe('Logged-in Users', function () { | ||
| let testUser: LoggedInUser; | ||
|
|
||
| beforeAll(async function () { | ||
| testUser = await UserFactory.createNewUser( | ||
| 'testuser', | ||
| 'testuser@example.com' | ||
| ); | ||
| }, DEFAULT_SPEC_TIMEOUT); | ||
|
|
||
| it( | ||
| 'should be able to navigate to the Creator Guidelines page using the footer', | ||
|
There was a problem hiding this comment. Since you already test the "Creator Guidelines" button in the footer in the file "click-all-links-in-teach-learn-footer.spec.ts", you don't need to test it again. |
||
| async function () { | ||
| await testUser.navigateToAboutFoundationPage(); | ||
| await testUser.navigateToCreatorGuidelinesPageViaFooter(); | ||
| }, | ||
| DEFAULT_SPEC_TIMEOUT | ||
| ); | ||
|
|
||
| describe('on the Creator Guidelines page', function () { | ||
| beforeEach(async function () { | ||
| await testUser.navigateToCreatorGuidelinesPage(); | ||
|
There was a problem hiding this comment. I don't see the function navigateToCreatorGuidelinesPage in logged-in-users-utils.ts? |
||
| }, DEFAULT_SPEC_TIMEOUT); | ||
|
|
||
| it( | ||
| 'should be able to use the "forum" link', | ||
| async function () { | ||
| await testUser.clickForumLinkOnCreatorGuidelinesPage(); | ||
| }, | ||
| DEFAULT_SPEC_TIMEOUT | ||
| ); | ||
|
|
||
| it( | ||
| 'should be able to use the "Design Tips" link', | ||
| async function () { | ||
| await testUser.clickDesignTipsLinkOnCreatorGuidelinesPage(); | ||
| }, | ||
| DEFAULT_SPEC_TIMEOUT | ||
| ); | ||
|
|
||
| it( | ||
| 'should be able to use the "Create an Exploration" link', | ||
| async function () { | ||
| await testUser.clickCreateAnExplorationLinkOnCreatorGuidelinesPage(); | ||
| }, | ||
| DEFAULT_SPEC_TIMEOUT | ||
| ); | ||
|
|
||
| it( | ||
| 'should be able to use the "Browse our Expectations" link', | ||
| async function () { | ||
| await testUser.clickBrowseOurExpectationsLinkOnCreatorGuidelinesPage(); | ||
| }, | ||
| DEFAULT_SPEC_TIMEOUT | ||
| ); | ||
| }); | ||
|
|
||
| afterAll(async function () { | ||
| await UserFactory.closeAllBrowsers(); | ||
| }); | ||
| }); | ||
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 noticed that some functions' names start with "navigateTo...ViaFooter". I suggest changing it to "click(name of the button/link)Button/LinkOnFooter" and leaving "navigateTo..." only for functions to open a link directly, like using page.goto() function. Take this function as an example, it's not clear enough to tell which button you click because the name of the button is not mentioned. Also, functions like navigateToAboutFoundationPage only open the page without checking if the right page is opened. So it's different from functions that click a certain button/link and check if it opens the right page.
My suggestion would be to change the names of these functions to "click(name of the button/link)Button/LinkOnFooter" and also mention the name of the button/link in the comment. What do you think?
(not sure about adding "AndCheck" into the name yet)
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.
Renamed various functions to click.*footer