GNOME Bugzilla – Bug 748455
Some cleanup I made in grilo, I want to implement a test for GrlMediaContainer in a separate branch
Last modified: 2015-07-17 13:29:06 UTC
GrlMediaContainer in a separate branch.
Created attachment 302340 [details] [review] core: Remove remaining references to GrlMetadataSource.
Created attachment 302341 [details] [review] tests: make C tests run again.
Created attachment 302342 [details] [review] tests: make python tests run again.
Created attachment 302343 [details] [review] tests: disable python tests. As they don't pass anymore.
Created attachment 302344 [details] [review] core: Fix gitignore generation.
Created attachment 302345 [details] [review] core: skip introspection of grl_init_get_option_group. This triggered a warning as it returns a bare structure, it is usually skipped by other libraries.
Review of attachment 302340 [details] [review]: The test files should probably be left well alone. They are still working and used.
Review of attachment 302341 [details] [review]: Can you add the reasoning behind this fix in the commit message? Looks fine otherwise.
Review of attachment 302342 [details] [review]: Can you split those up into "Use GLib through introspection" and "fix loading filesystem plugin"?
Review of attachment 302343 [details] [review]: Aren't those the python tests you fixed earlier?
(In reply to Bastien Nocera from comment #7) > Review of attachment 302340 [details] [review] [review]: > > The test files should probably be left well alone. They are still working > and used. make check doesn't even build with master here.
(In reply to Bastien Nocera from comment #8) > Review of attachment 302341 [details] [review] [review]: > > Can you add the reasoning behind this fix in the commit message? Looks fine > otherwise. (In reply to Bastien Nocera from comment #9) > Review of attachment 302342 [details] [review] [review]: > > Can you split those up into "Use GLib through introspection" and "fix > loading filesystem plugin"? Will do
(In reply to Bastien Nocera from comment #10) > Review of attachment 302343 [details] [review] [review]: > > Aren't those the python tests you fixed earlier? I fixed running them, not the actual assertions which I haven't dug into.
(In reply to Mathieu Duponchelle from comment #13) > (In reply to Bastien Nocera from comment #10) > > Review of attachment 302343 [details] [review] [review] [review]: > > > > Aren't those the python tests you fixed earlier? > > I fixed running them, not the actual assertions which I haven't dug into. Probably worth holding onto those fixes until they allow running the tests then.
Review of attachment 302344 [details] [review]: Can you split this into 2? 1 for the lib/pls fix, one for the src/data fix?
Review of attachment 302345 [details] [review]: That's wrong, see bug 743351. We might need to bump dependencies to make this work though.
(In reply to Bastien Nocera from comment #14) > (In reply to Mathieu Duponchelle from comment #13) > > (In reply to Bastien Nocera from comment #10) > > > Review of attachment 302343 [details] [review] [review] [review] [review]: > > > > > > Aren't those the python tests you fixed earlier? > > > > I fixed running them, not the actual assertions which I haven't dug into. > > Probably worth holding onto those fixes until they allow running the tests > then. I think being able to run make check should be the priority, if someone comes up and fixes the actual tests then it would be easy to reenable them?
(In reply to Bastien Nocera from comment #16) > Review of attachment 302345 [details] [review] [review]: > > That's wrong, see bug 743351. > > We might need to bump dependencies to make this work though. Ah cool :)
(In reply to Bastien Nocera from comment #15) > Review of attachment 302344 [details] [review] [review]: > > Can you split this into 2? 1 for the lib/pls fix, one for the src/data fix? Will do this afternoon to
(In reply to Mathieu Duponchelle from comment #17) > (In reply to Bastien Nocera from comment #14) > > (In reply to Mathieu Duponchelle from comment #13) > > > (In reply to Bastien Nocera from comment #10) > > > > Review of attachment 302343 [details] [review] [review] [review] [review] [review]: > > > > > > > > Aren't those the python tests you fixed earlier? > > > > > > I fixed running them, not the actual assertions which I haven't dug into. > > > > Probably worth holding onto those fixes until they allow running the tests > > then. > > I think being able to run make check should be the priority, if someone > comes up and fixes the actual tests then it would be easy to reenable them? Right, in which case, I'd rather have an "exit 0" with a reference to a bug added to the tests.
Created attachment 302375 [details] [review] tests: make C tests run again. GrlMediaPlugin is now GrlSource.
Created attachment 302376 [details] [review] tests: Use GLib though introspection + And remove useless pygtk import.
Created attachment 302377 [details] [review] tests: Fix loading plugins.
Created attachment 302378 [details] [review] tests: disable python tests. They fail for various reasons for now and we want make check to pass.
Created attachment 302379 [details] [review] core: Add a Makefile.am in src/data To have a gitignore generated there as well.
Created attachment 302380 [details] [review] core: generate gitgnore in libs/pls too.
Review of attachment 302344 [details] [review]: obsoleted
Review of attachment 302342 [details] [review]: obsoleted
Review of attachment 302340 [details] [review]: As answered, these test files don't work , can you review this again ?
Created attachment 302418 [details] [review] core: Add a Makefile.am in src/data To have a gitignore generated there as well.
Review of attachment 302375 [details] [review]: Sure.
Review of attachment 302376 [details] [review]: Sure.
Review of attachment 302377 [details] [review]: Looks good.
Review of attachment 302378 [details] [review]: ::: tests/python/testrunner.py @@ +5,3 @@ import sys +exit (0) Can you add a bug reference (eg. a new bug) for fixing the Python plugins? Thinking about it, maybe those tests, which all require the filesystem plugin, would be better suited for grilo-plugins. That can be done separately.
Review of attachment 302380 [details] [review]: Sure.
Review of attachment 302418 [details] [review]: This would be better if the sources were also added to the new Makefile.am, instead of compiled from the parent directory (all the "data/" references in src/Makefile.am). Probably good enough for now.
Created attachment 302419 [details] [review] core: Remove remaining references to GrlMetadataSource
Created attachment 302420 [details] [review] tests: make C tests run again GrlMediaPlugin is now GrlSource.
Created attachment 302421 [details] [review] tests: Use GLib though introspection + And remove useless pygtk import.
Created attachment 302422 [details] [review] tests: Fix loading plugins
Created attachment 302423 [details] [review] tests: disable python tests They fail for various reasons for now and we want make check to pass.
Created attachment 302424 [details] [review] core: Add a Makefile.am in src/data To have a gitignore generated there as well.
Created attachment 302425 [details] [review] core: generate gitgnore in libs/pls too
Review of attachment 302340 [details] [review]: Sure then.
Review of attachment 302419 [details] [review]: Looks good.
Review of attachment 302420 [details] [review]: Sure.
Review of attachment 302421 [details] [review]: Remove the "+" in the commit message, looks fine otherwise.
Review of attachment 302422 [details] [review]: Looks good.
Review of attachment 302423 [details] [review]: Sure.
Review of attachment 302424 [details] [review]: If "make distcheck" still passes, then this looks good.
Review of attachment 302425 [details] [review]: Yep.
Created attachment 303608 [details] [review] core: Remove remaining references to GrlMetadataSource
The patch I reattached was breaking make distcheck, it is fixed now, make distcheck passes on my end with the commits I proposed in https://bugzilla.gnome.org/show_bug.cgi?id=749587, I'll just need a "go" and I'll push everything :)