test(web) Add tests for asset repository #680
Conversation
| expect(result.buckets.length).toEqual(2); | ||
| }); | ||
|
|
||
| it('get asset count by user id', async () => { |
There was a problem hiding this comment.
@jbaez Can you help me with some feedback on this specific test. What are other property on this specific API/Service I should have tested?
There was a problem hiding this comment.
According to the code in the service, you have 100% coverage for getAssetCountByUserId:
getAssetCountByUserId(authUser: AuthUserDto): Promise<AssetCountByUserIdResponseDto> {
return this._assetRepository.getAssetCountByUserId(authUser.id);
}Since it only accepts an authUser, I don't think there would be anything else to add. If that accepted a userId, then it would probably need to test what happens if the user doesn't exist, or you are not allowed to get the asset count for that user etc.
| // }); | ||
| expect(result.userId).toEqual(authUser.id); | ||
| expect(result.resizePath).toEqual(''); | ||
| expect(result.webpPath).toEqual(''); |
There was a problem hiding this comment.
Instead of doing these expectations on individual properties of result, it would probably be easier to do:
expect(result).toBe(assetEntity);There are 2 things we want to check here:
- the
assetEntityreturned by theassetRepositoryis returned by the service (like you have or example above) - the
assetRepositoryis called with the expected params.
For the second there would need to be an expect(createAssetDto. create).toHaveBeenCalledWith():
createAssetDto,
authUser.id,
originalPath,
mimeType,
checksum,
| const assets = _getAssets(); | ||
|
|
||
| assetRepositoryMock.getAllByDeviceId.mockImplementation(() => | ||
| Promise.resolve<string[]>(Array.from(assets.map((asset) => asset.deviceAssetId))), |
There was a problem hiding this comment.
It is tempting to use a loop to get the IDs here. In this case it doesn't seem too bad. However, the general rule is to not use loops or conditions inside the test, makes it more simple and easier to read. Hard-coding the array of strings would be preferable.
| expect(result.length).toEqual(1); | ||
| expect(result[0]).toEqual('4967046344801'); | ||
| expect(result.length).toEqual(2); | ||
| expect(result).toEqual(assets.map((asset) => asset.deviceAssetId)); |
There was a problem hiding this comment.
Same here, you could define a const assetsResult = ["with", "hardCoded", "ids"] and return in the the promise and use it here in the expect
| timeGroup: TimeGroupEnum.Month, | ||
| }); | ||
|
|
||
| expect(result.totalCount).toEqual(assetCountByTimeBucket.reduce((a, b) => a + b.count, 0)); |
There was a problem hiding this comment.
Should have a hardcoded value, no loops allowed 😃
| expect(result.buckets.length).toEqual(2); | ||
| }); | ||
|
|
||
| it('get asset count by user id', async () => { |
There was a problem hiding this comment.
According to the code in the service, you have 100% coverage for getAssetCountByUserId:
getAssetCountByUserId(authUser: AuthUserDto): Promise<AssetCountByUserIdResponseDto> {
return this._assetRepository.getAssetCountByUserId(authUser.id);
}Since it only accepts an authUser, I don't think there would be anything else to add. If that accepted a userId, then it would probably need to test what happens if the user doesn't exist, or you are not allowed to get the asset count for that user etc.
No description provided.