GNOME Bugzilla – Bug 748422
thetvdb: Make it possible to hit only cache
Last modified: 2015-08-29 15:37:47 UTC
When doing lookups, it might be useful to avoid hitting the network, and only process from the cache. This would be flagged with the GRL_RESOLVE_FAST_ONLY cap.
Created attachment 302388 [details] [review] thetvdb: enable cache-only on GRL_RESOLVE_FAST_ONLY The patch includes a new unit test for cache-only.
Review of attachment 302388 [details] [review]: ::: src/thetvdb/grl-thetvdb.c @@ +1508,3 @@ os->lang = get_pref_language (GRL_THETVDB_SOURCE (source)); os->fetched_web = FALSE; + os->cache_only = (res & GRL_RESOLVE_FAST_ONLY) ? TRUE : FALSE; os->cache_only = (res & GRL_RESOLVE_FAST_ONLY); should be enough here. ::: tests/thetvdb/test_thetvdb_resolve_shows.c @@ +197,3 @@ + g_test_add_func ("/thetvdb/resolve/normal/shows", test_shows_normal); + g_test_add_func ("/thetvdb/resolve/fast-only/shows", test_shows_fast_only); How do you know it's only hitting the cache here? The best way to do this would be to: - do the lookup with an empty DB and cache only, check that it fails - do the lookup with a series name - and figure out a way to make the network lookup fail, and do the lookup again
Thanks for the review, (In reply to Bastien Nocera from comment #2) > Review of attachment 302388 [details] [review] [review]: > > ::: src/thetvdb/grl-thetvdb.c > @@ +1508,3 @@ > os->lang = get_pref_language (GRL_THETVDB_SOURCE (source)); > os->fetched_web = FALSE; > + os->cache_only = (res & GRL_RESOLVE_FAST_ONLY) ? TRUE : FALSE; > > os->cache_only = (res & GRL_RESOLVE_FAST_ONLY); > should be enough here. Sure. > > ::: tests/thetvdb/test_thetvdb_resolve_shows.c > @@ +197,3 @@ > > + g_test_add_func ("/thetvdb/resolve/normal/shows", test_shows_normal); > + g_test_add_func ("/thetvdb/resolve/fast-only/shows", > test_shows_fast_only); > > How do you know it's only hitting the cache here? > > The best way to do this would be to: > - do the lookup with an empty DB and cache only, check that it fails > - do the lookup with a series name > - and figure out a way to make the network lookup fail, and do the lookup > again Okay, I'll fix the test. - Regarding the database, is it enough to set XDG_DATA_HOME on the unit test in order to avoid using the default database?
(In reply to Victor Toso from comment #3) > Thanks for the review, > > (In reply to Bastien Nocera from comment #2) > > Review of attachment 302388 [details] [review] [review] [review]: > > > > ::: src/thetvdb/grl-thetvdb.c > > @@ +1508,3 @@ > > os->lang = get_pref_language (GRL_THETVDB_SOURCE (source)); > > os->fetched_web = FALSE; > > + os->cache_only = (res & GRL_RESOLVE_FAST_ONLY) ? TRUE : FALSE; > > > > os->cache_only = (res & GRL_RESOLVE_FAST_ONLY); > > should be enough here. > > Sure. > > > > > ::: tests/thetvdb/test_thetvdb_resolve_shows.c > > @@ +197,3 @@ > > > > + g_test_add_func ("/thetvdb/resolve/normal/shows", test_shows_normal); > > + g_test_add_func ("/thetvdb/resolve/fast-only/shows", > > test_shows_fast_only); > > > > How do you know it's only hitting the cache here? > > > > The best way to do this would be to: > > - do the lookup with an empty DB and cache only, check that it fails > > - do the lookup with a series name > > - and figure out a way to make the network lookup fail, and do the lookup > > again > > Okay, I'll fix the test. > > - Regarding the database, is it enough to set XDG_DATA_HOME on the unit test > in order to avoid using the default database? It should be enough, yes.
Created attachment 302482 [details] [review] thetvdb: enable cache-only on GRL_RESOLVE_FAST_ONLY
Created attachment 302483 [details] [review] tests: use temporary database on thetvdb tests
Created attachment 302484 [details] [review] tests: thetvdb test cache-only cache-only test with empty database and with filled database but without network
Forgot to include that this test changes depends on Grilo patch/bug: https://bugzilla.gnome.org/show_bug.cgi?id=74855
Sorry, wrong url: https://bugzilla.gnome.org/show_bug.cgi?id=748550
Review of attachment 302482 [details] [review]: Looks good.
Review of attachment 302483 [details] [review]: ::: tests/thetvdb/test_thetvdb_utils.c @@ +33,3 @@ GrlRegistry *registry; GError *error = NULL; + const gchar *tmp_dir = g_get_tmp_dir (); Use g_mkdtemp() to create a separate directory inside the temporary directory. @@ +70,3 @@ + + /* Remove database so it doesn't affect other tests */ + db_path = g_build_filename (g_get_tmp_dir (), You'll need to keep using the same temp dir as in the setup. You can make your temp XDG_DATA_HOME a global variable for that.
Review of attachment 302484 [details] [review]: ::: tests/thetvdb/test_thetvdb_resolve_shows.c @@ +24,3 @@ #include <grilo.h> +/* Returns FALSE if can't find tvdb-id; TRUE otherwise */ "Returns whether it could find tvdb-id" @@ +87,3 @@ + } + + *imdb = g_strdup (grl_data_get_string (GRL_DATA (video), imdb_key)); This should be in a separate patch. @@ +203,3 @@ +test_shows_fast_only_full_db(void) +{ + const gchar *mock = g_getenv ("GRL_NET_MOCKED"); You'll need to make a copy of it here, because it's going to get overwritten in the g_setenv() call below.
Thanks for the review! (In reply to Bastien Nocera from comment #11) > Review of attachment 302483 [details] [review] [review]: > > ::: tests/thetvdb/test_thetvdb_utils.c > @@ +33,3 @@ > GrlRegistry *registry; > GError *error = NULL; > + const gchar *tmp_dir = g_get_tmp_dir (); > > Use g_mkdtemp() to create a separate directory inside the temporary > directory. Okay! > > @@ +70,3 @@ > + > + /* Remove database so it doesn't affect other tests */ > + db_path = g_build_filename (g_get_tmp_dir (), > > You'll need to keep using the same temp dir as in the setup. You can make > your temp XDG_DATA_HOME a global variable for that. Okay! (In reply to Bastien Nocera from comment #12) > Review of attachment 302484 [details] [review] [review]: > > ::: tests/thetvdb/test_thetvdb_resolve_shows.c > @@ +24,3 @@ > #include <grilo.h> > > +/* Returns FALSE if can't find tvdb-id; TRUE otherwise */ > > "Returns whether it could find tvdb-id" Thanks! > > @@ +87,3 @@ > + } > + > + *imdb = g_strdup (grl_data_get_string (GRL_DATA (video), imdb_key)); > > This should be in a separate patch. I'll split. > > @@ +203,3 @@ > +test_shows_fast_only_full_db(void) > +{ > + const gchar *mock = g_getenv ("GRL_NET_MOCKED"); > > You'll need to make a copy of it here, because it's going to get overwritten > in the g_setenv() call below. Indeed. Thanks
Comment on attachment 302482 [details] [review] thetvdb: enable cache-only on GRL_RESOLVE_FAST_ONLY Attachment 302482 [details] pushed as eaa64e9 - thetvdb: enable cache-only on GRL_RESOLVE_FAST_ONLY
Created attachment 302742 [details] [review] tests: use temporary database on thetvdb tests
Created attachment 302743 [details] [review] tests: improve internals of thetvdb resolve shows This apply last commit changes to have each test with its own thetvdb database and also improves test by: - having resolve flag as parameter; - possibilty to fail test when expected;
Created attachment 302744 [details] [review] tests: thetvdb tests cache-only cache-only test with empty database and with filled database but without network
Review of attachment 302742 [details] [review]: Looks fine otherwise. ::: tests/thetvdb/test_thetvdb_utils.c @@ +77,3 @@ + g_remove (db_path); + g_free (db_path); + g_clear_pointer (&tmp_dir, g_free); You should probably remove the grilo-plugins subdir, and the tmp_dir with g_remove() as well.
Review of attachment 302743 [details] [review]: Sure.
Review of attachment 302744 [details] [review]: Looks good!
(In reply to Bastien Nocera from comment #18) > Review of attachment 302742 [details] [review] [review]: > > Looks fine otherwise. > > ::: tests/thetvdb/test_thetvdb_utils.c > @@ +77,3 @@ > + g_remove (db_path); > + g_free (db_path); > + g_clear_pointer (&tmp_dir, g_free); > > You should probably remove the grilo-plugins subdir, and the tmp_dir with > g_remove() as well. After running the test I have gvfs cache there as well and g_remove() only works for empty dirs. So I thought that removing the database would be enough instead of tracking everything created and removing each one of them. I'll create a rmdir using GFileEnumerator then :-)
Disable gvfs in the tests instead?
(In reply to Bastien Nocera from comment #22) > Disable gvfs in the tests instead? GIO_USE_VFS=0 as an envvar IIRC.
GIO_USE_VFS=local worked, thanks
Comment on attachment 302742 [details] [review] tests: use temporary database on thetvdb tests Attachment 302742 [details] pushed as b4cf71a - tests: use temporary database on thetvdb tests
Created attachment 310219 [details] [review] tests: new function to reset test environment With commit b4cf71a18de08825006ed7e7c3b8545e736b1839 we are using temporary database for thetvdb tests. The goal of this patch is a way to reset test environment by removing the database and re-loading the plugin. If a test need a clean environment, it can call test_reset_thetvdb ().
Created attachment 310220 [details] [review] tests: improve internals of thetvdb resolve shows This apply last commit changes to have each test with its own thetvdb database and also improves test by: - having resolve flag as parameter; - possibilty to fail test when expected;
Created attachment 310221 [details] [review] tests: thetvdb tests cache-only cache-only test with empty database and with filled database but without network
Review of attachment 310219 [details] [review]: Looks fine.
Review of attachment 310220 [details] [review]: Sure.
Review of attachment 310221 [details] [review]: Looks good.
Attachment 310219 [details] pushed as ef77cb7 - tests: new function to reset test environment Attachment 310220 [details] pushed as 9169323 - tests: improve internals of thetvdb resolve shows Attachment 310221 [details] pushed as 9b67d69 - tests: thetvdb tests cache-only