Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[#10920] Add front-end tests for instructor courses page #10967

Merged
merged 3 commits into from
Feb 25, 2021
Merged

[#10920] Add front-end tests for instructor courses page #10967

merged 3 commits into from
Feb 25, 2021

Conversation

halfwhole
Copy link
Contributor

Part of #10920

Outline of Solution

  • Added API test for loading courses (active, archived, and soft deleted)
  • Added API test for getting course statistics
  • Added tests for disabling front-end buttons when instructor has no modification permissions
  • Added tests for sorting by column

@halfwhole halfwhole added the s.ToReview The PR is waiting for review(s) label Feb 15, 2021
t-cheepeng
t-cheepeng reviewed Feb 16, 2021
View reviewed changes
Copy link
Contributor

@t-cheepeng t-cheepeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great Work!

Let's add some more tests

  • Snapshot tests for the boolean variables which affect the layout of the page
  • Unit tests to ensure that service methods are called properly with proper params

@halfwhole halfwhole added s.Ongoing The PR is being worked on by the author(s) and removed s.ToReview The PR is waiting for review(s) labels Feb 16, 2021
@halfwhole
Copy link
Contributor Author

Added more tests here:

  • Added unit tests for archiving/unarchiving/soft deleting/permanent deleting
  • Added testing for service methods being called with correct params
  • Added snapshot tests for two more boolean variables (isAddNewCourseFormExpanded and isArchivedCourseExpanded), other boolean variables should already be covered by previous snapshot tests

@halfwhole halfwhole added s.ToReview The PR is waiting for review(s) and removed s.Ongoing The PR is being worked on by the author(s) labels Feb 16, 2021
t-cheepeng
t-cheepeng approved these changes Feb 22, 2021
View reviewed changes
Copy link
Contributor

@t-cheepeng t-cheepeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@t-cheepeng t-cheepeng added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Feb 22, 2021
ChooJeremy
ChooJeremy reviewed Feb 23, 2021
View reviewed changes
src/web/app/pages-instructor/instructor-courses-page/instructor-courses-page.component.ts
@@ -292,10 +292,10 @@ export class InstructorCoursesPageComponent implements OnInit {
/**
* Permanently deletes a soft-deleted course in Recycle Bin.
*/
onDeletePermanently(courseId: string): void {
onDeletePermanently(courseId: string): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the goal of returning Promise here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The promise helps us to know if the function has finished deleting the course (because the ngbmodal result itself is a promise) -- otherwise, it would be hard for the tests to know when the async operations are done before testing for the desired changes. There doesn't seem to be any other way to do it that's as straightforward as using promises, so I went with this approach

ChooJeremy
ChooJeremy approved these changes Feb 25, 2021
View reviewed changes
Copy link
Contributor

@ChooJeremy ChooJeremy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@madanalogy madanalogy added c.Task Other non-user-facing works, e.g. refactoring, adding tests s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Feb 25, 2021
@madanalogy madanalogy added this to the V7.11.0 milestone Feb 25, 2021
@madanalogy madanalogy merged commit 3c3a3b6 into TEAMMATES:master Feb 25, 2021
@halfwhole halfwhole deleted the 10920-frontend-test-courses-page branch February 26, 2021 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChooJeremy ChooJeremy ChooJeremy approved these changes

@t-cheepeng t-cheepeng t-cheepeng approved these changes

Assignees

@ChooJeremy ChooJeremy

@t-cheepeng t-cheepeng

Labels
c.Task Other non-user-facing works, e.g. refactoring, adding tests s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Milestone
V7.11.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@halfwhole @ChooJeremy @t-cheepeng @madanalogy