[#10920] Add snapshot tests for admin search page #10949

jianhandev · 2021-02-07T15:34:25Z

Outline of solution:

Added snapshot tests for AdminSearchPageComponent
Added ~~positive and~~ negative tests for openCourseJoinEmail and openFeedbackSessionReminderEmail

src/web/app/pages-admin/admin-search-page/admin-search-page.component.spec.ts

jianhandev · 2021-02-28T04:48:18Z

src/web/app/pages-admin/admin-search-page/admin-search-page.component.spec.ts

+    const spyEmailGenerationService: any = spyOn(emailGenerationService, 'getCourseJoinEmail').and.callFake(
+        (): Observable<Email> => of({
+          recipient: 'Jacky Chan',
+          subject: 'Course join email',
+          content: 'Course join email content',
+        }),
+    );
+
+    const sendButton: any = fixture.debugElement.nativeElement.querySelector('#send-course-join-button');
+    sendButton.click();
+
+    expect(spyEmailGenerationService).toBeCalled();


Technically the email object is not returned or stored in the component afterwards. So I'm wondering if I am spying on the email generation service, is it possible to retrieve the email's recipient/subject/content?

True, this indeed relies window.location.href which is not very straightforward to test...

Any suggestions? @ChooJeremy @t-cheepeng @madanalogy

@jianhandev You could try to inject a window object into the test suite using a InjectionToken and test against that.

Some references:
https://angular.io/api/core/InjectionToken
https://jasminexie.github.io/injecting-window-in-an-angular-application/

Thanks for the suggestion! Will seriously look into that 👍

Derek-Hardy · 2021-02-28T09:27:15Z

@jianhandev You seem to have missed the id in html file for the send-course-join-button. Which leads to some test failures.

jianhandev · 2021-02-28T10:21:05Z

@jianhandev You seem to have missed the id in html file for the send-course-join-button. Which leads to some test failures.

Thanks for pointing that out! Has updated with whatever that is missing.

ChooJeremy

Tests LGTM, other than the issue mentioned on email generation service that uses window.location.href. Good coverage.

madanalogy

I'm not usually particular about small unintended changes being made but there's really just too many occurrences in this PR.

madanalogy · 2021-03-04T13:34:27Z

src/web/app/pages-admin/admin-search-page/admin-search-page.component.spec.ts

      ],
      providers: [AccountService, SearchService, StatusMessageService, NgbModal],
    })
-    .compileComponents();
+        .compileComponents();


Any particular reason for this extra indent?

I think those were auto-indented by my IDE. It did managed to pass the lint test so I thought it was fine. Nevertheless, I will double check my IDE settings again for indentation. Thanks for pointing out!

madanalogy · 2021-03-04T13:34:57Z

src/web/app/pages-admin/admin-search-page/admin-search-page.component.spec.ts

-      });
+        (args: string): void => {
+          expect(args).toEqual('This is the error message');
+        });


Similarly here

madanalogy · 2021-03-04T13:35:05Z

src/web/app/pages-admin/admin-search-page/admin-search-page.component.spec.ts

-      });
+        (args: string): void => {
+          expect(args).toEqual('No results found.');
+        });


madanalogy · 2021-06-04T06:03:11Z

src/web/app/pages-admin/admin-search-page/admin-search-page.component.spec.ts

+
+  it('should generate email when send session reminder email button clicked', () => {
+    const studentResult: StudentAccountSearchResult = {
+      name: 'Jack Chan',


Could we abstract the StudentAccountSearchResult objects in this file into a factory function of sorts, and you can populate the necessary fields as parameters. Seems to me like not many fields need to change between test cases. For example,

const initStudentSearchResult = (name, email) => { return { // populate data

@madanalogy Thanks for the suggestion, made the changes 👍

madanalogy · 2021-06-04T06:07:49Z

src/web/app/pages-admin/admin-search-page/admin-search-page.component.spec.ts

-    const spyStatusMessageService: any = spyOn(statusMessageService, 'showSuccessToast').and.callFake(
-        (args: string): void => {
+    const spyStatusMessageService: any = spyOn(statusMessageService, 'showSuccessToast')
+        .and.callFake((args: string) => {


Still seems like a bunch of unnecessary changes in this file

The re-indentation was done because it exceeds the line limit, I double-checked my indentation settings in my IDE and seems to be correct also (I'm following what is specified in https://github.com/TEAMMATES/teammates/blob/master/docs/ide-setup.md).

…#10949) * Add frontend tests for admin search page * Add snapshots and add missing ids to admin search page send buttons * Standardize indentation * Refactor student result * Remove positive tests due to incompleteness Co-authored-by: Rahul Rajesh <rahul.rajesh.bhat@gmail.com>

jianhandev changed the title ~~[#10920] Add snapshot testing for admin search page~~ [#10920] Add snapshot tests for admin search page Feb 7, 2021

rrtheonlyone added the s.ToReview The PR is waiting for review(s) label Feb 7, 2021

Derek-Hardy reviewed Feb 8, 2021

View reviewed changes

src/web/app/pages-admin/admin-search-page/admin-search-page.component.spec.ts Show resolved Hide resolved

src/web/app/pages-admin/admin-search-page/admin-search-page.component.spec.ts Show resolved Hide resolved

Derek-Hardy self-assigned this Feb 8, 2021

Derek-Hardy 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 13, 2021

jianhandev closed this Feb 28, 2021

jianhandev force-pushed the 10920-admin-search-page-test branch from 27df5cf to 56d7c65 Compare February 28, 2021 04:38

Add frontend tests for admin search page

ab7cd88

jianhandev reopened this Feb 28, 2021

jianhandev commented Feb 28, 2021

View reviewed changes

jianhandev requested a review from Derek-Hardy February 28, 2021 04:48

Derek-Hardy added s.ToReview The PR is waiting for review(s) s.Ongoing The PR is being worked on by the author(s) and removed s.Ongoing The PR is being worked on by the author(s) s.ToReview The PR is waiting for review(s) labels Feb 28, 2021

Derek-Hardy requested review from t-cheepeng, ChooJeremy and madanalogy February 28, 2021 09:46

Add snapshots and add missing ids to admin search page send buttons

50dcf73

Derek-Hardy 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 28, 2021

ChooJeremy approved these changes Mar 2, 2021

View reviewed changes

madanalogy requested changes Mar 4, 2021

View reviewed changes

madanalogy 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 Mar 4, 2021

jianhandev added s.OnHold The issue/PR's validity has been put on hold pending some other event and removed s.Ongoing The PR is being worked on by the author(s) labels Mar 29, 2021

jianhandev force-pushed the 10920-admin-search-page-test branch from cc3b465 to e31dc1b Compare May 30, 2021 03:23

Merge branch 'master' into 10920-admin-search-page-test

b99f27d

jianhandev force-pushed the 10920-admin-search-page-test branch from e31dc1b to b99f27d Compare May 30, 2021 04:06

rrtheonlyone requested a review from madanalogy May 30, 2021 08:39

madanalogy requested changes Jun 4, 2021

View reviewed changes

jianhandev added 3 commits June 6, 2021 17:35

Refactor student result

c13c3c9

Remove positive tests due to incompleteness

249b724

Merge branch 'master' into 10920-admin-search-page-test

4f8e6cf

jianhandev requested a review from madanalogy June 6, 2021 10:04

jianhandev 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 Jun 6, 2021

madanalogy approved these changes Jun 6, 2021

View reviewed changes

madanalogy requested review from rrtheonlyone and removed request for t-cheepeng and Derek-Hardy June 6, 2021 11:15

madanalogy assigned ChooJeremy, rrtheonlyone and madanalogy and unassigned Derek-Hardy Jun 6, 2021

madanalogy added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Jun 6, 2021

rrtheonlyone approved these changes Jun 13, 2021

View reviewed changes

Merge branch 'master' into 10920-admin-search-page-test

161b2f6

rrtheonlyone merged commit 476193c into TEAMMATES:master Jun 13, 2021

madanalogy added this to the V8.0.0 milestone Jun 14, 2021

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 Jun 14, 2021

jianhandev deleted the 10920-admin-search-page-test branch August 4, 2023 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#10920] Add snapshot tests for admin search page #10949

[#10920] Add snapshot tests for admin search page #10949

jianhandev commented Feb 7, 2021 •

edited

jianhandev Feb 28, 2021

Derek-Hardy Feb 28, 2021

t-cheepeng Mar 2, 2021

jianhandev Mar 2, 2021

Derek-Hardy commented Feb 28, 2021 •

edited

jianhandev commented Feb 28, 2021 •

edited

ChooJeremy left a comment

madanalogy left a comment

madanalogy Mar 4, 2021

jianhandev Mar 5, 2021

madanalogy Mar 4, 2021

madanalogy Mar 4, 2021

madanalogy Jun 4, 2021

jianhandev Jun 6, 2021

madanalogy Jun 4, 2021

jianhandev Jun 6, 2021

[#10920] Add snapshot tests for admin search page #10949

[#10920] Add snapshot tests for admin search page #10949

Conversation

jianhandev commented Feb 7, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Derek-Hardy commented Feb 28, 2021 • edited

jianhandev commented Feb 28, 2021 • edited

ChooJeremy left a comment

Choose a reason for hiding this comment

madanalogy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianhandev commented Feb 7, 2021 •

edited

Derek-Hardy commented Feb 28, 2021 •

edited

jianhandev commented Feb 28, 2021 •

edited