Repository navigation
test(e2e): Cover the media storage modes - #1366
utkarshcloudinary wants to merge 2 commits into
Conversation
Add an e2e spec for the three storage settings. It checks that "Cloudinary only" keeps the local files during the delete delay and removes them after it, that "Cloudinary and WordPress" downloads the full-quality files again, and that the low resolution mode keeps a smaller local copy. In each mode the image must still load from Cloudinary. Cleanup removes the Cloudinary assets through the API. Deleting an attachment removes its asset only when the plugin thinks it is synced, and after a storage change it is not.
gabrielcld2
left a comment
gabrielcld2
left a comment
There was a problem hiding this comment.
A couple of suggestions and nitpicks but otherwise it looks good 👍
| $api->destroy( | ||
| $media->get_resource_type( $id ), | ||
| array( | ||
| 'public_id' => $media->get_public_id( $id ), |
There was a problem hiding this comment.
nitpick: Delete_Sync::delete_asset() calls get_public_id( $id, true ) to get the suffixed public ID. With the timestamped filenames a suffix is unlikely, but passing true here too would make sure we destroy the same asset the plugin would.
There was a problem hiding this comment.
Done in d8df727: it now uses get_public_id( $id, true ).
| 'exists' => file_exists( $file ), | ||
| 'bytes' => file_exists( $file ) ? filesize( $file ) : 0, | ||
| 'sizes' => $sizes, | ||
| 'storage' => (string) $media->get_post_meta( $id, Sync::META_KEYS['storage'], true ), |
There was a problem hiding this comment.
Suggestion: could storageState() also return the sync_error meta, with an assertion after each sync() that it's empty? Right now process_assets() can fail without any error the spec sees. The test would then fail later on "the delay should start" or the file checks, which doesn't point to the real cause.
There was a problem hiding this comment.
Done in d8df727. I put the check in sync() instead of storageState(): it reads the sync_error meta right after process_assets() and fails with "attachment N should sync" and the error text. So every sync is checked, and the failure points to the real cause.
| expect( | ||
| state.bytes, | ||
| 'the local file should be the low-quality version' | ||
| ).toBeLessThan( FIXTURE_BYTES ); |
There was a problem hiding this comment.
nitpick: the other two tests check the size files, but this one doesn't. Should we also check that they're generated again from the low-resolution download?
There was a problem hiding this comment.
Done in d8df727: the low resolution test now checks that every size file exists after the download.
Fail at once when a sync saves an error, check the size files in the low resolution mode, and destroy the suffixed public ID as delete sync does.
Adds an e2e spec for the three storage settings: "Cloudinary only", "Cloudinary and WordPress" and "Cloudinary and WordPress (low resolution)". No spec covered them before, and a bug here can remove the only local copy of a file.
Approach
cloudinary-storage-modes.spec.js(@serial) pins lazy load off and auto sync on, setsoffloadfor each test, and restores the settings inafterAll.Storage::DELETE_DELAY, syncs again, and checks that the main file and every size file are removed and_cloudinary_storageiscld. The image must still load from Cloudinary.process_assets(), notwp cloudinary sync. A storage change replaces or removes the local files of every synced attachment, so a library-wide sync changes media that other specs or developers left in the test site.afterAllremoves the Cloudinary assets through the API before it deletes the attachments. Delete sync removes an asset only whenSync::is_synced()is true, and after a storage change it is not, so the assets stayed in the account.QA notes
npm run test:e2e:serial -- cloudinary-storage-modesstorage-*assets, andcloudinary_sync_media.offloadis back to its value before the run.