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

Create tests for ErrorMessage, ErrorHandling and Milestone services #290

Closed
wants to merge 11 commits into from
22 changes: 22 additions & 0 deletions tests/constants/error.constants.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { HttpErrorResponse } from '@angular/common/http';
import { RequestError } from '@octokit/request-error';

export const STANDARD_ERROR = new Error('This is a normal error');

export const ERROR_WITH_NO_MESSAGE = new Error();

export const HTTP_304_ERROR = new HttpErrorResponse({ status: 304 });

export const HTTP_422_ERROR = new HttpErrorResponse({ status: 422 });

export const HTTP_500_ERROR = new HttpErrorResponse({ status: 500 });

export const HTTP_400_ERROR = new HttpErrorResponse({ status: 400 });

export const HTTP_401_ERROR = new HttpErrorResponse({ status: 401 });

export const HTTP_404_ERROR = new HttpErrorResponse({ status: 404 });

export const OCTOKIT_REQUEST_ERROR = new RequestError('This is an octokit request error', 400, {
request: { method: 'GET', url: '', headers: {} }
});
74 changes: 74 additions & 0 deletions tests/services/error-handling.service.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
import { ErrorHandlingService } from '../../src/app/core/services/error-handling.service';
import { FormErrorComponent } from '../../src/app/shared/error-toasters/form-error/form-error.component';
import { GeneralMessageErrorComponent } from '../../src/app/shared/error-toasters/general-message-error/general-message-error.component';
import {
ERROR_WITH_NO_MESSAGE,
HTTP_304_ERROR,
HTTP_400_ERROR,
HTTP_401_ERROR,
HTTP_404_ERROR,
HTTP_422_ERROR,
HTTP_500_ERROR,
OCTOKIT_REQUEST_ERROR,
STANDARD_ERROR
} from '../constants/error.constants';

let errorHandlingService: ErrorHandlingService;
let mockLoggingService;
let mockSnackBar;

describe('ErrorHandlingService', () => {
beforeEach(() => {
mockLoggingService = jasmine.createSpyObj('LoggingService', ['error', 'debug']);
mockSnackBar = jasmine.createSpyObj('MatSnackBar', ['openFromComponent']);
errorHandlingService = new ErrorHandlingService(mockSnackBar, mockLoggingService);
});

describe('ErrorHandlingService: handleError()', () => {
it('should log errors when handling errors', () => {
errorHandlingService.handleError(STANDARD_ERROR);
expect(mockLoggingService.error).toHaveBeenCalledWith(STANDARD_ERROR);
});

it('should use the GeneralMessageErrorComponent when handling Errors', () => {
errorHandlingService.handleError(STANDARD_ERROR);
expect(mockSnackBar.openFromComponent).toHaveBeenCalledWith(GeneralMessageErrorComponent, {
data: { message: STANDARD_ERROR.message }
});
});

it('should stringify Errors if there is no message before displaying', () => {
errorHandlingService.handleError(ERROR_WITH_NO_MESSAGE);
expect(mockSnackBar.openFromComponent).toHaveBeenCalledWith(GeneralMessageErrorComponent, {
data: { message: JSON.stringify(ERROR_WITH_NO_MESSAGE) }
});
});

it('should not open the snackbar when handling http status 304 errors', () => {
errorHandlingService.handleError(HTTP_304_ERROR);
expect(mockSnackBar.openFromComponent).not.toHaveBeenCalled();
});

it('should use the FormErrorComponent when handling http status 422 errors', () => {
errorHandlingService.handleError(HTTP_422_ERROR);
expect(mockSnackBar.openFromComponent).toHaveBeenCalledWith(FormErrorComponent, { data: HTTP_422_ERROR });
});

it('should use the GeneralMessageErrorComponent when handling other http errors', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to test another HttpError to test the default case.

errorHandlingService.handleError(HTTP_500_ERROR);
expect(mockSnackBar.openFromComponent).toHaveBeenCalledWith(GeneralMessageErrorComponent, { data: HTTP_500_ERROR });
errorHandlingService.handleError(HTTP_400_ERROR);
expect(mockSnackBar.openFromComponent).toHaveBeenCalledWith(GeneralMessageErrorComponent, { data: HTTP_400_ERROR });
errorHandlingService.handleError(HTTP_401_ERROR);
expect(mockSnackBar.openFromComponent).toHaveBeenCalledWith(GeneralMessageErrorComponent, { data: HTTP_401_ERROR });
errorHandlingService.handleError(HTTP_404_ERROR);
expect(mockSnackBar.openFromComponent).toHaveBeenCalledWith(GeneralMessageErrorComponent, { data: HTTP_404_ERROR });
expect(mockSnackBar.openFromComponent).toHaveBeenCalledTimes(4);
});

it('should treat octokit request errors as http errors', () => {
errorHandlingService.handleError(OCTOKIT_REQUEST_ERROR);
expect(mockSnackBar.openFromComponent).toHaveBeenCalledWith(GeneralMessageErrorComponent, { data: OCTOKIT_REQUEST_ERROR });
});
});
});
117 changes: 117 additions & 0 deletions tests/services/error-message.service.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
import { ErrorMessageService } from '../../src/app/core/services/error-message.service';

let errorMessageService: ErrorMessageService;

describe('ErrorMessageService', () => {
beforeEach(() => {
errorMessageService = new ErrorMessageService();
});

describe('ErrorMessageService: repositoryNotPresentMessage()', () => {
it('should return the correct message', () => {
expect(ErrorMessageService.repositoryNotPresentMessage()).toBe(
'Invalid repository name. Please provide Github repository URL or the repository name in the format Org/Repository Name.'
);
});
});

describe('ErrorMessageService: invalidUrlMessage()', () => {
it('should return the correct message', () => {
expect(ErrorMessageService.invalidUrlMessage()).toBe(
'URL is invalid, or repository does not exist, please indicate the repository you wish to view to continue.'
);
});
});

describe('ErrorMessageService: unableToFetchIssuesMessage()', () => {
it('should return the correct message', () => {
expect(ErrorMessageService.unableToFetchIssuesMessage()).toBe('Failed to fetch issue.');
});
});

describe('ErrorMessageService: usesrsUnassignableMessage()', () => {
it('should return the correct message', () => {
expect(ErrorMessageService.usersUnassignableMessage('test')).toBe(
'Cannot assign test to the issue. Please check if test is authorized.'
);
});
});

describe('ErrorMessageService: unableToFetchMilestoneMessage()', () => {
it('should return the correct message', () => {
expect(ErrorMessageService.unableToFetchMilestoneMessage()).toBe('Failed to fetch milestones.');
});
});

describe('ErrorMessageService: unableToFetchLabelsMessage()', () => {
it('should return the correct message', () => {
expect(ErrorMessageService.unableToFetchLabelsMessage()).toBe('Failed to fetch labels.');
});
});

describe('ErrorMessageService: unableToFetchUsersMessage()', () => {
it('should return the correct message', () => {
expect(ErrorMessageService.unableToFetchUsersMessage()).toBe('Failed to fetch assignable users for repository');
});
});

describe('ErrorMessageService: unableToFetchDataFileMessage()', () => {
it('should return the correct message', () => {
expect(ErrorMessageService.unableToFetchDataFileMessage()).toBe('Failed to fetch data file.');
});
});

describe('ErrorMessageService: unableToFetchEventsMessage()', () => {
it('should return the correct message', () => {
expect(ErrorMessageService.unableToFetchEventsMessage()).toBe('Failed to fetch issue events for repository');
});
});

describe('ErrorMessageService: unableToFetchActivityEventsMessage()', () => {
it('should return the correct message', () => {
expect(ErrorMessageService.unableToFetchActivityEventsMessage()).toBe('Failed to fetch activity events for repository');
});
});

describe('ErrorMessageService: unableToFetchLatestRelease()', () => {
it('should return the correct message', () => {
expect(ErrorMessageService.unableToFetchLatestReleaseMessage()).toBe('Failed to fetch latest release.');
});
});

describe('ErrorMessageService: unableToFetchSettingsFileMessage()', () => {
it('should return the correct message', () => {
expect(ErrorMessageService.unableToFetchSettingsFileMessage()).toBe('Failed to fetch settings file.');
});
});

describe('ErrorMessageService: unableToFetchAuthenticatedUsersMessage()', () => {
it('should return the correct message', () => {
expect(ErrorMessageService.unableToFetchAuthenticatedUsersMessage()).toBe('Failed to fetch authenticated user.');
});
});

describe('ErrorMessageService: unableToOpenInBrowserMessage()', () => {
it('should return the correct message', () => {
expect(ErrorMessageService.unableToOpenInBrowserMessage()).toBe('Unable to open this issue in Browser');
});
});

describe('ErrorMessageService: applicationVersionOutdatedMessage()', () => {
it('should return the correct message', () => {
expect(ErrorMessageService.applicationVersionOutdatedMessage()).toBe('Please update to the latest version of WATcher.');
});
});

describe('ErrorMessageService: multipleDropdownOptionsErrorMessage()', () => {
it('should return the correct message', () => {
expect(ErrorMessageService.multipleDropdownOptionsErrorMessage()).toBe('Supply either Dropdown option number or text, not both.');
});
});

describe('ErrorMessageService: noDropdownOptionsErrorMessage()', () => {
it('should return the correct message', () => {
expect(ErrorMessageService.noDropdownOptionsErrorMessage()).toBe('No Dropdown identification parameters supplied.');
});
});
Comment on lines +5 to +116
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if there's any benefit to write test cases for the constant provider service. If test cases fail due to future changes in constant values, developers will likely just edit the test cases, resulting in duplicated changes. Also, I think that integration tests for services using these constants should suffice to catch any errors in those constant values.
@CATcher-org/2324s2 what do yall think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I'm in agreement with you, don't think we need tests for this.

});
52 changes: 52 additions & 0 deletions tests/services/milestone.service.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { of } from 'rxjs';
import { Milestone } from '../../src/app/core/models/milestone.model';
import { GithubService } from '../../src/app/core/services/github.service';
import { MilestoneService } from '../../src/app/core/services/milestone.service';

let milestoneService: MilestoneService;
let githubServiceSpy: jasmine.SpyObj<GithubService>;

describe('MilestoneService', () => {
beforeEach(() => {
githubServiceSpy = jasmine.createSpyObj('GithubService', ['fetchAllMilestones']);
milestoneService = new MilestoneService(githubServiceSpy);
});

describe('MilestoneService: fetchMilestones()', () => {
it('should fetch all milestones', async () => {
const mockMilestones = [{ title: 'Milestone 1' }, { title: 'Milestone 2' }];
githubServiceSpy.fetchAllMilestones.and.returnValue(of(mockMilestones));
milestoneService.fetchMilestones().subscribe((response) => {
expect(githubServiceSpy.fetchAllMilestones).toHaveBeenCalled();
expect(response).toEqual(mockMilestones);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this line since you're already setting the return value ofr fetchAllMilestones as mockMilestones three lines above?

expect(milestoneService.milestones.length).toBe(3);
expect(milestoneService.milestones[0].title).toBe('Milestone 1');
expect(milestoneService.hasNoMilestones).toBeFalse();
});
Comment on lines +16 to +25
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using Jasmine's done() callback at the end of the assertions. This is to ensure that the test doesn't complete before the value is emitted, which would lead to the assertions not being executed by Jasmine.

The same issue applies to the test case below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into this, and received an Error Log.

Screenshot 2024-03-15 at 2 11 33 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

If I am not wrong, async/await cannot be used with done. You try remove the async keyword.

});

it('should handle no milestones', async () => {
githubServiceSpy.fetchAllMilestones.and.returnValue(of([]));
milestoneService.fetchMilestones().subscribe((response) => {
expect(githubServiceSpy.fetchAllMilestones).toHaveBeenCalled();
expect(milestoneService.milestones.length).toBe(1);
expect(milestoneService.hasNoMilestones).toBeTrue();
});
});
});

describe('MilestoneService: parseMilestoneData()', () => {
it('should parse milestone data correctly', () => {
const mockMilestones = [{ title: 'Milestone 2' }, { title: 'Milestone 1' }];
const parsedMilestones = milestoneService.parseMilestoneData(mockMilestones);

for (const milestone of parsedMilestones) {
expect(milestone instanceof Milestone).toBeTrue();
}

expect(parsedMilestones.length).toBe(3);
expect(parsedMilestones[0].title).toBe('Milestone 1');
expect(parsedMilestones[parsedMilestones.length - 1]).toBe(Milestone.DefaultMilestone);
});
});
});
Loading