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.

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

[#12588] Added unit tests for SessionEditFormComponent #12627

Merged
merged 12 commits into from
Dec 5, 2023

Conversation

kenneySiu
Copy link
Contributor

Part of #12588

Outline of Solution
Added unit tests for the following methods in SessionEditFormComponent
triggerModelChange,
triggerSubmissionOpeningDateModelChange,
courseIdChangeHandler,
submitFormHandler,
cancelHandler,
deleteHandler,
copyHandler,
copyOthersHandler,
closeEditFormHandler

Copy link
Contributor

@cedricongjh cedricongjh left a comment

Choose a reason for hiding this comment

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

some changes,

also minTimeForSubmissionEnd, minDateForSessionVisible, minTimeForSessionVisible, maxDateForSessionVisible, maxTimeForSessionVisible, minDateForResponseVisible, minTimeForResponseVisible are currently not tested as well, do add tests for those

Comment on lines 133 to 147
it('should emit a cancelEditingSession event after modal confimation', async () => {
jest.spyOn((component as any).simpleModalService, 'openConfirmationModal').mockReturnValue(mockModal);
const cancelEditingSessionSpy = jest.spyOn(component.cancelEditingSessionEvent, 'emit');
component.cancelHandler();
await mockModal.result;
expect(cancelEditingSessionSpy).toHaveBeenCalled();
});

it('should emit a deleteExistingSession event after modal confimation', async () => {
jest.spyOn((component as any).simpleModalService, 'openConfirmationModal').mockReturnValue(mockModal);
const deleteExistingSessionSpy = jest.spyOn(component.deleteExistingSessionEvent, 'emit');
component.deleteHandler();
await mockModal.result;
expect(deleteExistingSessionSpy).toHaveBeenCalled();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

for these methods, we should be creating a modalSpy and checking if it's called with the appropriate params, you can see admin-notifications-page.component.spec.ts for an example

@kenneySiu
Copy link
Contributor Author

Added additional tests
Ready for review

Copy link
Contributor

@cedricongjh cedricongjh left a comment

Choose a reason for hiding this comment

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

thanks for adding the new tests, left a few comments!

import { SessionEditFormComponent } from './session-edit-form.component';
import { SessionEditFormModule } from './session-edit-form.module';

describe('SessionEditFormComponent', () => {
let component: SessionEditFormComponent;
let fixture: ComponentFixture<SessionEditFormComponent>;
let simpleModalService: SimpleModalService;
let service: DateTimeService;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: let's rename this dateTimeService for better readability

component.model.submissionStartDate = date;
component.model.submissionStartTime = time;
const minTimeForSubmissionEnd = component.minTimeForSubmissionEnd;
expect(minTimeForSubmissionEnd).toStrictEqual(time);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: let's just use component.minTimeForSubmissionEnd here instead of the const

component.model.submissionStartDate = date;
component.model.submissionStartTime = time;
const minTimeForSubmissionEnd = component.minTimeForSubmissionEnd;
expect(minTimeForSubmissionEnd).toStrictEqual(oneHourBeforeNow);
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, use component.minTimeForSubmissionEnd directly, do make the changes for all the others below

+ 'when response visible setting is CUSTOM and submissionStartDate is before customResponseVisibleDate', () => {
component.model.responseVisibleSetting = ResponseVisibleSetting.CUSTOM;
component.model.submissionStartDate =
service.getDateInstance(moment().tz(component.model.timeZone));
Copy link
Contributor

Choose a reason for hiding this comment

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

let's save moment().tz(component.model.timeZone) into a const here, as unlikely as it may be, there might be a case where the date changes while the test runs and thus it might end up as different days

+ 'when response visible setting is CUSTOM and submissionStartDate is after customResponseVisibleDate', () => {
component.model.responseVisibleSetting = ResponseVisibleSetting.CUSTOM;
component.model.submissionStartDate =
service.getDateInstance(moment().tz(component.model.timeZone).add(1, 'days'));
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above here

Comment on lines 240 to 248
component.model.responseVisibleSetting = ResponseVisibleSetting.CUSTOM;
component.model.submissionStartTime =
service.getTimeInstance(moment());
component.model.customResponseVisibleTime =
service.getTimeInstance(moment());
component.model.submissionStartDate =
service.getDateInstance(moment().tz(component.model.timeZone));
component.model.customResponseVisibleDate =
service.getDateInstance(moment().tz(component.model.timeZone).add(1, 'days'));
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, let's save all the moment() calls into a const instead

Comment on lines 256 to 264
component.model.submissionStartTime =
service.getTimeInstance(moment());
component.model.customResponseVisibleTime =
service.getTimeInstance(moment());
component.model.submissionStartDate =
service.getDateInstance(moment().tz(component.model.timeZone).add(1, 'days'));
component.model.customResponseVisibleDate =
service.getDateInstance(moment().tz(component.model.timeZone));
const maxTimeForSessionVisible = component.maxTimeForSessionVisible;
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above here

Comment on lines 286 to 288
service.getDateInstance(moment().tz(component.model.timeZone));
component.model.customSessionVisibleDate =
service.getDateInstance(moment().tz(component.model.timeZone));
Copy link
Contributor

Choose a reason for hiding this comment

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

we should make customerSessionVisibleDate different from submissionStartDate here

Comment on lines 311 to 313
service.getTimeInstance(moment().tz(component.model.timeZone));
component.model.customSessionVisibleTime =
service.getTimeInstance(moment().tz(component.model.timeZone));
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, let's make the customSessionVisibleTime different from submissionStartTime

@kenneySiu
Copy link
Contributor Author

Ready for review

Copy link
Contributor

@cedricongjh cedricongjh left a comment

Choose a reason for hiding this comment

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

LGTM! thank you for your work on this!

@nusoss-bot
Copy link

Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢
Hope someone can get it to move forward again soon...

1 similar comment
@nusoss-bot
Copy link

Folks, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢
Hope someone can get it to move forward again soon...

@nusoss-bot
Copy link

Folks, This PR seems to be stalling (no activities for the past 11 days). 🐌 😢
Hope someone can get it to move forward again soon...

Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

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

LGTM

@weiquu weiquu merged commit 77631b1 into TEAMMATES:master Dec 5, 2023
9 checks passed
@wkurniawan07 wkurniawan07 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging c.Task Other non-user-facing works, e.g. refactoring, adding tests and removed s.FinalReview The PR is ready for final review labels Jan 21, 2024
@wkurniawan07 wkurniawan07 added this to the V8.30.0 milestone Jan 21, 2024
cedricongjh pushed a commit to cedricongjh/teammates that referenced this pull request Feb 20, 2024
…MATES#12627)

* Added unit tests for session-edit-form-component

* Added unit tests for session-edit-form-component

* added changes to sessioneditformcomponent tests

* fixed indent on seesioneditformcomponent tests

* moved test in sessioneditformcomponent

* made changes to session edit form tests

* fixed issues on session edit form component

---------

Co-authored-by: Wei Qing <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

5 participants