Re: RFS: added autopkgtest for pplacer
Andreas Tille <andreas@fam-tille.de> wrote:
>Hi Mohd,
>
>Am Sun, Apr 10, 2022 at 08:08:42PM -0000 schrieb Mohd Bilal:
>> I've added autopkgtest for pplacer[1] . As suggested in the matrix room , I've used the test prescribed in the Makefile as well as included some of the commands from the pplacer tutorial[2] .
>
>Thanks a lot for your effort to write a test for pplacer.
>
>> Requesting someone to review and sponsor my changes.
>
>I have some remarks here.
>
>1. There is no information given where you obtained the data in
> debian/tests/data from. It is good style to add some script that
> fetches the data from somewhere (may be via wget or whaterver
> tool you prefer.
>
I've added a script[4] in d/tests/ that fetches the test data
>2. The data should be also mentioned in d/copyright.
>
>3. Since the size of the data exceedes the size of the other files
> it might make sense to create a separate tarball as it is
> described in the Debian Med team policy[3]. Alternatively
> you might consider xz compressing the data files (which needs
> adding them in debian/source/include-binaries) and uncompressing
> them once they are needed. The compression rate might be
> sufficiently high to tolerate the size of the compressed data
> files.
> It might be also be sensible to create an extra pplacer-examples
> binary package in case the compression rate is not high enough.
> A sign for the issue is also given by lintian:
> I: pplacer: arch-dep-package-has-big-usr-share 12192kB 39%
>
I've followed the instructions in the med-team policy and moved the tests data to debian-tests-data. The upstream repo[5] for test data doesn;t state a license . So what should be included in the d/copyright file?
>4. Unfortunately the test does not work in my pbuilder environment.
> I've added a `set -x` to run-unit-test and so I'Ve got:
>
>autopkgtest [07:44:44]: test run-unit-test: [-----------------------
>+ export LC_ALL=C.UTF-8
>+ LC_ALL=C.UTF-8
>+ '[' /tmp/autopkgtest.W5By5K/autopkgtest_tmp = '' ']'
>+ cp -a /usr/share/doc/pplacer/examples/src /usr/share/doc/pplacer/examples/vaginal_16s.refpkg /tmp/autopkgtest.W5By5K/autopkgtest_tmp
>+ cp -a /tmp/autopkgtest.W5By5K/tree/ /tmp/autopkgtest.W5By5K/autopkgtest_tmp
>+ cd /tmp/autopkgtest.W5By5K/autopkgtest_tmp
>+ echo -e '\e[93m\e[1mTest 1\e[0m'
>+ cd src
>+ make test
>Test 1
>make: *** No rule to make target 'test'. Stop.
>autopkgtest [07:44:45]: test run-unit-test: -----------------------]
>
>There is no Makefile provided and if I'm checking the build directory
>the Makefile would need a binary _build/tests/tests.native which
>contains the according calls to the pplacer binaries. Thus the test
>only works in a full source tree ... and I admit I'm quite baffled
>why the Salsa-CI test is passing. It obviously rebuilds the needed
>binary but I have no idea why.
>
I've removed the part that uses the Makefile for testing and autopkgtest now uses commands from the tutorial[6] and verifies their checksums. So would this be ok for now?
>I think what we should do instead is to read the text of the upstream
>test and implement (part of) the calls to the tools as shell script.
>
>Kind regards and thanks again for your work on this anyway
>
> Andreas.
>
Thanks a lot for this detailed review. Please see whether my changes are appropriate or is there anything more to be done.
Regards,
rmb
>> [1] - https://salsa.debian.org/med-team/pplacer
>> [2] - https://fhcrc.github.io/microbiome-demo/
>[3] https://med-team.pages.debian.net/policy/#embedding-large-test-data
>
[4] - https://salsa.debian.org/med-team/pplacer/-/blob/master/debian/tests/get-test-data
[5] - https://github.com/fhcrc/microbiome-demo
[6] -https://fhcrc.github.io/microbiome-demo/
>--
>http://fam-tille.de
Reply to: