Add tests for BPD

[arcresu]

I'd like to work on updating the MPD server version supported by BPD (#800) so as a first step I'm adding some tests that exercise BPD. I found an existing file that was testing parts of the BPD plugin, though the name is a bit unexpected (test_player.py rather than, say, test_bpd.py) so perhaps this should be renamed.

The test code adds a simple TCP client that can speak to an MPD server. It provides a small number of convenience functions for building requests and interpreting responses but it intended to be pretty dumb. This way we can treat BPD as a black box and test how it responds over the protocol rather than unit testing the functions in the plugin's module. The real BPD server is started in a separate thread, and we have to wait a small amount of time for it to spin up before attempting to connect to it. Since starting the BPD server requires executing import gi this means the tests have to grow a dependency on the relevant GObject modules. Since I start the BPD server in the setUp method, we incur the wait-to-connect delay for each test. I set this to 0.1s and it's sufficient on my laptop but it might need to be longer on other hardware. (The client now polls the server until the socket is bound so there's not much wasted delay, and the overall timeout is longer.)

I've written tests based on the current MPD protocol specification, which is several versions ahead of where BPD is at. For this reason I've marked several of them as expected failures for now. The various test_implements_* tests are just checking that BPD claims to support all of the commands in each category of the full MPD list. They can perhaps be removed after we support all the commands, but I'm just using them as a progress meter for now to highlight what's missing from the implementation.

My plan is to add more tests and update BPD in subsequent PRs. As well as new commands there are new behaviours like a new more powerful filter syntax and additional flags for old commands. For now I just want to get this stub for the test framework merged.

[arcresu]

On Travis PyGObject is complaining about GLib being too old. Based on this commit is seems PyGObject 3.31.2 was the version where this last changed (and before that commit, the GLib version on Travis would be OK for PyGObject). On Appveyor the problem is about the cairo header being missing on Windows.

I could try and fix up the dependency configuration for the CIs to make this work (not sure what to do about cairo on Windows), but I think instead I'll change the test approach to use a dummy gstplayer module to avoid having this dependency at all. I'm not actually testing playback, so this should be fine.

[arcresu]

The tests failed on Windows because it's attempting to pickle the whole BPD server as part of spawning the server process, and that includes some un-picklable objects (locks for the db). I tried running the server in a threading.Thread instead of a multiprocessing.Process, but that seemed to interfere with how bluelet works.

I suppose I need to be setting up the library in the process that's spawned to run the server rather than trying to pass it in. That would probably mean losing access to some of the Library test helpers though. I had a first attempt at rewriting to do this but ran into problems with the socket connections being refused or not closing properly. I'll try again when I next have time to look at this.

[sampsyo]

This is awesome! Thank you for getting this started. It will be really really useful to have better test coverage of the server. I have two high-level suggestions:

• For mocking the player, it could be more convenient to use the mock module's patch functionality.
• Even just for convenience's sake, it would be great—although perhaps really hard—to avoid the need to launch a separate process (or thread) for the server. This could maybe be done by making the tests use Bluelet's async IO to communicate with the server, or by invoking commands "directly" instead of literally opening a socket for communication. That's probably too tricky to get right, so a good fallback would be to launch a subprocess that runs the server via the command line: that is, instead of "forking" the process with multiprocessing, use subprocess to launch a beet bpd command that starts the server "from scratch" using the same database file.

[arcresu]

Thanks for the review! I've had another go now using subprocess and it's passing the tests on my machine.

The difficulty is that whatever mocking of the player is done, it has to happen in the new process rather than the test process. Maybe I'm missing an easier way of doing that, but my hacky solution was the best way I could think of after trying a few things. I can't see a good way to use mock.patch in this case. My solution for now is:

1. During setUp I write python file in the test's temp_dir that does the necessary mocking and import mechanism hijacking before importing the real bpd plugin. (This could just be a real file in the test/ directory since it doesn't need to be dynamically created.)
2. I then configure beets to load this file as a plugin so that it gets executed.

The main inelegant bit is ensuring that the launched process has the correct python executable and paths. Looks like I didn't properly serialise the python path into the environment for Windows so I'll still have to sort that out.

[sampsyo]

Aha; you're right—that is way more complicated than I made it seem at first. Sorry to lead you astray! This does seem to work—at least, most of the way—but I'm no longer convinced it's any easier than the "fork" approach with multiprocessing that you were proposing before.

To get the correct Python executable, you can use sys.executable (but you probably already tried that).

[arcresu]

I think in the end I prefer the forking approach because I don't trust that I can launch a fresh process with exactly the same python environment otherwise. The detour wasn't wasted effort though since the server management is much more robust now.

I stumbled into an unexpected behaviour with paths on the BPD virtual filesystem under Windows. When listing directory contents with the lsinfo command, paths to directories are returned with a forward slash as the path separator, but paths to files are returned using a backslash. Attempting to add the backslash-separated paths to the BPD playlist causes an error. This might be the first bug flushed out by the new tests :)

As far as I'm concerned now that this is running it's ready to go in. I'll save the addition of new tests and changes in the BPD server itself for future PRs.

[sampsyo]

Awesome! Yes, this does indeed seem like the way to go. Thank you for continuing to dig into this!

When I try running these tests with nose locally, each one prints out an exception from the server that indicates that the client has disconnected:

Traceback (most recent call last):
  File "/Users/asampson/Documents/code/beets/beets/util/bluelet.py", line 343, in run
    value = event.fire()
  File "/Users/asampson/Documents/code/beets/beets/util/bluelet.py", line 508, in fire
    return self.conn.sock.recv(self.bufsize)
ConnectionResetError: [Errno 54] Connection reset by peer

To avoid this extra noise when running tests, maybe it's possible to kill the server before disconnecting? Or else disconnect the client by sending the "close" command first?

[arcresu]

I didn't see that connection reset message on Linux, but I see from the CI logs that there's a similar error on Windows. I thought the client and its socket were being garbage collected (and so closed) before the server was killed but I guess I was wrong. I've tried a different approach where the "test socket" is passed into the client rather than having the client open a fresh one. I'm then explicitly closing that socket before killing the server process.

I've been mainly using pytest locally, but I'm seeing now that with nose there are warnings about unittest.expectedFailure not being supported. It also seems that nose completely fails to collect the tests dynamically created with my implements() helper (pytest sees them just fine). In both cases this should be a temporary thing while updating the protocol.

I'm really not sure why the Python 2.7 tests started failing on the CI with an ascii encoding exception. I don't see how the latest commit changes any string handling, and the tests run fine on my Linux machine under Python 2.7.16

[arcresu]

It seems that the issue is something to do with the encoding of the source files. I can't see any problem with the files those commits touched and in fact rolling back to the exact commit that was previously passing CI is now failing CI. I can reproduce the error locally by adding setenv = LANG=C to tox.ini, and stop the error by making that LANG=en_US.UTF-8. That made no difference to either CI though. I think it must be something external to this MR responsible.

[sampsyo]

Sorry about those test failures! I tracked that down to a Tox bug that was introduced just a couple of days ago: tox-dev/tox#1234

I changed the Travis and Appveyor configurations to avoid the version with the bug, so future commits should not run into this problem.

[sampsyo]

And you're right—it does look like nose just doesn't support unittest.expectedFailure, and that the issue is known: nose-devs/nose#33

[arcresu]

Ok thanks for following up about the tox issues. I rebased my branch and switched the expectedFailure decorators for skip so that there's no spurious output on nose as far as I can see besides bpd printing Building directory tree...\n... done. each test. I've already been working on another branch to improve bpd itself including converting that message to use the debug logger.

It might seem that the test file is a bit helper-heavy, but it means that the actual tests are short and readable and provide useful errors when they fail. As I improve the test coverage the ratio of test code to helper code will improve. E.g. this test asserts that all of the responses were OK up to the one that is expected to fail, and takes care of starting the bpd server, adding the item to its playlist (and checking that response), and stopping the server:

    def test_cmd_playlistinfo(self):
        with self.run_bpd() as client:
            self._bpd_add(client, self.item1)
            responses = client.send_commands(
                    ('playlistinfo',),
                    ('playlistinfo', '0'),
                    ('playlistinfo', '200'))

        self._assert_failed(responses, bpd.ERROR_ARG, pos=2)

[sampsyo]

Awesome. This will be incredibly valuable to have, as you say, for improving BPD and increasing its protocol coverage. Thank you for all the work you've already put into this. ✨