GNOME Bugzilla – Bug 755447
Lua-Factory tests
Last modified: 2018-09-24 09:36:38 UTC
We need tests to verify if sources written in lua behaves as sources written in c; We also need tests to check api for lua sources, such as grl.unzip and grl.lua.json.string_to_table() and hopefully more in the future :) This initial patch helps verify problems with metadata-keys and it *should* check if the media is the correct one (audio, video, image, box) It is WIP (but works already)
Created attachment 311916 [details] [review] tests: lua-factory check media from lua sources Check if GrlMedia is GrlMediaBox, GrlMediaAudio, GrlMediaVideo, GrlMediaImage or just GrlMedia. This also do a basic test for all metadata-keys
Review of attachment 311916 [details] [review]: Looks fine otherwise. ::: tests/lua-factory/test_lua_factory_grl_media.c @@ +190,3 @@ + media = get_media (source, media); + g_assert_nonnull (media); + /* FIXME: This actually does not work. How to verify from GrlMedia if G_TYPE_CHECK_CLASS_TYPE? @@ +261,3 @@ + g_test_init (&argc, &argv, NULL); + +#if !GLIB_CHECK_VERSION(2,32,0) That's not needed anymore.
Created attachment 312071 [details] [review] tests: lua-factory check media from lua sources Check if GrlMedia is GrlMediaBox, GrlMediaAudio, GrlMediaVideo, GrlMediaImage or just GrlMedia. This also do a basic test for all metadata-keys Hey, Thanks for taking a look on this. I've fixed what you suggested about this patch. This is still a WIP with some changes to be done and improved... it is already providing some fixes, gladly :) Please, take a look at the json file, I'm sure we could think of ways to test/break lua-factory by tweaking with values. I've stopped at the moment with multiple-values (which I forgot about in the first interaction). This is the case of genre metadata key and the test will break with: ERROR:test_lua_factory_grl_media.c:81:check_metadata: assertion failed (from_json == from_media): (NULL == "Hard rock")
Created attachment 312072 [details] [review] lua-factory: get correct typename on warnings lua_typename (L, -1) is wrong and returns 'no value'. The correct way is using lua_type(L, -1) as the second parameter.
Created attachment 312073 [details] [review] lua-factory: fix types in lua-json If we lua_pushnumber instead of lua_pushboolean, the result would be an integer instead of boolean. When this is converted to media keys, it fails with: grl-lua-library.c:394: 'number' is not compatible for 'favourite'
Created attachment 312074 [details] [review] lua-factory: fix use of wrong grl_data_set_int commit 1e20497737db94c0932a12b4395859 changed the behavior and current tests got it. grl_data_set_int64 should be used with G_TYPE_INT64 or else we would fail with: data/grl-related-keys.c:253: value has type gint, but expected gint64
Regarding reading the json in the test to compare with the media provided by lua-factory, it might be worth to implement it in the core already: https://bugzilla.gnome.org/show_bug.cgi?id=755448
Review of attachment 312071 [details] [review]: ::: tests/lua-factory/data/grl-fake-source.lua @@ +30,3 @@ + description = "a source to test GrlMedia", + supported_keys = { "title" }, + supported_media = { 'audio', 'video' }, 'image' as well? ::: tests/lua-factory/test_lua_factory_grl_media.c @@ +207,3 @@ + g_object_unref (file); + + resolve_fake_src (GRL_TYPE_MEDIA, input); This is pretty gross, stuffing the full json file inside the ID. I would make it load the file through a mocked network instead, much cleaner, and closer to what it does in reality.
Review of attachment 312072 [details] [review]: Really? That used to work :(
Review of attachment 312073 [details] [review]: Yes!
Review of attachment 312074 [details] [review]: Looks fine otherwise. ::: src/lua-factory/grl-lua-library.c @@ +354,3 @@ gint value = lua_tointegerx (L, -1, &success); if (success) + (type == G_TYPE_INT) ? Ugh, multi-line ternary. Use a real if() please.
(In reply to Bastien Nocera from comment #8) > Review of attachment 312071 [details] [review] [review]: > > ::: tests/lua-factory/data/grl-fake-source.lua > @@ +30,3 @@ > + description = "a source to test GrlMedia", > + supported_keys = { "title" }, > + supported_media = { 'audio', 'video' }, > > 'image' as well? it should have, yes. Actually I will remove the media-type check. The media-type should not be changed by plugins as explained by Juan at IRC. > > ::: tests/lua-factory/test_lua_factory_grl_media.c > @@ +207,3 @@ > + g_object_unref (file); > + > + resolve_fake_src (GRL_TYPE_MEDIA, input); > > This is pretty gross, stuffing the full json file inside the ID. I would > make it load the file through a mocked network instead, much cleaner, and > closer to what it does in reality. I agree :) I don't know why I thought it was a good idea at the time. Fixing...
Created attachment 312122 [details] [review] lua-factory: fix use of wrong grl_data_set_int commit 1e20497737db94c0932a12b4395859 changed the behavior and current tests got it. grl_data_set_int64 should be used with G_TYPE_INT64 or else we would fail with: data/grl-related-keys.c:253: value has type gint, but expected gint64 change: no multiple lines ternary!
Attachment 312072 [details] pushed as 42717c0 - lua-factory: get correct typename on warnings Attachment 312073 [details] pushed as 7c7d4d8 - lua-factory: fix types in lua-json
Review of attachment 312122 [details] [review]: Yes.
Comment on attachment 312122 [details] [review] lua-factory: fix use of wrong grl_data_set_int Attachment 312122 [details] pushed as 8cc02f2 - lua-factory: fix use of wrong grl_data_set_int
Created attachment 312639 [details] [review] tests: lua-factory check media from lua sources Check if GrlMedia is GrlMediaBox, GrlMediaAudio, GrlMediaVideo, GrlMediaImage or just GrlMedia. This also do a basic test for all metadata-keys
Created attachment 312640 [details] [review] tests: lua-factory's xml-parser
Review of attachment 312640 [details] [review]: ::: tests/lua-factory/data/test-source-xml-parser.lua @@ +73,3 @@ + if xml and xml.movieinfo then + for i, t in ipairs(xml.movieinfo) do + print ("[" .. i .. "] is " .. t.id) All this debug is not necessary, is it? ::: tests/lua-factory/data/xml-parser-test-apple-trailers.xml @@ +1,1 @@ +<?xml version="1.0" encoding="utf-8"?> There are better tests (with expected output) in the lubyx XML docs. One small one: http://doc.lubyk.org/xml.html#Lua-table-format And some bigger ones under: https://github.com/lubyk/xml/blob/master/test/ ::: tests/lua-factory/test_lua_factory_xml_parser.c @@ +28,3 @@ +#define TEST_URL_XML_PARSER "http://xml.parser.test/lua-factory/" + +#define JSON_FILE_MEDIA_ALL_METADATA_KEYS GRESOURCE_PREFIX "sample.xml" Unused. @@ +29,3 @@ + +#define JSON_FILE_MEDIA_ALL_METADATA_KEYS GRESOURCE_PREFIX "sample.xml" +#define JSON_STR_BASIC_MEDIA "{ 'type' : '%s', 'title' : '%s' }" This isn't used, is it? @@ +94,3 @@ + +static void +resolve_fake_src (const gchar *input, You don't use the input. @@ +119,3 @@ + gchar *url; + } xml_tests[] = { + { GRESOURCE_PREFIX "xml-parser-test-simple.xml", Again, this needs to pass through a mocked network.
Review of attachment 312639 [details] [review]: Looks fine otherwise. ::: tests/lua-factory/test_lua_factory_grl_media.c @@ +113,3 @@ + + default: + if (type == G_TYPE_DATE_TIME) { Add the same explanation we have in the lua-factory plugin as to why it's not a switch case. @@ +226,3 @@ + gchar *url; + } media_tests[] = { + { GRESOURCE_PREFIX "grl-media-test-all-metadata.json", This test will really need some explanations.
Created attachment 312856 [details] [review] tests: lua-factory check media from lua sources Introducing tests to the lua-factory plugin and its API. The test-lua-factory-grl-media has the goal to check if the data provided by lua-library is the same that the lua-source received. This is done by 'fake' lua-source provided for this test. -- changes: -> Addressing Bastien's comments -> Fixed multiple values for genre (string type) -> Included a TODO for future tests.
Created attachment 312857 [details] [review] lua-factory: double check optional argument in fetch Strange error on xml-parser tests when fetching mocked content. The third argument was recognized as _G which is lua's table for its environment. Best thing to do is check number of arguments with lua_gettop instead of relying on lua_istable.
(In reply to Bastien Nocera from comment #19) > Review of attachment 312640 [details] [review] [review]: > > ::: tests/lua-factory/data/test-source-xml-parser.lua > @@ +73,3 @@ > + if xml and xml.movieinfo then > + for i, t in ipairs(xml.movieinfo) do > + print ("[" .. i .. "] is " .. t.id) > > All this debug is not necessary, is it? No! This was really a WIP patch, I should have attached that in the summary. > > ::: tests/lua-factory/data/xml-parser-test-apple-trailers.xml > @@ +1,1 @@ > +<?xml version="1.0" encoding="utf-8"?> > > There are better tests (with expected output) in the lubyx XML docs. One > small one: > http://doc.lubyk.org/xml.html#Lua-table-format > And some bigger ones under: > https://github.com/lubyk/xml/blob/master/test/ I'll use those tests, much better indeed. The table format is a little different but I can fix that. Thanks for the review!
Created attachment 313017 [details] [review] lua-factory: better organize grl.get_media_keys() The logic around pushing the keys and its values are about to get a little bit more complex. This commit is to prepare the code for it.
Created attachment 313018 [details] [review] lua-factory: allow multiple values for each key All GrlMedia is GrlData and all its keys could have multiple values by default from grl_data_add_* API. Till now, Lua sources were only getting the first value of the key. This patch fixes that by providing multiple values as an array.
Created attachment 313029 [details] [review] tests: lua-factory check xml-parser Basic test for lua's xml-parser. This test runs a fake lua-source which download two mocked contents: a xml and its equivalent in lua's table. The fake test ceck if the table provided by grl.lua.xml.string_to_table is correct.
Review of attachment 312856 [details] [review]: Looks good.
Review of attachment 312857 [details] [review]: Yes, please be sure to backport.
Review of attachment 313017 [details] [review]: Looks fine if the tests still pass.
Review of attachment 313018 [details] [review]: Is that meant to be a fix for https://bugzilla.gnome.org/show_bug.cgi?id=756203 instead? Looks fine to me.
Review of attachment 313029 [details] [review]: > The fake test ceck if checks Looks fine otherwise. Can you add a test for the huge XML file in the original library? ::: tests/lua-factory/data/test-source-xml-parser.lua @@ +76,3 @@ + grl.warning("xml parser failed, results are not the same\n" .. + "reference table of test:\n" .. grl.lua.inspect(ref) .. + "\ntable from xml parser:\n" .. grl.lua.inspect(xml)) Don't like the "\n" at the beginning of the string here, add it at the end of the previous line instead.
(In reply to Bastien Nocera from comment #30) > Review of attachment 313018 [details] [review] [review]: > > Is that meant to be a fix for > https://bugzilla.gnome.org/show_bug.cgi?id=756203 instead? Not yet. This allows multiple values per key like multiple urls. This should be okay as it is allowed by grl_data_add_* API, so we should handle it. The GrlRelatedKeys is similar but we don't handle it from lua source and we don't pass this to the lua source. > Looks fine to me. Thanks
(In reply to Bastien Nocera from comment #31) > Review of attachment 313029 [details] [review] [review]: > > > The fake test ceck if > > checks fixed! > > Looks fine otherwise. Can you add a test for the huge XML file in the > original library? > > ::: tests/lua-factory/data/test-source-xml-parser.lua > @@ +76,3 @@ > + grl.warning("xml parser failed, results are not the same\n" .. > + "reference table of test:\n" .. grl.lua.inspect(ref) .. > + "\ntable from xml parser:\n" .. grl.lua.inspect(xml)) > > Don't like the "\n" at the beginning of the string here, add it at the end > of the previous line instead. sure!
Keeping the bug open for a few more tests in TODO for the next weeks Attachment 312856 [details] pushed as 1c6519b - tests: lua-factory check media from lua sources Attachment 312857 [details] pushed as ce0e7d4 - lua-factory: double check optional argument in fetch Attachment 313017 [details] pushed as 0530a37 - lua-factory: better organize grl.get_media_keys() Attachment 313018 [details] pushed as 9018103 - lua-factory: allow multiple values for each key Attachment 313029 [details] pushed as f67dbca - tests: lua-factory check xml-parser
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/grilo/issues/68.