[Bug 41448] Installing unit tests

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=41448

Alban Crequy <alban.crequy at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|http://cgit.collabora.com/g |http://cgit.collabora.com/g
                   |it/user/alban/telepathy-gab |it/user/alban/telepathy-gab
                   |ble.git/log/?h=tests3       |ble.git/log/?h=tests4
  Status Whiteboard|review-                     |

--- Comment #4 from Alban Crequy <alban.crequy at collabora.co.uk> 2011-10-12 10:57:16 PDT ---
(In reply to comment #1)
> tests: add --enable-install-tests¡
> 
> Did you test what happens if $TEST_PYTHON is false (i.e. if python or twisted
> or similar are missing)?

"make check" tests only the C tests, but the python tests are still installed
when requested.

> +    [Enable installation of twisted tests]),
> 
> Only twisted ones?

C tests added.

> The existing ones use a lower care first letter. I would keep the string just
> as ´install tests¡ or ´make tests installable¡.

Done

> +  AC_DEFINE(ENABLE_TESTS_INSTALL, [],
> 
> Why not ENABLE_INSTALL_TESTS like the option name?

Changed to ENABLE_INSTALLED_TESTS

> +      [Installation of test files])
> 
> The documentation string is not very clear

Changed

> +twistedcapstestsdir = $(GABBLE_TESTS_PATH)/twisted/caps
> 
> Are you sure there is no way to avoid having an entry per subdirectory? :(

Changed to nobase

> +BUILT_SOURCES += $(service_files) $(conf_files) tools/exec-with-log.sh
> run-test.sh
> 
> And run-gabble.sh?

Added

> +EXTRA_DIST = \
> +   $(TWISTED_TESTS) \
> +   $(TWISTED_OTHER_FILES) \
> 
> How was it working before without this?

TWISTED_TESTS was already in EXTRA_DIST. TWISTED_OTHER_FILES was not but files
were listed individually in EXTRA_DIST.

> +script_fullname=`readlink -e "@abs_top_tests_installdir@/twisted/run-test.sh"`
> +if [ `readlink -e "$0"` != "$script_fullname" ] ; then
> +  echo "This script is meant to be installed"
> +  exit 1
> +fi
> 
> Hm?

If run-test.sh is run from the source tree, it exits with an error.

> +Exec=@abs_top_tests_installdir@/twisted/tools/run-gabble.sh
> 
> I think it's a bit weird that you changed the purpose of the .service.in file
> in tools/ and moved the old one to tools/servicedir-uninstalled.
> Why not keeping the old files as they were and having another dir for the
> installed ones? Or maybe having an uninstalled/ and an installed/ directories.

I think it worth having 2 subdirectories:
- servicedir/
- servicedir-uninstalled/
Updated.

> +export GABBLE_PLUGIN_DIR="usr/lib/telepathy/gabble-0"
> 
> It's missing a ´/¡ at the beginning, but anyway you should not rely on stuff
> being in /usr.

Fixed

> +  <!-- This is included last so local configuration can override what's 
> +       in this standard file -->
> +  
> +
> +  
> +
> +</busconfig>
> 
> Too many empty lines :P

Fixed

> ´tests/twisted/Makefile.am: fix make dist¡
> ´fix confusion between both dbus config files¡
> 
> I think you should squash these patches with the previous one.

Yes.

(In reply to comment #2)
> (In reply to comment #1)
> > +  AC_DEFINE(ENABLE_TESTS_INSTALL, [],
> > 
> > Why not ENABLE_INSTALL_TESTS like the option name?
> 
> --enable-installed-tests, ENABLE_INSTALLED_TESTS would be better grammar, and
> match what I did in dbus.

Done

> > +twistedcapstestsdir = $(GABBLE_TESTS_PATH)/twisted/caps
> > 
> > Are you sure there is no way to avoid having an entry per subdirectory? :(
> 
> The thing to search for is "nobase" in Automake documentation. If
> nobase_dist_twisted_DATA contains "caps/foo.py" then it will be installed to
> $(twisteddir)/caps/foo.py.

Done

> > You should not rely on the python executable to be /usr/bin/python. I think
> > that AM_PATH_PYTHON provides some magic to detect the python executable.
> 
> AM_PATH_PYTHON sets $(PYTHON). In Gabble only, $(TEST_PYTHON) is also relevant.

Really? A few scripts start with the shebang #!/usr/bin/python, e.g.
tools/glib-ginterface-gen.py

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.