Bug 28334 - Test library missing namespace (making it much more difficult to bind in other languages)
Summary: Test library missing namespace (making it much more difficult to bind in othe...
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-glib (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/sm...
Whiteboard:
Keywords: patch
Depends on: 28347
Blocks: 27792
  Show dependency treegraph
 
Reported: 2010-05-31 11:39 UTC by Travis Reitter
Modified: 2010-06-07 04:00 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Description Travis Reitter 2010-05-31 11:39:21 UTC
See the fix branch in the URL. It just adds the namespace tp_tests/TpTests/TP_TESTS to the test library and updates its dependencies accordingly.

Necessary for the Vala bindings (so its tests work). This bug is partly to reduce the size of the Vala bindings branch, partly to make it a lot easier for me to keep up with mainline until the bindings are merged.
Comment 1 Simon McVittie 2010-06-01 03:08:02 UTC
The test library isn't really a proper library, but if Vala really can't deal with code that isn't at least pretending to be a library, and you need the test pseudo-library for the Vala tests, I suppose we could re-namespace it.

However, this is far too large for a single patch and I'd much prefer it in sensible-sized patches: for instance, one to rename each GObject class and make the tests pass again, and one for each utility function or group of utility functions.

> tp_tests_clear_object

Unless I'm missing something, this macro is useless for bindings, so renaming it is meaningless?

(I'm tempted to add it to the public util.h as tp_clear_object, though; we do this sort of boilerplate often enough that it seems potentially useful. CPython has Py_CLEAR for this purpose.)

> tp_tests_assert_no_error

If you're going to replace this with anything, please replace calls to it with g_assert_no_error() and delete the definition. I didn't do this systematically yet because it would be a relatively intrusive patch (although, less intrusive than this one).

> test_connection_run_until_dbus_queue_processed

If you're going to replace this with anything, please replace it with test_proxy_run_until_dbus_queue_processed and delete the definition. I didn't do this systematically yet because it would be a relatively intrusive patch.

> test_connection_run_until_ready

If you're going to replace this with anything, please replace it with test_proxy_run_until_prepared (., [TP_CONNECTION_FEATURE_CONNECTED]) and delete the definition. I didn't do this systematically yet because it would be a relatively intrusive patch.

> test_connection_manager_run_until_ready

Likewise, but test_proxy_run_until_prepared (., NULL).

> test_connection_manager_run_until_readying_fails

Likewise, but tp_tests_proxy_run_until_prepared_or_failed (cm, NULL, &error) and ensure it's followed by either g_assert_error (error, domain, code) or g_assert (error != NULL).
Comment 2 Simon McVittie 2010-06-01 06:43:21 UTC
(In reply to comment #1)
> > tp_tests_clear_object
> 
> Unless I'm missing something, this macro is useless for bindings, so renaming
> it is meaningless?
> 
> (I'm tempted to add it to the public util.h as tp_clear_object, though; we do
> this sort of boilerplate often enough that it seems potentially useful. CPython
> has Py_CLEAR for this purpose.)

Fixed in Bug #28345.

> > tp_tests_assert_no_error
> 
> If you're going to replace this with anything, please replace calls to it with
> g_assert_no_error() and delete the definition. I didn't do this systematically
> yet because it would be a relatively intrusive patch (although, less intrusive
> than this one).

This and the other similar things are fixed in Bug #28347.
Comment 3 Travis Reitter 2010-06-04 21:08:08 UTC
(In reply to comment #1)
> The test library isn't really a proper library, but if Vala really can't deal
> with code that isn't at least pretending to be a library, and you need the test
> pseudo-library for the Vala tests, I suppose we could re-namespace it.

Yeah, as far as Vala's concerned, there are something like 15-20 namespaces in this library (including "" in at least once place), and I don't think it supports (> 1 namespace)/library (at least when you're binding it from C).

> However, this is far too large for a single patch and I'd much prefer it in
> sensible-sized patches: for instance, one to rename each GObject class and make
> the tests pass again, and one for each utility function or group of utility
> functions.

Sorry about that. I thought it was a lot smaller, but it was painfully obvious when I split it up.

Please see the new branch. It's rebased against the branch from bug #28347
Comment 4 Simon McVittie 2010-06-07 02:50:30 UTC
Thanks, this looks much better. review+ with a couple of changes:

> -   * regression test that's going to be run under a temporary session bus,
> +   * regression tp_tests that's going to be run under a temporary session bus,

Over-zealous sed detected :-)

> -#endif
> +#endif /* #ifndef __TP_TESTS_LIB_UTIL_H__ */
> -#endif
> +#endif /* ifndef __TP_TESTS_CONTACTS_CONN_H__ */

If anything, I'd be inclined to *remove* this comment from the multi-inclusion header guards. It doesn't seem to me as though it adds value (it should be obvious that the last #endif in a header is for the multi-inclusion guard) and it's one more thing to (fail to) change when basing a new header file on an old one.
Comment 5 Simon McVittie 2010-06-07 03:07:56 UTC
(In reply to comment #4)
> Thanks, this looks much better. review+ with a couple of changes:
> 
> > -   * regression test that's going to be run under a temporary session bus,
> > +   * regression tp_tests that's going to be run under a temporary session bus,
> 
> Over-zealous sed detected :-)

Because I suspect this branch will cause conflicts/test failures in most other pending telepathy-glib branches, I'd like to fast-track it in, so I've corrected this in branch smcv/tp-tests and am stealing this bug.

> > -#endif
> > +#endif /* #ifndef __TP_TESTS_LIB_UTIL_H__ */
> > -#endif
> > +#endif /* ifndef __TP_TESTS_CONTACTS_CONN_H__ */
> 
> If anything, I'd be inclined to *remove* this comment from the multi-inclusion
> header guards.

Not a merge blocker, just a comment for the future really, so I'm ignoring this.
Comment 6 Simon McVittie 2010-06-07 03:09:45 UTC
Actually, I'll need to fix conflicts with some changes from Zdra that are already in master. In progress...
Comment 7 Simon McVittie 2010-06-07 03:17:13 UTC
Try this. With my changes, this is r+ from me.
Comment 8 Simon McVittie 2010-06-07 04:00:14 UTC
r+ from Guillaume, merged for 0.11.7.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.