Beets: Pass tests on Windows

[sampsyo]

I finally got a Windows VM up and running to try out our test suite there. The results were unsurprising:

Ran 980 tests in 32.036s
FAILED (errors=255, failures=14)

We should get to the bottom of these so that we can confidently run the test suite on Windows before each release. Even better would be to add new tests for Windows-specific dangers like Unicode path handling and subprocess invocations.

Help from Windows natives would be enormously helpful here.

[yevgenybezman]

Is there a way to run these tests automatically with travis?

On Tue, Apr 8, 2014 at 11:10 PM, Adrian Sampson [email protected]:

I finally got a Windows VM up and running to try out our test suite there.
The results were unsurprising:

---

Ran 980 tests in 32.036s
FAILED (errors=255, failures=14)

We should get to the bottom of these so that we can confidently run the
test suite on Windows before each release. Even better would be to add new
tests for Windows-specific dangers like Unicode path handling and
subprocess invocations.

Help from Windows natives would be enormously helpful here.

Reply to this email directly or view it on GitHubhttps://github.com/sampsyo/beets/issues/670
.

[sampsyo]

Alas, not yet: https://github.com/travis-ci/travis-ci/issues/216

[sampsyo]

Made some progress on fixing the tests on Windows today. Ran into a couple of more challenging issues:

• Some of the tests are multi-threaded and therefore open multiple thread-specific SQLite connections. These can't be closed from the main thread (conn.close() raises an exception). We need some way to close the connections from the threads that created them.
• We're getting "invalid filename" errors that I can't explain for several tests.
• There are a few tests that use the beets source directory to keep temporary files. These should use a temporary sandbox instead.
• Plenty of tests assume Unix-style paths (forward-slash-separated), leading to "Cannot mix UNC and non-UNC paths" errors.

[yevgenybezman]

Some of the tests are multi-threaded and therefore open multiple thread-specific SQLite connections. These can't be closed from the main thread (conn.close() raises an exception). We need some way to close the connections from the threads that created them.

@sampsyo
How bad would it be to wrap each usage of _connection() with contextlib.closing? Local sqlite connections should be cheap to create I think.

[sampsyo]

I'm actually not sure that they are that cheap. Closing and re-opening the connection on every transaction would mean going to the OS to open the file, loading the schema, invoking SQLite's integrity checks, etc. every time we need to read from or write to the database. I think that's a little bit overkill.

Some way to clean up the connection when a thread is actually done would be ideal. One nice thing is that this can be conservative—closing the connection early does not need to lead to correctness issues, since it will be re-opened on demand. It's just a performance thing.

[sampsyo]

Alright, we now have a Windows continuous integration service up and running! https://ci.appveyor.com/project/sampsyo/beets

Here's the current status:

Ran 1494 tests in 303.796s
FAILED (SKIP=26, errors=39, failures=46)

94% passes ain't bad! :smiley: Let the fixing begin.

[jackwilsdon]

Is there a reason that AppVeyor always reports a success, even if some tests fail? For example here. In this build, 51 tests failed, 38 errors and the command even exited with code 1 (highlighted in red at the end) but AppVeyor still says everything is fine.

[sampsyo]

I've told Appveyor to ignore all failures, since our test suite doesn't yet pass on Windows. Otherwise, we'd get "checks failed" for every PR.

[jackwilsdon]

Aha! I might try working out some of the Windows issues, 51 failed tests out of over 1500 isn't bad!

[sampsyo]

I did a little more work on the Windows tests today. I ran into a couple of obvious loose ends:

• Query paths like C:\foo\bar look like queries for the field c to beets. This is probably not worth addressing for UI reasons, but it did break some tests that were relying on passing paths as arguments.
• Speaking of which, is was really tricky to get the encoding right for passing paths as arguments. I fixed it for the convert plugin tests, but the problem may be lurking elsewhere too.
• The convert tests use an external program to do a fake conversion. I rewrote this in Python instead of using Unix shell commands, but it still doesn't work because of encodings.

There's still something mysterious going on with plugin loading too. I haven't worked out what it is yet because the problem seems to go away if you run any of the test files individually; the problem only arises as an inter-test dependency. :cry:

Current status:

Ran 1659 tests in 54.532s
FAILED (SKIP=34, errors=55, failures=48)

[sampsyo]

That convert stub problem is now fixed.

[sampsyo]

In 82640260c266aae3a895dfdd934b21a149aa1dad, I fixed an absolutely maddening bug that caused almost all plugin tests to fail because of some old, broken code in testall.py. It was hard to narrow down because it _only_ happened when running the entire test suite; running nosetests test/test_something.py alone made the problem disappear.

Ran 1659 tests in 63.838s
FAILED (SKIP=34, errors=25, failures=21)

[sampsyo]

I fiddled with some of the database closing issues today, which made up the bulk of the remaining unhandled exceptions:

Ran 1669 tests in 61.500s
FAILED (SKIP=35, errors=3, failures=20)

Pretty close! Most of the remaining failures have to do with filename vagaries.

[sampsyo]

I'm having fun making this a running log! I got some more of the low-hanging fruit today:

Ran 1676 tests in 63.462s
FAILED (SKIP=39, errors=2, failures=11)

There's a bunch of vexing path stuff in those remaining 13 tests.

[sampsyo]

A few more path-related fixes and a judicious skip brings us ever closer:

Ran 1677 tests in 61.198s
FAILED (SKIP=40, failures=9)

No more unhandled exceptions! That's something, right?

[jackwilsdon]

Great work on this so far @sampsyo! Windows support is ever-nearing 🎉 😄.

[sampsyo]

On my machine, we're down to one last test!!!!!!!!!!!!

Ran 1679 tests in 56.951s
FAILED (SKIP=40, failures=1)

It's a tricky one: it's a weird interaction with ImageMagick where my setup is saying "Invalid Parameter" for no reason I can discern.

FAIL: test_accept_similar_art (test.test_embedart.EmbedartCliTest)
----------------------------------------------------------------------
[...]
beets.embedart: DEBUG: embedart: ImageMagick convert failed with status 4: u'Invalid Parameter -
c:\\users\\xxx\\appdata\\local\\temp\\tmpsycmii.jpeg\r\n'

[jrobeson]

can you run the command it runs manually with convert?

[sampsyo]

Yeah, and weirdly the command works fine. A little fiddling with that did narrow it down, though: for whatever reason, Popen is getting a different convert.exe program that ships with Windows rather than the ImageMagick binary. Typing commands into PowerShell was invoking the right program, hence the disconnect.

[jackwilsdon]

Hmm, have you tried setting shell to True to see if it's PATH or cmd.exe related?

[sampsyo]

Good call; shell=True makes it work. I guess we could paper over the problem by just setting that flag on Windows? That feels a little icky...

[jackwilsdon]

Yeah it does. It seems that subprocess.Popen uses the CreateProcess function on Windows, which has the following description:

The name of the module to be executed. This module can be a Windows-based application. It can be some other type of module (for example, MS-DOS or OS/2) if the appropriate subsystem is available on the local computer.
The string can specify the full path and file name of the module to execute or it can specify a partial name. In the case of a partial name, the function uses the current drive and current directory to complete the specification. The function will not use the search path. This parameter must include the file name extension; no default extension is assumed.

This doesn't apply when using shell=True, as we run cmd.exe with CreateProcess and then run the convert command inside it.

I'm not entirely sure what the implications are of this, maybe we need to use the absolute path to convert somehow?

[sampsyo]

Yeah, an absolute path to convert.exe would work -- it would be nice to make this configurable.

In the mean time, though, I'm going to address the other problem here: that the check for ImageMagick "lies" and says everything is working fine even though convert refers to something else. I just noticed that a0c38a07a6d99ae8bb7461f1122ee34d70d2cd65 changed the check to use identify instead of convert for exactly this reason, which I believe was wrong -- it caused this problem we're having now. Finding out that we don't have a working convert is important!