[PB-6085] feature/Added unit tests for photo upload service layer#437
[PB-6085] feature/Added unit tests for photo upload service layer#437CandelR wants to merge 2 commits intofeature/PB-6085-upload-photosfrom
Conversation
|
|
||
| const folderNames = mockCreateFolder.mock.calls.map((call) => call[1]); | ||
| const monthCalls = folderNames.filter((name) => name === '06'); | ||
| expect(monthCalls).toHaveLength(1); |
There was a problem hiding this comment.
shouldn't it also be checked expect(mockCreateFolder).toHaveBeenCalledTimes(9); (5 for May 15 and 4 for May 20)?
There was a problem hiding this comment.
We can add it, but there will be expect(mockCreateFolder).toHaveBeenCalledTimes(6), not 9. There’s a cache to prevent unnecessary calls :)
| }); | ||
|
|
||
| describe('PhotoBackupFolders.getOrCreateFolderForDate', () => { | ||
| test('when called for a date, then creates the full folder hierarchy and returns the day folder uuid', async () => { |
There was a problem hiding this comment.
Should there be a test for a date in the future? something like: does not create anything for the future date?
There was a problem hiding this comment.
The folder is created using the asset's metadata, so if it indicates a future date, it will be created based on that. It may be that the device's time is incorrect, and we shouldn't block the copying of any assets
| expect(mockSetItem).toHaveBeenCalledWith(expect.any(String), 'generated-uuid'); | ||
| }); | ||
|
|
||
| test('when no device ID is stored, then successive calls generate only one ID per call', async () => { |
There was a problem hiding this comment.
And if a stored device ID is different? Should we have a test for that too? Say the user has 2 different phones and 1 account
There was a problem hiding this comment.
then it will have 2 ids, the id is for device
| import { AssetUploadJob, PhotoUploadQueue } from './PhotoUploadQueue'; | ||
| import { PhotoUploadService } from './PhotoUploadService'; | ||
|
|
||
| jest.mock('./PhotoUploadService', () => ({ |
There was a problem hiding this comment.
btw, maybe with Vitest directly it will be easier
There was a problem hiding this comment.
I’d say that Vitests is specifically designed for Vite, so it’s not the right choice for a React Native project
| }); | ||
|
|
||
| test('when the path contains percent-encoded characters, then it decodes them', () => { | ||
| expect(stripFileScheme('file:///data/My%20Camera/photo.jpg')).toBe('/data/My Camera/photo.jpg'); |
There was a problem hiding this comment.
will it work for extension too?
expect(stripFileScheme('file:///downloads/photo.jpg%2Eexe')).toBe('/downloads/photo.jpg.exe');
There was a problem hiding this comment.
Yes, it works, and that test could be added. However, this is only used for media assets, with all that that entails in terms of file extensions
|
|
||
| test('when a filename has multiple dots, then it uses the last dot as the separator', () => { | ||
| expect(splitFileNameAndExtension('my.vacation.photo.jpg')).toEqual({ | ||
| plainName: 'my.vacation.photo', |
There was a problem hiding this comment.
will it do this too? Cause maybe we'll need to blacklist some extensions
expect(splitFileNameAndExtension('my.vacation.photo.apk')).toEqual({
plainName: 'my.vacation.photo',
fileExtension: 'apk',
});
There was a problem hiding this comment.
This feature only scans media assets. Basically, what you can see in a gallery app
|
2 participants
Covers PhotoBackupFolders caching behaviour, PhotoDeduplicator new/edited asset detection, PhotoDeviceId getOrCreate logic, PhotoUploadQueue success/error/replace flows, and PhotoUploadService URI parsing utilities.
Summary