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

Pass tests on Windows #670

Closed
sampsyo opened this issue Apr 8, 2014 · 23 comments
Closed

Pass tests on Windows #670

sampsyo opened this issue Apr 8, 2014 · 23 comments

Comments

@sampsyo
Copy link
Member

sampsyo commented Apr 8, 2014

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
Copy link
Contributor

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

On Tue, Apr 8, 2014 at 11:10 PM, Adrian Sampson notifications@git.luolix.topwrote:

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//issues/670
.

@sampsyo
Copy link
Member Author

sampsyo commented Apr 9, 2014

Alas, not yet: travis-ci/travis-ci#216

sampsyo added a commit that referenced this issue Apr 12, 2014
@sampsyo
Copy link
Member Author

sampsyo commented Apr 12, 2014

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.

sampsyo added a commit that referenced this issue Apr 13, 2014
Identified while tackling #670, but this should actually solve some legitimate
problems with cataloging music on a network drive.
sampsyo added a commit that referenced this issue Apr 13, 2014
Doing test-specific cleanup in tearDown before general sandbox deletion helps
avoiding contamination of global state between tests when cleanup fails.

Current Windows status:

    Ran 1106 tests in 72.373s
    FAILED (SKIP=10, errors=13, failures=15)

Closer!
@yevgenybezman
Copy link
Contributor

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
Copy link
Member Author

sampsyo commented Apr 16, 2014

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
Copy link
Member Author

sampsyo commented Apr 14, 2015

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! 😃 Let the fixing begin.

@jackwilsdon
Copy link
Member

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
Copy link
Member Author

sampsyo commented Sep 11, 2015

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
Copy link
Member

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

@sampsyo
Copy link
Member Author

sampsyo commented May 31, 2016

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. 😢

Current status:

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

@sampsyo
Copy link
Member Author

sampsyo commented Jun 1, 2016

That convert stub problem is now fixed.

@sampsyo
Copy link
Member Author

sampsyo commented Jun 2, 2016

In 8264026, 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
Copy link
Member Author

sampsyo commented Jun 5, 2016

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
Copy link
Member Author

sampsyo commented Jun 6, 2016

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
Copy link
Member Author

sampsyo commented Jun 8, 2016

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
Copy link
Member

jackwilsdon commented Jun 8, 2016

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

@sampsyo
Copy link
Member Author

sampsyo commented Jun 10, 2016

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'

@ghost
Copy link

ghost commented Jun 10, 2016

can you run the command it runs manually with convert?

@sampsyo
Copy link
Member Author

sampsyo commented Jun 10, 2016

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
Copy link
Member

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

@sampsyo
Copy link
Member Author

sampsyo commented Jun 10, 2016

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
Copy link
Member

jackwilsdon commented Jun 10, 2016

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
Copy link
Member Author

sampsyo commented Jun 10, 2016

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 a0c38a0 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!

sampsyo added a commit that referenced this issue Jun 10, 2016
See #670 for details. This reverts a change from
a0c38a0. This caused problems on
Windows, but the fundamental problem is more general.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants