Removed FeedImage #2726
Removed FeedImage #2726
Conversation
| import static org.junit.Assert.assertFalse; | ||
| import static org.junit.Assert.assertTrue; | ||
|
|
||
| public class FeedTest { |
There was a problem hiding this comment.
actually, I think we should keep these tests
| import static org.junit.Assert.assertEquals; | ||
| import static org.junit.Assert.assertTrue; | ||
|
|
||
| public class FeedItemTest { |
| + " WHERE " + PodDBAdapter.TABLE_NAME_FEED_IMAGES + "." + PodDBAdapter.KEY_ID | ||
| + " = " + PodDBAdapter.TABLE_NAME_FEEDS + "." + PodDBAdapter.KEY_IMAGE + ")"); | ||
|
|
||
| db.execSQL("DROP TABLE " + PodDBAdapter.TABLE_NAME_FEED_IMAGES); |
There was a problem hiding this comment.
Do we intend to shrink the database file with the upgrade?
Using sqlite3 command line on windows, I noticed the db file is shrinked after DROP TABLE is done, as it VACCUM has been done. But I am unclear if the same behavior applies to Android environment.
There was a problem hiding this comment.
VACUUM is a quite dangerous operation. Mfietz and I decided that we do not want to execute that currently. In my case, the database shrinked significantly even without VACUUM
There was a problem hiding this comment.
Good to hear - my main concern is the issue should involve a deliberate decision, which is the case here.
There was a problem hiding this comment.
For testing loading images with authentication: it might be another PR, but it'd be better if a test can be included with no external server dependency, e.g., add a test for de.danoeh.antennapod.core.glide.ApOkHttpUrlLoader.BasicAuthenticationInterceptor
The existing HttpDownloaderTest.java uses an embedded HTTP server to avoid external dependency.
| + " INNER JOIN " + TABLE_NAME_FEEDS | ||
| + " ON " + TABLE_NAME_FEED_ITEMS + "." + KEY_FEED + "=" + TABLE_NAME_FEEDS + "." + KEY_ID | ||
| + " WHERE " + TABLE_NAME_FEED_IMAGES + "." + KEY_DOWNLOAD_URL + "=" + downloadUrl; | ||
| + " ON " + TABLE_NAME_FEED_ITEMS + "." + KEY_FEED + " = " + TABLE_NAME_FEEDS + "." + KEY_ID |
There was a problem hiding this comment.
Regression Image authentication: the logic here supports images of FeedItem, but not the images of Feed.
There was a problem hiding this comment.
Nice catch. I used the same icon for feed and item in my tests, so I did not notice.
but it'd be better if a test can be included with no external server dependency
Oh, I did not mean mean my server to be used for unit tests. Just for manual testing ;)
| null, "http://example.com/feed", true); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Thanks for restoring. PR #2714 include tests that depend on FeedMother .
|
My main database size decreased from 20 to 15 MB. |
Please note that due to the database updates, you will not be able to downgrade from this version.
For testing, I have set up a password protected feed: http://www.tools.bytehamster.com/podcast/rss.xml
Username:
user, password:passwordCloses #2725