Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

failing unittests in systemd-nspawn container #157

Closed
shibumi opened this issue May 7, 2017 · 21 comments
Closed

failing unittests in systemd-nspawn container #157

shibumi opened this issue May 7, 2017 · 21 comments
Assignees
Labels
Bug Generic bug: can be used together with more specific labels

Comments

@shibumi
Copy link

shibumi commented May 7, 2017

Hello,
I'd like to move whipper to the official arch linux repositories. For this I need to package whipper in a clean chroot/systemd-nspawn container. Whipper is building, but the unit tests are failing:

...............WARNING:morituri.common.program:Track 01: not matched in AccurateRip database
WARNING:morituri.common.program:Track 01: none of the responses matched.
WARNING:morituri.common.program:Track 10: not matched in AccurateRip database
WARNING:morituri.common.program:Track 10: none of the responses matched.
......................................................WARNING:morituri.program.cdrdao:cdrdao version detection failed: could not find version
F..WARNING:morituri.common.mbngs:Release with ID '3451f29c-9bb8-4cc5-bfcc-bd50104b94f8' (Jeff Buckley - Everybody Here Wants You) does not have a date
............F
======================================================================
FAIL: testGetVersion (morituri.test.test_program_cdrdao.VersionTestCase)
testGetVersion
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/twisted/internet/defer.py", line 150, in maybeDeferred
    result = f(*args, **kw)
  File "/usr/lib/python2.7/site-packages/twisted/internet/utils.py", line 201, in runWithWarningsSuppressed
    reraise(exc_info[1], exc_info[2])
  File "/usr/lib/python2.7/site-packages/twisted/internet/utils.py", line 197, in runWithWarningsSuppressed
    result = f(*a, **kw)
  File "/build/whipper/src/whipper-0.5.1/morituri/test/test_program_cdrdao.py", line 15, in testGetVersion
    self.failUnless(v)
  File "/usr/lib/python2.7/site-packages/twisted/trial/_synctest.py", line 395, in assertTrue
    super(_Assertions, self).assertTrue(condition, msg)
  File "/usr/lib/python2.7/unittest/case.py", line 422, in assertTrue
    raise self.failureException(msg)
FailTest: None is not true

======================================================================
FAIL: testAll (morituri.test.test_common_directory.DirectoryTestCase)
testAll
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/twisted/internet/defer.py", line 150, in maybeDeferred
    result = f(*args, **kw)
  File "/usr/lib/python2.7/site-packages/twisted/internet/utils.py", line 201, in runWithWarningsSuppressed
    reraise(exc_info[1], exc_info[2])
  File "/usr/lib/python2.7/site-packages/twisted/internet/utils.py", line 197, in runWithWarningsSuppressed
    result = f(*a, **kw)
  File "/build/whipper/src/whipper-0.5.1/morituri/test/test_common_directory.py", line 13, in testAll
    self.failUnless(path.startswith('/home'))
  File "/usr/lib/python2.7/site-packages/twisted/trial/_synctest.py", line 395, in assertTrue
    super(_Assertions, self).assertTrue(condition, msg)
  File "/usr/lib/python2.7/unittest/case.py", line 422, in assertTrue
    raise self.failureException(msg)
FailTest: False is not true

----------------------------------------------------------------------
Ran 85 tests in 6.352s

FAILED (failures=2)
==> ERROR: A failure occurred in check().
    Aborting...
==> ERROR: Build failed, check /var/lib/archbuild/extra-x86_64/chris/build
extra-x86_64-build -- -I   5.20s user 1.55s system 48% cpu 13.907 total

Do I miss a dependency or is this an issue because of the chroot?

My dependencies:

accuraterip-checksum' 
cddb-py'              
cdparanoia'           
cdrdao'               
libsndfile'           
mutagen'              
python2-musicbrainzngs
python2-pycdio'       
python2-setuptools'   
sox'                  
python2-gobject2 #only for the make process
python2-twisted #only for the unit tests
@Freso
Copy link
Member

Freso commented May 8, 2017

Can you try building the whipper-git package? I made a couple of changes to the codebase since the 0.5.1 release which I have also reflected in the whipper-git PKGBUILD (e.g., it no longer conflicts with morituri). … Which means that whatever's causing this might have been fixed in the master branch. If not, can you maybe try to do a git bisect to see when this was introduced (or if it was "always" there!)?

(I've also been meaning to apply for TU myself so I could push this package to [community], but eh. :) I've tried using makechrootpkg on some of my AUR PKGBUILDs (whipper{,-git} included), but it just doesn't want to work for me, and I haven't been able to find any references to others with the issues that actually had usable solutions. :/ I guess I'll have to come around posting on my own one of these days…)

@shibumi
Copy link
Author

shibumi commented May 8, 2017

Well..I don't want to test it with whipper-git because I want to build a stable release. Could you push a new version? I've just downloaded the tarball-release..

About building packages: You can use extra-x86_64-build instead of mkchrootpkg.. thats a wrapper for mkchrootpkg.. just do $ extra-x86_64-build in your directory with the PKGBUILD.

@Freso
Copy link
Member

Freso commented May 8, 2017

I'm not one of the core maintainers (edit: and as such can't tag and push new releases), but it seems futile to try and find a bug if it has already been fixed in latest master. Can you please try and see if building whipper-git works? If it does, I can push the core maintainers to make a new release with the updated code. If it doesn't work, then we know the issue is still present, and I will ask you to please do a git bisect to figure out when this broke.

@shibumi
Copy link
Author

shibumi commented May 8, 2017

@Freso sure

@tobbez
Copy link
Contributor

tobbez commented May 8, 2017

For the first error, cdrdao needs to be on the path, and if called without arguments its output (sent to stderr) should contain a line starting with Cdrdao version .

The second error occurs because the test's assumption that the home directory of the user running the test is stored under /home/. You can work around this by making sure the user's home directory inside the chroot is stored under /home/, but the test's behaviour here is broken, and should be fixed.

@shibumi
Copy link
Author

shibumi commented May 10, 2017

@tobbez well, cdrdao should be in the path. It's definitly installed inside of the container.

So the tests are nothing serious and the application will work?

@JoeLametta
Copy link
Collaborator

JoeLametta commented Jun 19, 2017

@tobbez well, cdrdao should be in the path. It's definitly installed inside of the container.

I think the first test fails in Arch Linux because the cdrdao package provided by that distribution ships with this patch applied:

--- cdrdao/dao/main.cc~	2009-04-16 15:34:27.000000000 +0200
+++ cdrdao/dao/main.cc	2009-04-16 15:34:38.000000000 +0200
@@ -207,7 +207,7 @@
 
 static void printVersion()
 {
-  log_message(2, "Cdrdao version %s - (C) Andreas Mueller <[email protected]>",
+  log_message(0, "Cdrdao version %s - (C) Andreas Mueller <[email protected]>",
 	  VERSION);
 
   std::list<std::string> list;

So what happens is that the "Cdrdao version" line isn't being printed to stderr unless cdrdao's verbosity level is increased to, at least, zero. I currently haven't checked if the error would go away by simply changing cdrdao's invocation in the test with something like this:

cdrdao -v0

Regarding the second test I agree it's silly. Maybe it could be rewritten like this:

--- test_common_directory.py.a
+++ test_common_directory.py.b
@@ -1,16 +1,18 @@
 # -*- Mode: Python; test-case-name: whipper.test.test_common_directory -*-
 # vi:si:et:sw=4:sts=4:ts=4
 
+from os.path import dirname, expanduser
 from whipper.common import directory
-
 from whipper.test import common
 
 
 class DirectoryTestCase(common.TestCase):
+    HOME = expanduser('~')
+    HOME_PARENT = dirname(HOME)
 
     def testAll(self):
         path = directory.config_path()
-        self.failUnless(path.startswith('/home'))
+        self.failUnless(path.startswith(DirectoryTestCase.HOME_PARENT))
 
         path = directory.cache_path()
-        self.failUnless(path.startswith('/home'))
+        self.failUnless(path.startswith(DirectoryTestCase.HOME_PARENT))

P.S. @shibumi sorry for the slow reply...

@tobbez
Copy link
Contributor

tobbez commented Jun 19, 2017

The patch you refer to has no impact on this issue. The change in the patch means it will be displayed in more cases, not fewer. A message is displayed if its level is less than or equal to the verbosity requested upon invocation, and the default is 2. Both 2 (unpatched) and 0 (patched) are <= 2.

The problem is most likely with Arch's libao, as shown in this session provided by a user on IRC:

[antergos@ant-17 ~]$ cdrdao
ERROR: Failed to load plugin /usr/lib/ao/plugins-4/libpulse.so => dlopen() failed
Cdrdao version 1.2.3 - (C) Andreas Mueller <[email protected]>

Usage: cdrdao <command> [options] [toc-file]
[...]

There are two issues here:

  • There's a problem with Arch's libao. This is not an issue for us, as libao is only used by cdrdao for conversion to MP3 and Ogg, which is not used by whipper.
  • libao prints to stderr by default when it fails to load a plugin, which is not sensible for a library. Debian had the same issue, and fixed it by adding the quiet option to /etc/libao.conf in their package.

@JoeLametta
Copy link
Collaborator

JoeLametta commented Jun 19, 2017

The patch you refer to has no impact on this issue.

You're right, it seems I've got the logic backwards. I'm sorry.

The problem is most likely with Arch's libao

@shibumi Could you try executing cdrdao in the clean chroot and report here everything that gets printed (stdout & stderr)?

Thanks

@shibumi
Copy link
Author

shibumi commented Jun 21, 2017

@JoeLametta Shall I just execute cdrdao --version or anything else?

@JoeLametta
Copy link
Collaborator

JoeLametta commented Jun 21, 2017

@JoeLametta Shall I just execute cdrdao --version or anything else?

Just execute cdrdao (without any argument)

😉

@shibumi
Copy link
Author

shibumi commented Jun 21, 2017

@JoeLametta

ERROR: Failed to load plugin /usr/lib/ao/plugins-4/libpulse.so => dlopen() failed
Cdrdao version 1.2.3 - (C) Andreas Mueller <[email protected]>

Usage: cdrdao <command> [options] [toc-file]
command:
  show-toc   - prints out toc and exits
  toc-info   - prints out short toc-file summary
  toc-size   - prints total number of blocks for toc
  read-toc   - create toc file from audio CD
  read-cd    - create toc and rip audio data from CD
  read-cddb  - contact CDDB server and add data as CD-TEXT to toc-file
  show-data  - prints out audio data and exits
  read-test  - reads all audio files and exits
  disk-info  - shows information about inserted medium
  discid     - prints out CDDB information
  msinfo     - shows multi session info, output is suited for scripts
  drive-info - shows drive information
  unlock     - unlock drive after failed writing
  blank      - blank a CD-RW
  scanbus    - scan for devices
  simulate   - shortcut for 'write --simulate'
  write      - writes CD
  copy       - copies CD


 Try 'cdrdao <command> -h' to get a list of available options

the first ERROR is because there is currently no pulseaudio in the container..

@JoeLametta
Copy link
Collaborator

JoeLametta commented Jun 21, 2017

the first ERROR is because there is currently no pulseaudio in the container..

Thanks for your test.

It seems @tobbez was right indeed (message here): that's not an issue which affects whipper.
@shibumi So, have you decided how to handle that test failure under Arch Linux?

P.S. In the meantime I'm going to fix the second test failure using the patch proposed above (edited).

@shibumi
Copy link
Author

shibumi commented Jun 22, 2017

@JoeLametta At the moment I just ignore the failing tests.. the package is already in the official repos:

https://www.archlinux.org/packages/community/any/whipper/

@JoeLametta
Copy link
Collaborator

@JoeLametta At the moment I just ignore the failing tests.. the package is already in the official repos

I've just pushed a commit to whipper's master branch which should fix test_common_directory.py.

Yeah, I've seen that. What I meant was: how are you going to deal with the first failure in Arch Linux (as that's something which isn't related to whipper?)

@JoeLametta JoeLametta added the Bug Generic bug: can be used together with more specific labels label Jun 30, 2017
@JoeLametta JoeLametta self-assigned this Jun 30, 2017
@shibumi
Copy link
Author

shibumi commented Jun 30, 2017

@JoeLametta sorry which failure do you talk about exactly? All failures are because of failing unit tests for whipper. At the moment I just don't do unit tests and call the install process directly. So no real issue.

@JoeLametta
Copy link
Collaborator

@shibumi Regarding the test_program_cdrdao.py, here's what tobbez wrote a while ago...

The problem is most likely with Arch's libao, as shown in this session provided by a user on IRC:

[antergos@ant-17 ~]$ cdrdao
ERROR: Failed to load plugin /usr/lib/ao/plugins-4/libpulse.so => dlopen() failed
Cdrdao version 1.2.3 - (C) Andreas Mueller [email protected]

Usage: cdrdao [options] [toc-file]
[...]

There are two issues here:

There's a problem with Arch's libao. This is not an issue for us, as libao is only used by cdrdao for conversion to MP3 and Ogg, which is not used by whipper.
libao prints to stderr by default when it fails to load a plugin, which is not sensible for a library. Debian had the same issue, and fixed it by adding the quiet option to /etc/libao.conf in their package.

The output you've posted here seems to confirm that explanation. I'm referring to the two issues mentioned by tobbez in the quoted section: the cdrdao test is failing because of reasons beyond the control of whipper.

@JoeLametta
Copy link
Collaborator

@shibumi Nothing to reply to my previous message?

@shibumi
Copy link
Author

shibumi commented Aug 12, 2017

@JoeLametta What is the issue with libao in arch linux? We use upstream libao. There shouldn't be a problem.

@tobbez
Copy link
Contributor

tobbez commented Aug 13, 2017

@shibumi I already explained this: libao will by default write errors to stdout when failing to load a plugin. This is simply not acceptable for a library, since it breaks other applications that parse the output from programs using that library.

This behavior can be disabled in the configuration file (/etc/libao.conf) by adding the directive quiet on its own line.

Debian already had the very same problem, and fixed it that way: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=590961#27.

The way this needs to be fixed makes it a packaging problem, which means Arch has to fix that (i.e. add the quiet directive to the libao.conf their libao package ships).

@shibumi
Copy link
Author

shibumi commented Aug 15, 2017

@tobbez thanks I will add a bug report for this issue in the arch linux bug tracker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Generic bug: can be used together with more specific labels
Projects
None yet
Development

No branches or pull requests

4 participants